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

[Jonathan Cameron]

On Tue, 01 Mar 2022 18:46:30 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> 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>

Hi Alex,

> > ---
> >  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.

Replaced all instances of alignment, packed and Static_assert in the
patch set with the compiler.h equivalents. Not that has minor
affect on some earlier patch sets but feels trivial enough
that I've kept RBs etc.

> 
> > +
> > +    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");

Make sense.

>   
> > +
> > +    *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 },
> >  };
> >  
> > -
Also fixed this bit of rebasing mess up.

Thanks,

Jonathan

> >  void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> >  {
> >      uint16_t ret = CXL_MBOX_SUCCESS;  
> 
>