Re: [PATCH v4 10/42] hw/cxl/device: Add log commands (8.2.9.4) + CEL

[Alex Bennée]

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> From: Ben Widawsky <ben.widawsky@intel.com>
>
> CXL specification provides for the ability to obtain logs from the
> device. Logs are either spec defined, like the "Command Effects Log"
> (CEL), or vendor specific. UUIDs are defined for all log types.
>
> The CEL is a mechanism to provide information to the host about which
> commands are supported. It is useful both to determine which spec'd
> optional commands are supported, as well as provide a list of vendor
> specified commands that might be used. The CEL is already created as
> part of mailbox initialization, but here it is now exported to hosts
> that use these log commands.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index cea4b2a59c..0ab0592e6c 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -46,6 +46,9 @@ enum {
>      TIMESTAMP   = 0x03,
>          #define GET           0x0
>          #define SET           0x1
> +    LOGS        = 0x04,
> +        #define GET_SUPPORTED 0x0
> +        #define GET_LOG       0x1
>  };
>  
>  /* 8.2.8.4.5.1 Command Return Codes */
> @@ -122,6 +125,8 @@ 
> define_mailbox_handler_zeroed(EVENTS_GET_INTERRUPT_POLICY, 4);
>  define_mailbox_handler_nop(EVENTS_SET_INTERRUPT_POLICY);
>  declare_mailbox_handler(TIMESTAMP_GET);
>  declare_mailbox_handler(TIMESTAMP_SET);
> +declare_mailbox_handler(LOGS_GET_SUPPORTED);
> +declare_mailbox_handler(LOGS_GET_LOG);
>  
>  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -137,6 +142,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
>      CXL_CMD(EVENTS, SET_INTERRUPT_POLICY, 4, IMMEDIATE_CONFIG_CHANGE),
>      CXL_CMD(TIMESTAMP, GET, 0, 0),
>      CXL_CMD(TIMESTAMP, SET, 8, IMMEDIATE_POLICY_CHANGE),
> +    CXL_CMD(LOGS, GET_SUPPORTED, 0, 0),
> +    CXL_CMD(LOGS, GET_LOG, 0x18, 0),
>  };
>  
>  #undef CXL_CMD
> @@ -188,6 +195,66 @@ define_mailbox_handler(TIMESTAMP_SET)
>  
>  QemuUUID cel_uuid;
>  
> +/* 8.2.9.4.1 */
> +define_mailbox_handler(LOGS_GET_SUPPORTED)
> +{

Here is where I get a bit wary of the define_mailbox_handler define
which from what I can tell just hides the declarations. This makes the
handling of things like *cmd rather opaque. There is an argument for the
boilerplate definitions (_nop and _zeroed) but perhaps not these.

> +    struct {
> +        uint16_t entries;
> +        uint8_t rsvd[6];
> +        struct {
> +            QemuUUID uuid;
> +            uint32_t size;
> +        } log_entries[1];
> +    } __attribute__((packed)) *supported_logs = (void *)cmd->payload;
> +    _Static_assert(sizeof(*supported_logs) == 0x1c, "Bad supported log 
> size");
> +
> +    supported_logs->entries = 1;
> +    supported_logs->log_entries[0].uuid = cel_uuid;
> +    supported_logs->log_entries[0].size = 4 * cxl_dstate->cel_size;
> +
> +    *len = sizeof(*supported_logs);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +/* 8.2.9.4.2 */
> +define_mailbox_handler(LOGS_GET_LOG)
> +{
> +    struct {
> +        QemuUUID uuid;
> +        uint32_t offset;
> +        uint32_t length;
> +    } __attribute__((packed, __aligned__(16))) *get_log = (void 
> *)cmd->payload;
> +
> +    /*
> +     * 8.2.9.4.2
> +     *   The device shall return Invalid Parameter if the Offset or Length
> +     *   fields attempt to access beyond the size of the log as reported by 
> Get
> +     *   Supported Logs.
> +     *
> +     * XXX: Spec is wrong, "Invalid Parameter" isn't a thing.
> +     * XXX: Spec doesn't address incorrect UUID incorrectness.
> +     *
> +     * The CEL buffer is large enough to fit all commands in the emulation, 
> so
> +     * the only possible failure would be if the mailbox itself isn't big
> +     * enough.
> +     */
> +    if (get_log->offset + get_log->length > cxl_dstate->payload_size) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) {
> +        return CXL_MBOX_UNSUPPORTED;
> +    }
> +
> +    /* Store off everything to local variables so we can wipe out the 
> payload */
> +    *len = get_log->length;
> +
> +    memmove(cmd->payload, cxl_dstate->cel_log + get_log->offset,
> +           get_log->length);
> +
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>  {
>      uint16_t ret = CXL_MBOX_SUCCESS;


-- 
Alex Bennée