Re: [PATCH v6 20/43] hw/cxl/device: Add some trivial commands

[Alex Bennée]

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

> From: Ben Widawsky <ben.widawsky@intel.com>
>
> GET_FW_INFO and GET_PARTITION_INFO, for this emulation, is equivalent to
> info already returned in the IDENTIFY command. To have a more robust
> implementation, add those.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 69 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 808faec114..d022711b2a 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -44,6 +44,8 @@ enum {
>          #define CLEAR_RECORDS   0x1
>          #define GET_INTERRUPT_POLICY   0x2
>          #define SET_INTERRUPT_POLICY   0x3
> +    FIRMWARE_UPDATE = 0x02,
> +        #define GET_INFO      0x0
>      TIMESTAMP   = 0x03,
>          #define GET           0x0
>          #define SET           0x1
> @@ -52,6 +54,8 @@ enum {
>          #define GET_LOG       0x1
>      IDENTIFY    = 0x40,
>          #define MEMORY_DEVICE 0x0
> +    CCLS        = 0x41,
> +        #define GET_PARTITION_INFO     0x0
>  };
>  
>  /* 8.2.8.4.5.1 Command Return Codes */
> @@ -114,6 +118,39 @@ DEFINE_MAILBOX_HANDLER_NOP(events_clear_records);
>  DEFINE_MAILBOX_HANDLER_ZEROED(events_get_interrupt_policy, 4);
>  DEFINE_MAILBOX_HANDLER_NOP(events_set_interrupt_policy);
>  
> +/* 8.2.9.2.1 */
> +static ret_code cmd_firmware_update_get_info(struct cxl_cmd *cmd,
> +                                             CXLDeviceState *cxl_dstate,
> +                                             uint16_t *len)
> +{
> +    struct {
> +        uint8_t slots_supported;
> +        uint8_t slot_info;
> +        uint8_t caps;
> +        uint8_t rsvd[0xd];
> +        char fw_rev1[0x10];
> +        char fw_rev2[0x10];
> +        char fw_rev3[0x10];
> +        char fw_rev4[0x10];
> +    } __attribute__((packed)) *fw_info;
> +    _Static_assert(sizeof(*fw_info) == 0x50, "Bad firmware info
> size");

note: we have QEMU_PACKED, QEMU_BUILD_BUG_ON and friends in compiler.h which are
preferred for potential compiler portability reasons.

> +
> +    if (cxl_dstate->pmem_size < (256 << 20)) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    fw_info = (void *)cmd->payload;
> +    memset(fw_info, 0, sizeof(*fw_info));
> +
> +    fw_info->slots_supported = 2;
> +    fw_info->slot_info = BIT(0) | BIT(3);
> +    fw_info->caps = 0;
> +    snprintf(fw_info->fw_rev1, 0x10, "BWFW VERSION %02d", 0);

Given you have a fixed string here could you not:

  pstrcpy(fw_info->fw_rev1, 0x10, "BWFW VERSION 0");
  
> +
> +    *len = sizeof(*fw_info);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  /* 8.2.9.3.1 */
>  static ret_code cmd_timestamp_get(struct cxl_cmd *cmd,
>                                    CXLDeviceState *cxl_dstate,
> @@ -260,6 +297,33 @@ static ret_code cmd_identify_memory_device(struct 
> cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static ret_code cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
> +                                           CXLDeviceState *cxl_dstate,
> +                                           uint16_t *len)
> +{
> +    struct {
> +        uint64_t active_vmem;
> +        uint64_t active_pmem;
> +        uint64_t next_vmem;
> +        uint64_t next_pmem;
> +    } __attribute__((packed)) *part_info = (void *)cmd->payload;
> +    _Static_assert(sizeof(*part_info) == 0x20, "Bad get partition info 
> size");
> +    uint64_t size = cxl_dstate->pmem_size;
> +
> +    if (!QEMU_IS_ALIGNED(size, 256 << 20)) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    /* PMEM only */
> +    part_info->active_vmem = 0;
> +    part_info->next_vmem = 0;
> +    part_info->active_pmem = size / (256 << 20);
> +    part_info->next_pmem = part_info->active_pmem;
> +
> +    *len = sizeof(*part_info);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
>  #define IMMEDIATE_LOG_CHANGE (1 << 4)
> @@ -273,15 +337,18 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_events_get_interrupt_policy, 0, 0 },
>      [EVENTS][SET_INTERRUPT_POLICY] = { "EVENTS_SET_INTERRUPT_POLICY",
>          cmd_events_set_interrupt_policy, 4, IMMEDIATE_CONFIG_CHANGE },
> +    [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
> +        cmd_firmware_update_get_info, 0, 0 },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, 
> IMMEDIATE_POLICY_CHANGE },
>      [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 
> 0, 0 },
>      [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
>      [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
>          cmd_identify_memory_device, 0, 0 },
> +    [CCLS][GET_PARTITION_INFO] = { "CCLS_GET_PARTITION_INFO",
> +        cmd_ccls_get_partition_info, 0, 0 },
>  };
>  
> -
>  void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>  {
>      uint16_t ret = CXL_MBOX_SUCCESS;


-- 
Alex Bennée