| Message ID | 20230511175609.2091136-3-fan.ni@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/7] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command | expand |
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? > + } 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? -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"
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" >
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[]; + } 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; + } + + *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"