Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support

[fan <nifan.cxl@gmail.com>]

On Mon, Feb 26, 2024 at 05:33:17PM +0000, Jonathan Cameron wrote:
> On Wed, 21 Feb 2024 10:15:55 -0800
> nifan.cxl@gmail.com wrote:
> 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > Per cxl spec r3.1, add dynamic capacity region representative based on
> > Table 8-165 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> > 
> > Note: decode_len of a dc region is aligned to 256*MiB, divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > mailbox command.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> Hi Fan,
> 
> A few comments inline.
> 
> Jonathan
> 
> > ---
> >  hw/cxl/cxl-mailbox-utils.c  | 110 ++++++++++++++++++++++++++++++++++++
> >  include/hw/cxl/cxl_device.h |  16 ++++++
> >  2 files changed, 126 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index ba1d9901df..88e3b733e3 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -22,6 +22,7 @@
> >  
> >  #define CXL_CAPACITY_MULTIPLIER   (256 * MiB)
> >  #define CXL_DC_EVENT_LOG_SIZE 8
> > +#define CXL_SPEC_AFTER_R30
> As below. Drop this.  Kernel code needs to be able to cope with newer specs
> than it understands anyway so should be fine with the larger records (otherwise
> it's buggy and needs fixing!) 

Will remove it.
> 
> >  
> >  /*
> >   * How to add a new command, example. The command set FOO, with cmd BAR.
> > @@ -80,6 +81,8 @@ enum {
> >          #define GET_POISON_LIST        0x0
> >          #define INJECT_POISON          0x1
> >          #define CLEAR_POISON           0x2
> > +    DCD_CONFIG  = 0x48,
> > +        #define GET_DC_CONFIG          0x0
> >      PHYSICAL_SWITCH = 0x51,
> >          #define IDENTIFY_SWITCH_DEVICE      0x0
> >          #define GET_PHYSICAL_PORT_STATE     0x1
> > @@ -1238,6 +1241,103 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> >      return CXL_MBOX_SUCCESS;
> >  }
> >  
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.1: Get Dynamic Capacity Configuration
> > + * (Opcode: 4800h)
> > + */
> > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > +                                             uint8_t *payload_in,
> > +                                             size_t len_in,
> > +                                             uint8_t *payload_out,
> > +                                             size_t *len_out,
> > +                                             CXLCCI *cci)
> > +{
> > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > +    struct get_dyn_cap_config_in_pl {
> Type not needed - see below. 
> > +        uint8_t region_cnt;
> > +        uint8_t start_region_id;
> > +    } QEMU_PACKED;
>    } QEMU_PACKED *in;

Get your point, it seems I followed the wrong example like
cmd_media_clear_poison and cmd_inject_poison :-<

Fan

