| Message ID | 20230511175609.2091136-6-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 |
The 05/15/2023 15:37, Jonathan Cameron wrote: > On Thu, 11 May 2023 17:56:40 +0000 > Fan Ni <fan.ni@samsung.com> wrote: > > > From: Fan Ni <nifan@outlook.com> > > > > Per CXL spec 3.0, we implemented the two mailbox commands: > > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.8.9.3, and > > Release Dynamic Capacity Response (Opcode 4803h) 8.2.9.8.9.4. > > > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > > --- > > hw/cxl/cxl-mailbox-utils.c | 223 ++++++++++++++++++++++++++++++++++++ > > include/hw/cxl/cxl_device.h | 3 +- > > 2 files changed, 225 insertions(+), 1 deletion(-) > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > > index ed2ac154cb..7212934627 100644 > > --- a/hw/cxl/cxl-mailbox-utils.c > > +++ b/hw/cxl/cxl-mailbox-utils.c > > @@ -84,6 +84,8 @@ enum { > > DCD_CONFIG = 0x48, /*8.2.9.8.9*/ > > #define GET_DC_REGION_CONFIG 0x0 > > #define GET_DYN_CAP_EXT_LIST 0x1 > > + #define ADD_DYN_CAP_RSP 0x2 > > + #define RELEASE_DYN_CAP 0x3 > > PHYSICAL_SWITCH = 0x51 > > #define IDENTIFY_SWITCH_DEVICE 0x0 > > }; > > @@ -1069,6 +1071,221 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(struct cxl_cmd *cmd, > > return CXL_MBOX_SUCCESS; > > } > > > > +static inline int test_bits(const unsigned long *addr, int nr, int size) > > Not obvious what this does from the name. Please add some docs. > > > +{ > > + unsigned long res = find_next_zero_bit(addr, size + nr, nr); > > + > > + if (res >= nr + size) > > + return 1; > > + else > > + return 0; > > +} > > + > > +static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa > > Operates only on things in dev->dc, so perhaps pass that instead. dc is a struct defined inside CXLType3Dev struct, so will leave it as it is for now. Fan > > > + , uint64_t len) > > comma on previous line. > > > +{ > > + int8_t i = dev->dc.num_regions-1; > > + > > + while (i > 0 && dpa < dev->dc.regions[i].base) > > + i--; > > + > > + if (dpa < dev->dc.regions[i].base > > + || dpa + len > dev->dc.regions[i].base + dev->dc.regions[i].len) > > + return dev->dc.num_regions; > > + > > + return i; > > +} > > + > > +static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev, void *data) > > +{ > > + struct updated_dc_extent_list_in_pl { > This is same as used in next function. Pull it out of the functions and > use that type rather than mapping via a void *data pointer. > > > + uint32_t num_entries_updated; > > + uint8_t rsvd[4]; > > + struct { > > + uint64_t start_dpa; > > + uint64_t len; > > + uint8_t rsvd[8]; > > + } QEMU_PACKED updated_entries[]; > > + } QEMU_PACKED; > > + > > + struct updated_dc_extent_list_in_pl *in = data; > > + unsigned long *blk_bitmap; > > + uint64_t min_block_size = dev->dc.regions[0].block_size; > > + struct CXLDCD_Region *region = &dev->dc.regions[0]; > > + uint32_t i; > > + uint64_t dpa, len; > > + uint8_t rid; > > + > > + for (i = 1; i < dev->dc.num_regions; i++) { > > + region = &dev->dc.regions[i]; > > + if (min_block_size > region->block_size) > > + min_block_size = region->block_size; > > + } > > + blk_bitmap = bitmap_new((region->len + region->base > > + - dev->dc.regions[0].base) / min_block_size); > > + g_assert(blk_bitmap); > > Abort in bitmap_new() anyway so no need for this. Most qemu allocations > don't need to be checked as they fail hard anyway so we never get to the checks. > > > + bitmap_zero(blk_bitmap, (region->len + region->base > > + - dev->dc.regions[0].base) / min_block_size); > > bitmap_new() seems to use a g_malloc0 internally so no need to zero again here > I think. > > > + > > + for (i = 0; i < in->num_entries_updated; i++) { > > + dpa = in->updated_entries[i].start_dpa; > > + len = in->updated_entries[i].len; > > + > > + rid = find_region_id(dev, dpa, len); > > + if (rid == dev->dc.num_regions) { > > Use a goto and single cleanup path having set ret or similar to > the particular issue. > > > + g_free(blk_bitmap); > > + return CXL_MBOX_INVALID_PA; > > + } > > + region = &dev->dc.regions[rid]; > > + if (dpa % region->block_size || len % region->block_size) { > > + g_free(blk_bitmap); > > + return CXL_MBOX_INVALID_EXTENT_LIST; > goto from here as well.. etc. > > > + } > > + /* the dpa range already covered by some other extents in the list */ > > + if (test_bits(blk_bitmap, dpa/min_block_size, len/min_block_size)) { > > + g_free(blk_bitmap); > > + return CXL_MBOX_INVALID_EXTENT_LIST; > > + } > > + bitmap_set(blk_bitmap, dpa/min_block_size, len/min_block_size); > > + } > > + > > + g_free(blk_bitmap); > > + return CXL_MBOX_SUCCESS; > > +} > > + > > +/* > > + * cxl spec 3.0: 8.2.9.8.9.3 > > + * Add Dynamic Capacity Response (opcode 4802h) > > + * Assuming extent list is updated when a extent is added, when receiving > > + * the response, verify and ensure the extent is utilized by the host, and > > + * update extent list as needed. > > Double space in middle of sentence > > > + **/ > > +static CXLRetCode cmd_dcd_add_dyn_cap_rsp(struct cxl_cmd *cmd, > > + CXLDeviceState *cxl_dstate, > > + uint16_t *len_unused) > > +{ > > + struct add_dyn_cap_ext_list_in_pl { > > + uint32_t num_entries_updated; > > + uint8_t rsvd[4]; > > + struct { > > + uint64_t start_dpa; > > + uint64_t len; > > + uint8_t rsvd[8]; > > + } QEMU_PACKED updated_entries[]; > > These extent list entries keep turning up in the code. Pull that out of here > to be a general 'Extent list element' or similar. > > > + } QEMU_PACKED; > > + > > + struct add_dyn_cap_ext_list_in_pl *in = (void *)cmd->payload; > > + struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate); > > + CXLDCDExtentList *extent_list = &ct3d->dc.extents; > > + CXLDCD_Extent *ent; > > + uint32_t i; > > + uint64_t dpa, len; > > + CXLRetCode rs; > > + > > + if (in->num_entries_updated == 0) > > + return CXL_MBOX_SUCCESS; > > + > > + rs = detect_malformed_extent_list(ct3d, in); > > + if (rs != CXL_MBOX_SUCCESS) > > + return rs; > > + > > + for (i = 0; i < in->num_entries_updated; i++) { > > + dpa = in->updated_entries[i].start_dpa; > > + len = in->updated_entries[i].len; > > + > > + /* Todo: check following > > + * One or more of the updated extent lists contain Starting DPA > > + * or Lengths that are out of range of a current extent list > > + * maintained by the device. > > + **/ > > + > > + QTAILQ_FOREACH(ent, extent_list, node) { > > Add some comments here. Is this a repeated entry test? > > > + if (ent->start_dpa == dpa && ent->len == len) > > + return CXL_MBOX_INVALID_PA; > > + if (ent->start_dpa <= dpa > > + && dpa + len <= ent->start_dpa + ent->len) { > > Comment needed on this one. Why is it shrinking an existing entry? > > > + ent->start_dpa = dpa; > > + ent->len = len; > > + break; > > If you break here I think it will only result in return CXL_MBOX_SUCCESS so > might as well do that here. > > > + } else if ((dpa < ent->start_dpa + ent->len > > + && dpa + len > ent->start_dpa + ent->len) > > Above is new entry would contain existing one. > > > + || (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) > > + return CXL_MBOX_INVALID_EXTENT_LIST; > > + } > > + // a new extent added > > Why? This function generally need more documentation. > > > + if (!ent) { > > + ent = g_new0(CXLDCD_Extent, 1); > > + assert(ent); > > No need. g_new0 will already have blown up before you get here. > > > + ent->start_dpa = dpa; > > + ent->len = len; > > + memset(ent->tag, 0, 0x10); > Allocated empty just above, still empty so no need to set it again. > > + ent->shared_seq = 0; > > + QTAILQ_INSERT_TAIL(extent_list, ent, node); > > + } > > + } > > + > > + return CXL_MBOX_SUCCESS; > > +} > > + > > +/* > > + * Spec 3.0: 8.2.9.8.9.4 > > + * Release Dynamic Capacity (opcode 4803h) > > + **/ > > +static CXLRetCode cmd_dcd_release_dcd_capacity(struct cxl_cmd *cmd, > > + CXLDeviceState *cxl_dstate, > > + uint16_t *len_unused) > > +{ > > + struct release_dcd_cap_in_pl { > > + uint32_t num_entries_updated; > > + uint8_t rsvd[4]; > > + struct { > > + uint64_t start_dpa; > > + uint64_t len; > > + uint8_t rsvd[8]; > > + } QEMU_PACKED updated_entries[]; > > + } QEMU_PACKED; > > + > > + struct release_dcd_cap_in_pl *in = (void *)cmd->payload; > > + struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate); > > + CXLDCDExtentList *extent_list = &ct3d->dc.extents; > > + CXLDCD_Extent *ent; > > + uint32_t i; > > + uint64_t dpa, len; > > + CXLRetCode rs; > > + > > + if (in->num_entries_updated == 0) > > + return CXL_MBOX_INVALID_INPUT; > > + > > + rs = detect_malformed_extent_list(ct3d, in); > > + if (rs != CXL_MBOX_SUCCESS) > > + return rs; > > + > > + /* Todo: check following > > + * One or more of the updated extent lists contain Starting DPA > > + * or Lengths that are out of range of a current extent list > > + * maintained by the device. > > + **/ > > + > > + for (i = 0; i < in->num_entries_updated; i++) { > > + dpa = in->updated_entries[i].start_dpa; > > + len = in->updated_entries[i].len; > > + > > + QTAILQ_FOREACH(ent, extent_list, node) { > > + if (ent->start_dpa == dpa && ent->len == len) > > Do the remove here and comment on 'found entry' not needed. > Note I think you can release partial extents so that will need handling at some point. > > > + break; > > + else if ((dpa < ent->start_dpa + ent->len > > + && dpa + len > ent->start_dpa + ent->len) > > + || (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) > Comment on this condition and why it's a problem. > > > + return CXL_MBOX_INVALID_EXTENT_LIST; > > + } > > + /* found the entry, release it */ > > + if (ent) > > + QTAILQ_REMOVE(extent_list, ent, node); > > + } > > + > > + return CXL_MBOX_SUCCESS; > > +} > > + > > #define IMMEDIATE_CONFIG_CHANGE (1 << 1) > > #define IMMEDIATE_DATA_CHANGE (1 << 2) > > #define IMMEDIATE_POLICY_CHANGE (1 << 3) > > @@ -1112,6 +1329,12 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { > > [DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = { > > "DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list, > > 8, 0 }, > > + [DCD_CONFIG][ADD_DYN_CAP_RSP] = { > > + "ADD_DCD_DYNAMIC_CAPACITY_RESPONSE", cmd_dcd_add_dyn_cap_rsp, > > + ~0, IMMEDIATE_DATA_CHANGE }, > > + [DCD_CONFIG][RELEASE_DYN_CAP] = { > > + "RELEASE_DCD_DYNAMIC_CAPACITY", cmd_dcd_release_dcd_capacity, > > + ~0, IMMEDIATE_DATA_CHANGE }, > > }; > > > > 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 20ad5e7411..c0c8fcc24b 100644 > > --- a/include/hw/cxl/cxl_device.h > > +++ b/include/hw/cxl/cxl_device.h > > @@ -131,7 +131,8 @@ typedef enum { > > CXL_MBOX_INCORRECT_PASSPHRASE = 0x14, > > CXL_MBOX_UNSUPPORTED_MAILBOX = 0x15, > > CXL_MBOX_INVALID_PAYLOAD_LENGTH = 0x16, > > - CXL_MBOX_MAX = 0x17 > > + CXL_MBOX_INVALID_EXTENT_LIST = 0x17, > > 0x1e in the spec. > > > + CXL_MBOX_MAX = 0x18 > > } CXLRetCode; > > > > struct cxl_cmd; >
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index ed2ac154cb..7212934627 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -84,6 +84,8 @@ enum { DCD_CONFIG = 0x48, /*8.2.9.8.9*/ #define GET_DC_REGION_CONFIG 0x0 #define GET_DYN_CAP_EXT_LIST 0x1 + #define ADD_DYN_CAP_RSP 0x2 + #define RELEASE_DYN_CAP 0x3 PHYSICAL_SWITCH = 0x51 #define IDENTIFY_SWITCH_DEVICE 0x0 }; @@ -1069,6 +1071,221 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +static inline int test_bits(const unsigned long *addr, int nr, int size) +{ + unsigned long res = find_next_zero_bit(addr, size + nr, nr); + + if (res >= nr + size) + return 1; + else + return 0; +} + +static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa + , uint64_t len) +{ + int8_t i = dev->dc.num_regions-1; + + while (i > 0 && dpa < dev->dc.regions[i].base) + i--; + + if (dpa < dev->dc.regions[i].base + || dpa + len > dev->dc.regions[i].base + dev->dc.regions[i].len) + return dev->dc.num_regions; + + return i; +} + +static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev, void *data) +{ + struct updated_dc_extent_list_in_pl { + uint32_t num_entries_updated; + uint8_t rsvd[4]; + struct { + uint64_t start_dpa; + uint64_t len; + uint8_t rsvd[8]; + } QEMU_PACKED updated_entries[]; + } QEMU_PACKED; + + struct updated_dc_extent_list_in_pl *in = data; + unsigned long *blk_bitmap; + uint64_t min_block_size = dev->dc.regions[0].block_size; + struct CXLDCD_Region *region = &dev->dc.regions[0]; + uint32_t i; + uint64_t dpa, len; + uint8_t rid; + + for (i = 1; i < dev->dc.num_regions; i++) { + region = &dev->dc.regions[i]; + if (min_block_size > region->block_size) + min_block_size = region->block_size; + } + blk_bitmap = bitmap_new((region->len + region->base + - dev->dc.regions[0].base) / min_block_size); + g_assert(blk_bitmap); + bitmap_zero(blk_bitmap, (region->len + region->base + - dev->dc.regions[0].base) / min_block_size); + + for (i = 0; i < in->num_entries_updated; i++) { + dpa = in->updated_entries[i].start_dpa; + len = in->updated_entries[i].len; + + rid = find_region_id(dev, dpa, len); + if (rid == dev->dc.num_regions) { + g_free(blk_bitmap); + return CXL_MBOX_INVALID_PA; + } + region = &dev->dc.regions[rid]; + if (dpa % region->block_size || len % region->block_size) { + g_free(blk_bitmap); + return CXL_MBOX_INVALID_EXTENT_LIST; + } + /* the dpa range already covered by some other extents in the list */ + if (test_bits(blk_bitmap, dpa/min_block_size, len/min_block_size)) { + g_free(blk_bitmap); + return CXL_MBOX_INVALID_EXTENT_LIST; + } + bitmap_set(blk_bitmap, dpa/min_block_size, len/min_block_size); + } + + g_free(blk_bitmap); + return CXL_MBOX_SUCCESS; +} + +/* + * cxl spec 3.0: 8.2.9.8.9.3 + * Add Dynamic Capacity Response (opcode 4802h) + * Assuming extent list is updated when a extent is added, when receiving + * the response, verify and ensure the extent is utilized by the host, and + * update extent list as needed. + **/ +static CXLRetCode cmd_dcd_add_dyn_cap_rsp(struct cxl_cmd *cmd, + CXLDeviceState *cxl_dstate, + uint16_t *len_unused) +{ + struct add_dyn_cap_ext_list_in_pl { + uint32_t num_entries_updated; + uint8_t rsvd[4]; + struct { + uint64_t start_dpa; + uint64_t len; + uint8_t rsvd[8]; + } QEMU_PACKED updated_entries[]; + } QEMU_PACKED; + + struct add_dyn_cap_ext_list_in_pl *in = (void *)cmd->payload; + struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate); + CXLDCDExtentList *extent_list = &ct3d->dc.extents; + CXLDCD_Extent *ent; + uint32_t i; + uint64_t dpa, len; + CXLRetCode rs; + + if (in->num_entries_updated == 0) + return CXL_MBOX_SUCCESS; + + rs = detect_malformed_extent_list(ct3d, in); + if (rs != CXL_MBOX_SUCCESS) + return rs; + + for (i = 0; i < in->num_entries_updated; i++) { + dpa = in->updated_entries[i].start_dpa; + len = in->updated_entries[i].len; + + /* Todo: check following + * One or more of the updated extent lists contain Starting DPA + * or Lengths that are out of range of a current extent list + * maintained by the device. + **/ + + QTAILQ_FOREACH(ent, extent_list, node) { + if (ent->start_dpa == dpa && ent->len == len) + return CXL_MBOX_INVALID_PA; + if (ent->start_dpa <= dpa + && dpa + len <= ent->start_dpa + ent->len) { + ent->start_dpa = dpa; + ent->len = len; + break; + } else if ((dpa < ent->start_dpa + ent->len + && dpa + len > ent->start_dpa + ent->len) + || (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) + return CXL_MBOX_INVALID_EXTENT_LIST; + } + // a new extent added + if (!ent) { + ent = g_new0(CXLDCD_Extent, 1); + assert(ent); + ent->start_dpa = dpa; + ent->len = len; + memset(ent->tag, 0, 0x10); + ent->shared_seq = 0; + QTAILQ_INSERT_TAIL(extent_list, ent, node); + } + } + + return CXL_MBOX_SUCCESS; +} + +/* + * Spec 3.0: 8.2.9.8.9.4 + * Release Dynamic Capacity (opcode 4803h) + **/ +static CXLRetCode cmd_dcd_release_dcd_capacity(struct cxl_cmd *cmd, + CXLDeviceState *cxl_dstate, + uint16_t *len_unused) +{ + struct release_dcd_cap_in_pl { + uint32_t num_entries_updated; + uint8_t rsvd[4]; + struct { + uint64_t start_dpa; + uint64_t len; + uint8_t rsvd[8]; + } QEMU_PACKED updated_entries[]; + } QEMU_PACKED; + + struct release_dcd_cap_in_pl *in = (void *)cmd->payload; + struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate); + CXLDCDExtentList *extent_list = &ct3d->dc.extents; + CXLDCD_Extent *ent; + uint32_t i; + uint64_t dpa, len; + CXLRetCode rs; + + if (in->num_entries_updated == 0) + return CXL_MBOX_INVALID_INPUT; + + rs = detect_malformed_extent_list(ct3d, in); + if (rs != CXL_MBOX_SUCCESS) + return rs; + + /* Todo: check following + * One or more of the updated extent lists contain Starting DPA + * or Lengths that are out of range of a current extent list + * maintained by the device. + **/ + + for (i = 0; i < in->num_entries_updated; i++) { + dpa = in->updated_entries[i].start_dpa; + len = in->updated_entries[i].len; + + QTAILQ_FOREACH(ent, extent_list, node) { + if (ent->start_dpa == dpa && ent->len == len) + break; + else if ((dpa < ent->start_dpa + ent->len + && dpa + len > ent->start_dpa + ent->len) + || (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) + return CXL_MBOX_INVALID_EXTENT_LIST; + } + /* found the entry, release it */ + if (ent) + QTAILQ_REMOVE(extent_list, ent, node); + } + + return CXL_MBOX_SUCCESS; +} + #define IMMEDIATE_CONFIG_CHANGE (1 << 1) #define IMMEDIATE_DATA_CHANGE (1 << 2) #define IMMEDIATE_POLICY_CHANGE (1 << 3) @@ -1112,6 +1329,12 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { [DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = { "DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list, 8, 0 }, + [DCD_CONFIG][ADD_DYN_CAP_RSP] = { + "ADD_DCD_DYNAMIC_CAPACITY_RESPONSE", cmd_dcd_add_dyn_cap_rsp, + ~0, IMMEDIATE_DATA_CHANGE }, + [DCD_CONFIG][RELEASE_DYN_CAP] = { + "RELEASE_DCD_DYNAMIC_CAPACITY", cmd_dcd_release_dcd_capacity, + ~0, IMMEDIATE_DATA_CHANGE }, }; 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 20ad5e7411..c0c8fcc24b 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -131,7 +131,8 @@ typedef enum { CXL_MBOX_INCORRECT_PASSPHRASE = 0x14, CXL_MBOX_UNSUPPORTED_MAILBOX = 0x15, CXL_MBOX_INVALID_PAYLOAD_LENGTH = 0x16, - CXL_MBOX_MAX = 0x17 + CXL_MBOX_INVALID_EXTENT_LIST = 0x17, + CXL_MBOX_MAX = 0x18 } CXLRetCode; struct cxl_cmd;