Re: [RFC 2/7] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support

[address@hidden]

The 05/15/2023 14:58, Jonathan Cameron wrote:
> On Thu, 11 May 2023 16:53:23 -0500
> Nathan Fontenot <nafonten@amd.com> wrote:
> 
> > On 5/11/23 12:56, Fan Ni wrote:
> > > From: Fan Ni <nifan@outlook.com>
> > > 
> > > Per cxl spec 3.0, add dynamic capacity region representative based on
> > > Table 8-126 and extend the cxl type3 device definition to include dc 
> > > region
> > > information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> > > Configuration' mailbox support.
> > > 
> > > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > > ---
> > >  hw/cxl/cxl-mailbox-utils.c  | 68 +++++++++++++++++++++++++++++++++++++
> > >  include/hw/cxl/cxl_device.h | 16 +++++++++
> > >  2 files changed, 84 insertions(+)
> > > 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index 7ff4fbdf22..61c77e52d8 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -81,6 +81,8 @@ enum {
> > >          #define GET_POISON_LIST        0x0
> > >          #define INJECT_POISON          0x1
> > >          #define CLEAR_POISON           0x2
> > > + DCD_CONFIG = 0x48, /*8.2.9.8.9*/
> > > +         #define GET_DC_REGION_CONFIG   0x0
> > >      PHYSICAL_SWITCH = 0x51
> > >          #define IDENTIFY_SWITCH_DEVICE      0x0
> > >  };
> > > @@ -935,6 +937,70 @@ static CXLRetCode cmd_media_clear_poison(struct 
> > > cxl_cmd *cmd,
> > >      return CXL_MBOX_SUCCESS;
> > >  }
> > >  
> > > +/*
> > > + * cxl spec 3.0: 8.2.9.8.9.2
> > > + * Get Dynamic Capacity Configuration
> > > + **/
> > > +static CXLRetCode cmd_dcd_get_dyn_cap_config(struct cxl_cmd *cmd,
> > > +         CXLDeviceState *cxl_dstate,
> > > +         uint16_t *len)
> > > +{
> > > + struct get_dyn_cap_config_in_pl {
> > > +         uint8_t region_cnt;
> > > +         uint8_t start_region_id;
> > > + } QEMU_PACKED;
> > > +
> > > +    struct get_dyn_cap_config_out_pl {
> > > +         uint8_t num_regions;
> > > +         uint8_t rsvd1[7];
> > > +         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[];  
> > 
> > Could you declare CXLDCD_Region as QEMU_PACKED and use it here instead of
> > re-defining the region structure?
> 
> Could be done, but care needed on the endian conversions.  I wouldn't
> mind seeing this always held as little endian state though. We'd have
> done that anyway if it was a memory mapped command set rather than read
> via the mailbox so there is plenty of precedence.
> 
> Jonathan

I will leave it as it is for now.
> 
> > 
> > > + } QEMU_PACKED;
> > > +
> > > + struct get_dyn_cap_config_in_pl *in = (void *)cmd->payload;
> > > + struct get_dyn_cap_config_out_pl *out = (void *)cmd->payload;
> > > + struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, 
> > > cxl_dstate);
> > > + uint16_t record_count = 0, i = 0;
> > > + uint16_t out_pl_len;
> > > +
> > > + if (in->start_region_id >= ct3d->dc.num_regions)
> > > +         record_count = 0;
> > > + else if (ct3d->dc.num_regions - in->start_region_id < in->region_cnt)
> > > +         record_count = ct3d->dc.num_regions - in->start_region_id;
> > > + else
> > > +         record_count = in->region_cnt;
> > > +
> > > + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> > > + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > > +
> > > + memset(out, 0, out_pl_len);
> > > + out->num_regions = record_count;
> > > + for (; i < record_count; i++) {
> > > +         stq_le_p(&out->records[i].base,
> > > +                 ct3d->dc.regions[in->start_region_id+i].base);
> > > +         stq_le_p(&out->records[i].decode_len,
> > > +                 ct3d->dc.regions[in->start_region_id+i].decode_len);
> > > +         stq_le_p(&out->records[i].region_len,
> > > +                 ct3d->dc.regions[in->start_region_id+i].len);
> > > +         stq_le_p(&out->records[i].block_size,
> > > +                 ct3d->dc.regions[in->start_region_id+i].block_size);
> > > +         stl_le_p(&out->records[i].dsmadhandle,
> > > +                 ct3d->dc.regions[in->start_region_id+i].dsmadhandle);
> > > +         out->records[i].flags
> > > +                 = ct3d->dc.regions[in->start_region_id+i].flags;  
> > 
> > In this loop your reading from 'in' and writing to 'out' where in and out 
> > both
> > point to the same payload buffer. It works because of the structure layouts 
> > but
> > feels like a bug waiting to happen. Perhaps saving start_region to a local 
> > variable
> > and using that for the loop?
> 
> Does it work?  There is a memset of out above.
> Definitely need a local copy of start_region_id before that.
> This might only be working because of good fortune / compilers being 'clever'.
> 
> Jonathan

Yes. We need a local variable to record the start_region_id.

Thanks Nathan for catching the issue.

Fan
> 
> 
> > 
> > -Nathan
> > 
> > > + }
> > > +
> > > + *len = 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)
> > > @@ -973,6 +1039,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> > >          cmd_media_inject_poison, 8, 0 },
> > >      [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
> > >          cmd_media_clear_poison, 72, 0 },
> > > + [DCD_CONFIG][GET_DC_REGION_CONFIG] = { "DCD_GET_DC_REGION_CONFIG",
> > > +         cmd_dcd_get_dyn_cap_config, 2, 0 },
> > >  };
> > >  
> > >  static struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > index e285369693..8a04e53e90 100644
> > > --- a/include/hw/cxl/cxl_device.h
> > > +++ b/include/hw/cxl/cxl_device.h
> > > @@ -383,6 +383,17 @@ typedef struct CXLPoison {
> > >  typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> > >  #define CXL_POISON_LIST_LIMIT 256
> > >  
> > > +#define DCD_MAX_REGION_NUM 8
> > > +
> > > +typedef struct CXLDCD_Region {
> > > + uint64_t base;
> > > + uint64_t decode_len; /* in multiples of 256MB */
> > > + uint64_t len;
> > > + uint64_t block_size;
> > > + uint32_t dsmadhandle;
> > > + uint8_t flags;
> > > +} CXLDCD_Region;
> > > +
> > >  struct CXLType3Dev {
> > >      /* Private */
> > >      PCIDevice parent_obj;
> > > @@ -414,6 +425,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; // 1-8
> > > +         struct CXLDCD_Region regions[DCD_MAX_REGION_NUM];
> > > + } dc;
> > >  };
> > >  
> > >  #define TYPE_CXL_TYPE3 "cxl-type3"  
> 

-- 
Fan Ni <nifan@outlook.com>