> 
> > +
> > +    struct get_dyn_cap_config_out_pl {
> Don't think giving this a type is necessary - see below.
> > +        uint8_t num_regions;
> > +        uint8_t regions_returned;
> > +        uint8_t rsvd1[6];
> > +        struct {
> > +            uint64_t base;
> > +            uint64_t decode_len;
> > +            uint64_t region_len;
> > +            uint64_t block_size;
> > +            uint32_t dsmadhandle;
> > +            uint8_t flags;
> > +            uint8_t rsvd2[3];
> > +        } QEMU_PACKED records[];
> > +        /*
> > +         * if cxl spec version >= 3.1, extra output payload as defined
> > +         * in struct get_dyn_cap_config_out_pl_extra comes here.
> > +         */
> > +    } QEMU_PACKED;
>     } QEMU_PACKED *out;
> > +
> > +    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > +    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> 
> We've (mostly) use the (void *) casting where we haven't given the structures
> a type.  I think I'd prefer we kept to that style for consistency.
> 
> There is an argument we should have given all these types
> for readability reasons and to avoid casting via void * but
> we have gone this way now - with the exception of
> the poison list - oops.   
> 
> > +    uint16_t record_count = 0;
> > +    uint16_t i;
> > +    uint16_t out_pl_len;
> > +    uint8_t start_region_id = in->start_region_id;
> > +#ifdef CXL_SPEC_AFTER_R30
> 
> Handy for testing, but I'd drop the ifdef for the final
> version.  We don't need to support old specs.
> 
> > +    struct get_dyn_cap_config_out_pl_extra {
> > +        uint32_t num_extents_supported;
> > +        uint32_t num_extents_available;
> > +        uint32_t num_tags_supported;
> > +        uint32_t num_tags_available;
> > +    } QEMU_PACKED;
> > +    struct get_dyn_cap_config_out_pl_extra *extra_out;
> As above, anonymous structure should work ok.
> > +#endif
> > +
> > +    if (start_region_id >= ct3d->dc.num_regions) {
> > +        return CXL_MBOX_INVALID_INPUT;
> > +    }
> > +
> > +    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > +            in->region_cnt);
> > +
> > +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> > +#ifdef CXL_SPEC_AFTER_R30
> > +    extra_out = (struct get_dyn_cap_config_out_pl_extra *)(payload_out +
> > +            out_pl_len);
> > +    out_pl_len += sizeof(struct get_dyn_cap_config_out_pl_extra);
> > +#endif
> > +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > +
> > +    out->num_regions = ct3d->dc.num_regions;
> > +#ifdef CXL_SPEC_AFTER_R30
> > +    out->regions_returned = record_count;
> > +#endif
> > +    for (i = 0; i < record_count; i++) {
> > +        stq_le_p(&out->records[i].base,
> > +                ct3d->dc.regions[start_region_id + i].base);
> Qemu allows a couple of style choices, but don't think this matches any of them.
> https://elixir.bootlin.com/qemu/latest/source/docs/devel/style.rst#L79
> 
> My preference is immediately after opening bracket.
> 
> 
> > +        stq_le_p(&out->records[i].decode_len,
> > +                ct3d->dc.regions[start_region_id + i].decode_len /
> > +                CXL_CAPACITY_MULTIPLIER);
> > +        stq_le_p(&out->records[i].region_len,
> > +                ct3d->dc.regions[start_region_id + i].len);
> > +        stq_le_p(&out->records[i].block_size,
> > +                ct3d->dc.regions[start_region_id + i].block_size);
> > +        stl_le_p(&out->records[i].dsmadhandle,
> > +                ct3d->dc.regions[start_region_id + i].dsmadhandle);
> > +        out->records[i].flags = ct3d->dc.regions[start_region_id + i].flags;
> > +    }
> > +#ifdef CXL_SPEC_AFTER_R30
> > +    /*FIXME: need to set valid values in the future*/
> 
> We need to do that before upstreaming..
> For tags, 0 is fine for now.
> 
> For extents allow say 512 and count how many are in lists
> I guess then subtract that.
> 
> > +    stq_le_p(&extra_out->num_extents_supported, 0);
> > +    stq_le_p(&extra_out->num_extents_available, 0);
> > +    stq_le_p(&extra_out->num_tags_supported, 0);
> > +    stq_le_p(&extra_out->num_tags_available, 0);
> > +#endif
> > +
> > +    *len_out = out_pl_len;
> > +    return CXL_MBOX_SUCCESS;
> > +}
> > +
> >  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> >  #define IMMEDIATE_DATA_CHANGE (1 << 2)
> >  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> > @@ -1282,6 +1382,11 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> >          cmd_media_clear_poison, 72, 0 },
> >  };
> >  
> > +static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> > +    [DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
> > +        cmd_dcd_get_dyn_cap_config, 2, 0 },
> > +};
> > +
> >  static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> >      [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
> >      [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
> > @@ -1487,7 +1592,12 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> >  
> >  void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> >  {
> > +    CXLType3Dev *ct3d = CXL_TYPE3(d);
> > +
> >      cxl_copy_cci_commands(cci, cxl_cmd_set);
> > +    if (ct3d->dc.num_regions) {
> > +        cxl_copy_cci_commands(cci, cxl_cmd_set_dcd);
> > +    }
> >      cci->d = d;
> >  
> >      /* No separation for PCI MB as protocol handled in PCI device */
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 3cf3077afa..6df7fecdf1 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> >  typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> >  #define CXL_POISON_LIST_LIMIT 256
> >  
> > +#define DCD_MAX_REGION_NUM 8
> Really trivial but call this 
> 
> DCD_MAX_NUM_REGION because it's the maximum number of
> regions. The maximum region number is 7 (as zero indexed).
> 
> > +
> > +typedef struct CXLDCDRegion {
> > +    uint64_t base;
> Probably makes sense to call out that bas also aligned to 256 MiB
> as not immediately clear the comment applies to both.
> Easiest will be to duplicate it.
> > +    uint64_t decode_len; /* aligned to 256*MiB */
> > +    uint64_t len;
> > +    uint64_t block_size;
> > +    uint32_t dsmadhandle;
> > +    uint8_t flags;
> > +} CXLDCDRegion;
> > +
> >  struct CXLType3Dev {
> >      /* Private */
> >      PCIDevice parent_obj;
> > @@ -454,6 +465,11 @@ struct CXLType3Dev {
> >      unsigned int poison_list_cnt;
> >      bool poison_list_overflowed;
> >      uint64_t poison_list_overflow_ts;
> > +
> > +    struct dynamic_capacity {
> > +        uint8_t num_regions; /* 0-8 regions */
> > +        CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> > +    } dc;
> >  };
> >  
> >  #define TYPE_CXL_TYPE3 "cxl-type3"
>