| Message ID | 20250123050903.92336-3-vinayak.kh@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL CCI Media Operations | expand |
On Thu, 23 Jan 2025 10:39:03 +0530 Vinayak Holikatti <vinayak.kh@samsung.com> wrote: > CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands. > CXL devices supports media operations Sanitize and Write zero command. As before, don't indent this. > > Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com> > --- > hw/cxl/cxl-mailbox-utils.c | 217 ++++++++++++++++++++++++++++++++++-- > include/hw/cxl/cxl_device.h | 11 ++ > 2 files changed, 220 insertions(+), 8 deletions(-) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index 2315d07fb1..89847ddd9d 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -1722,6 +1722,145 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd, > return CXL_MBOX_BG_STARTED; > } > > +#define DPA_RANGE_GRANULARITY 64 Could use existing CXL_CACHELINE_SIZE definition for this, though I guess strictly speaking it could be unrelated. > +struct dpa_range_list_entry { > + uint64_t starting_dpa; > + uint64_t length; > +}; > + > +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr, > + size_t length) > +{ > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > + int rc = 0; > + > + if ((dpa_addr % DPA_RANGE_GRANULARITY) || > + (length % DPA_RANGE_GRANULARITY)) { Probably makes sense to also check for length 0 here as that would be very odd if sent. > + return -EINVAL; > + } > + > + if (ct3d->hostvmem) { > + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > + vmr_size = memory_region_size(vmr); > + } > + if (ct3d->hostpmem) { > + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > + pmr_size = memory_region_size(pmr); > + } > + if (ct3d->dc.host_dc) { > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > + dc_size = memory_region_size(dc_mr); > + } > + > + if (!vmr && !pmr && !dc_mr) { > + return -ENODEV; > + } > + > + if (dpa_addr >= vmr_size + pmr_size + dc_size || > + (dpa_addr + length) > vmr_size + pmr_size + dc_size) { If length is checked as non zero above, only the second test is needed. > + return -EINVAL; > + } > + > + if (dpa_addr > vmr_size + pmr_size) { > + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { > + return -ENODEV; > + } > + } > + > + > + return rc; return 0. rc is never set to anything else. > +} > + > +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length, > + uint8_t fill_value) > +{ > + > + MemoryRegion *vmr = NULL, *pmr = NULL; > + uint64_t vmr_size = 0, pmr_size = 0; > + AddressSpace *as = NULL; > + MemTxAttrs mem_attrs = {0}; > + > + if (ct3d->hostvmem) { > + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > + vmr_size = memory_region_size(vmr); > + } > + if (ct3d->hostpmem) { > + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > + pmr_size = memory_region_size(pmr); > + } > + > + if (dpa_addr < vmr_size) { > + as = &ct3d->hostvmem_as; > + } else if (dpa_addr < vmr_size + pmr_size) { > + as = &ct3d->hostpmem_as; > + } else { > + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { > + return -ENODEV; > + } > + as = &ct3d->dc.host_dc_as; > + } You could factor out everything down to here and then use that for the validate_dpa_addr() as finding an address space means we also checked the address is valid. Otherwise it does not match. > + > + return address_space_set(as, dpa_addr, odd spacing after return. Should just be one space. > + fill_value, length, mem_attrs); > +} > + > +/* Perform the actual device zeroing */ > +static void __do_sanitize(CXLType3Dev *ct3d) > +{ > + struct CXLSanitizeInfo *san_info = ct3d->media_op_sanitize; Single space only before *san_info > + int dpa_range_count = san_info->dpa_range_count; > + int rc = 0; > + > + for (int i = 0; i < dpa_range_count; i++) { > + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, > + san_info->dpa_range_list[i].length, san_info->fill_value); > + if (rc) { > + goto exit; > + } > + } > + cxl_discard_all_event_records(&ct3d->cxl_dstate); Add a comment on why we are deleting event records when sanitizing a small part of memory? > +exit: > + g_free(ct3d->media_op_sanitize); > + ct3d->media_op_sanitize = NULL; > + return; > +} > + > +static int get_sanitize_duration(uint64_t total_mem) Where did this come from? Factor out the existing code in cmd_santize_overwrite() instead of duplicating this stack of if/else if Ideally do that as a precursor patch as it's just code movement. > +{ > + int secs = 0; > + > + if (total_mem <= 512) { > + secs = 4; > + } else if (total_mem <= 1024) { > + secs = 8; > + } else if (total_mem <= 2 * 1024) { > + secs = 15; > + } else if (total_mem <= 4 * 1024) { > + secs = 30; > + } else if (total_mem <= 8 * 1024) { > + secs = 60; > + } else if (total_mem <= 16 * 1024) { > + secs = 2 * 60; > + } else if (total_mem <= 32 * 1024) { > + secs = 4 * 60; > + } else if (total_mem <= 64 * 1024) { > + secs = 8 * 60; > + } else if (total_mem <= 128 * 1024) { > + secs = 15 * 60; > + } else if (total_mem <= 256 * 1024) { > + secs = 30 * 60; > + } else if (total_mem <= 512 * 1024) { > + secs = 60 * 60; > + } else if (total_mem <= 1024 * 1024) { > + secs = 120 * 60; > + } else { > + secs = 240 * 60; /* max 4 hrs */ > + } > + > + return secs; > +} > + > enum { > MEDIA_OP_GENERAL = 0x0, > MEDIA_OP_SANITIZE = 0x1, > @@ -1729,10 +1868,9 @@ enum { > } MEDIA_OPERATION_CLASS; > > enum { > - MEDIA_OP_SUB_DISCOVERY = 0x0, > - MEDIA_OP_SUB_SANITIZE = 0x0, > - MEDIA_OP_SUB_ZERO = 0x1, > - MEDIA_OP_SUB_CLASS_MAX > + MEDIA_OP_GEN_DISCOVERY = 0x0, > + MEDIA_OP_SAN_SANITIZE = 0x0, > + MEDIA_OP_SAN_ZERO = 0x1, See naming suggestions in first patch. Make sure to tidy up so you don't introduce them there then change them here! > } MEDIA_OPERATION_SUB_CLASS; > > struct media_op_supported_list_entry { > @@ -1777,9 +1915,14 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, > }; > } QEMU_PACKED *media_op_in_pl = (void *)payload_in; > > + Blank line here doesn't add anything. > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); > uint8_t media_op_cl = media_op_in_pl->media_operation_class; > uint8_t media_op_subclass = media_op_in_pl->media_operation_subclass; > uint32_t dpa_range_count = media_op_in_pl->dpa_range_count; > + uint8_t fill_value = 0; > + uint64_t total_mem = 0; > + int secs = 0; > > if (len_in < sizeof(*media_op_in_pl)) { > return CXL_MBOX_INVALID_PAYLOAD_LENGTH; > @@ -1788,7 +1931,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, > switch (media_op_cl) { > case MEDIA_OP_GENERAL: > switch (media_op_subclass) { > - case MEDIA_OP_SUB_DISCOVERY: > + case MEDIA_OP_GEN_DISCOVERY: > int count = 0; > struct media_op_discovery_out_pl *media_out_pl = > (void *)payload_out; > @@ -1806,7 +1949,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, > return CXL_MBOX_INVALID_INPUT; > } > > - media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER; > + media_out_pl->dpa_range_granularity = DPA_RANGE_GRANULARITY; > media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS; > if (num_ops > 0) { > for (int i = start_index; i < MAX_SUPPORTED_OPS; i++) { > @@ -1824,22 +1967,73 @@ disc_out: > media_out_pl->num_of_supported_operations = count; > *len_out = sizeof(struct media_op_discovery_out_pl) + > (sizeof(struct media_op_supported_list_entry) * count); > - break; > + goto exit; return CXL_MBOX_SUCCESS; No purpose in having the exit label as nothing to be done. > default: > return CXL_MBOX_UNSUPPORTED; > } > break; > case MEDIA_OP_SANITIZE: > + { From code here not obvious why scoping {} needed. > switch (media_op_subclass) { > - > + case MEDIA_OP_SAN_SANITIZE: > + if (len_in < (sizeof(*media_op_in_pl) + > + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { > + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; > + } The check applies to all current cases. So move it outside this switch statement to remove duplication. Here all you should do is set the fill_value; > + fill_value = 0xF; > + goto sanitize_range; > + case MEDIA_OP_SAN_ZERO: > + if (len_in < (sizeof(*media_op_in_pl) + > + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { > + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; > + } > + fill_value = 0; > + goto sanitize_range; Why goto. Just break... > default: > return CXL_MBOX_UNSUPPORTED; > } and have the stuff below under the sanitize_range label here. > break; > + } > default: > return CXL_MBOX_UNSUPPORTED; > } > > +sanitize_range: > + if (dpa_range_count > 0) { > + for (int i = 0; i < dpa_range_count; i++) { > + if (validate_dpa_addr(ct3d, > + media_op_in_pl->dpa_range_list[i].starting_dpa, > + media_op_in_pl->dpa_range_list[i].length)) { > + return CXL_MBOX_INVALID_INPUT; > + } > + total_mem += media_op_in_pl->dpa_range_list[i].length; > + } > + ct3d->media_op_sanitize = g_malloc0(sizeof(struct CXLSanitizeInfo) + > + (dpa_range_count * > + sizeof(struct dpa_range_list_entry))); > + > + if (ct3d->media_op_sanitize) { > + ct3d->media_op_sanitize->dpa_range_count = dpa_range_count; > + ct3d->media_op_sanitize->fill_value = fill_value; > + memcpy(ct3d->media_op_sanitize->dpa_range_list, > + media_op_in_pl->dpa_range_list, > + (dpa_range_count * > + sizeof(struct dpa_range_list_entry))); > + secs = get_sanitize_duration(total_mem >> 20); > + goto start_bg; > + } > + } else if (dpa_range_count == 0) { > + goto exit; if (dpa_range_count == 0) { return CXL_MBOX_SUCCESS; } for (i = 0; i < dpa_range_count; i++) //that is return early in the simple case the no need for else and the extra level of indent on these long lines. > + } > + > +start_bg: > + /* EBUSY other bg cmds as of now */ > + cci->bg.runtime = secs * 1000UL; > + *len_out = 0; > + /* sanitize when done */ > + cxl_dev_disable_media(&ct3d->cxl_dstate); Why? This is santizing part of the device. As I undestand it the main aim is to offload cleanup when the device is in use. Definitely don't want to disable media. If I'm missing a reason please give a spec reference. > + return CXL_MBOX_BG_STARTED; > +exit: > return CXL_MBOX_SUCCESS; > } > > @@ -3154,6 +3348,13 @@ static void bg_timercb(void *opaque) > cxl_dev_enable_media(&ct3d->cxl_dstate); > } > break; > + case 0x4402: /* Media Operations sanitize */ > + { > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); > + __do_sanitize(ct3d); > + cxl_dev_enable_media(&ct3d->cxl_dstate); Ah. You turn it back on here. Specification reference needed. > + } > + break; > case 0x4304: /* scan media */ > { > CXLType3Dev *ct3d = CXL_TYPE3(cci->d); > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > index a64739be25..6d82eb266a 100644 > --- a/include/hw/cxl/cxl_device.h > +++ b/include/hw/cxl/cxl_device.h > @@ -581,6 +581,15 @@ typedef struct CXLSetFeatureInfo { > size_t data_size; > } CXLSetFeatureInfo; > > +struct CXLSanitizeInfo { > + uint32_t dpa_range_count; > + uint8_t fill_value; > + struct { > + uint64_t starting_dpa; > + uint64_t length; > + } dpa_range_list[0]; [] > +}; > + > struct CXLType3Dev { > /* Private */ > PCIDevice parent_obj; > @@ -651,6 +660,8 @@ struct CXLType3Dev { > uint8_t num_regions; /* 0-8 regions */ > CXLDCRegion regions[DCD_MAX_NUM_REGION]; > } dc; > + > + struct CXLSanitizeInfo *media_op_sanitize; Here we just need to know there is a definition of struct CXLSantizeInfo not what form it takes. So use a forwards defintion and push the definition of struct CXLSanitizeInfo down to where it is needed only (in the c file). Thanks, Jonathan > }; > > #define TYPE_CXL_TYPE3 "cxl-type3"
On Fri, Jan 24, 2025 at 03:19:46PM +0000, Jonathan Cameron wrote: > On Thu, 23 Jan 2025 10:39:03 +0530 > Vinayak Holikatti <vinayak.kh@samsung.com> wrote: > > > CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands. > > CXL devices supports media operations Sanitize and Write zero command. > > As before, don't indent this. > > > > > Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com> > > --- > > hw/cxl/cxl-mailbox-utils.c | 217 ++++++++++++++++++++++++++++++++++-- > > include/hw/cxl/cxl_device.h | 11 ++ > > 2 files changed, 220 insertions(+), 8 deletions(-) > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > > index 2315d07fb1..89847ddd9d 100644 > > --- a/hw/cxl/cxl-mailbox-utils.c > > +++ b/hw/cxl/cxl-mailbox-utils.c > > @@ -1722,6 +1722,145 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd, > > return CXL_MBOX_BG_STARTED; > > } > > > > +#define DPA_RANGE_GRANULARITY 64 > > Could use existing CXL_CACHELINE_SIZE definition for this, though I guess > strictly speaking it could be unrelated. > > > +struct dpa_range_list_entry { > > + uint64_t starting_dpa; > > + uint64_t length; > > +}; > > + > > +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr, > > + size_t length) > > +{ > > + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; > > + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; > > + int rc = 0; > > + > > + if ((dpa_addr % DPA_RANGE_GRANULARITY) || > > + (length % DPA_RANGE_GRANULARITY)) { > Probably makes sense to also check for length 0 here as that would > be very odd if sent. > > + return -EINVAL; > > + } > > + > > + if (ct3d->hostvmem) { > > + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > + vmr_size = memory_region_size(vmr); > > + } > > + if (ct3d->hostpmem) { > > + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > + pmr_size = memory_region_size(pmr); > > + } > > + if (ct3d->dc.host_dc) { > > + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); > > + dc_size = memory_region_size(dc_mr); > > + } > > + > > + if (!vmr && !pmr && !dc_mr) { > > + return -ENODEV; > > + } > > + > > + if (dpa_addr >= vmr_size + pmr_size + dc_size || > > + (dpa_addr + length) > vmr_size + pmr_size + dc_size) { > > If length is checked as non zero above, only the second test is needed. > > > + return -EINVAL; > > + } > > + > > + if (dpa_addr > vmr_size + pmr_size) { > > + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { > > + return -ENODEV; > > + } > > + } > > + > > + > > + return rc; > > return 0. rc is never set to anything else. > > > +} > > + > > +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length, > > + uint8_t fill_value) > > +{ > > + > > + MemoryRegion *vmr = NULL, *pmr = NULL; > > + uint64_t vmr_size = 0, pmr_size = 0; > > + AddressSpace *as = NULL; > > + MemTxAttrs mem_attrs = {0}; > > + > > + if (ct3d->hostvmem) { > > + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > + vmr_size = memory_region_size(vmr); > > + } > > + if (ct3d->hostpmem) { > > + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > + pmr_size = memory_region_size(pmr); > > + } > > + > > + if (dpa_addr < vmr_size) { > > + as = &ct3d->hostvmem_as; > > + } else if (dpa_addr < vmr_size + pmr_size) { > > + as = &ct3d->hostpmem_as; > > + } else { > > + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { > > + return -ENODEV; > > + } > > + as = &ct3d->dc.host_dc_as; > > + } > > You could factor out everything down to here and then use that > for the validate_dpa_addr() as finding an address space means > we also checked the address is valid. Otherwise it does not match. > > > + > > + return address_space_set(as, dpa_addr, > > odd spacing after return. Should just be one space. > > > + fill_value, length, mem_attrs); > > +} > > + > > +/* Perform the actual device zeroing */ > > +static void __do_sanitize(CXLType3Dev *ct3d) > > +{ > > + struct CXLSanitizeInfo *san_info = ct3d->media_op_sanitize; > > Single space only before *san_info > > > + int dpa_range_count = san_info->dpa_range_count; > > + int rc = 0; > > + > > + for (int i = 0; i < dpa_range_count; i++) { > > + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, > > + san_info->dpa_range_list[i].length, san_info->fill_value); > > + if (rc) { > > + goto exit; > > + } > > + } > > + cxl_discard_all_event_records(&ct3d->cxl_dstate); > > Add a comment on why we are deleting event records when sanitizing a small > part of memory? > See response below for disabling the media state. Same section referenced below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome depends on how we interpret "follow the method described in 8.2.10.9.5.1". > > +exit: > > + g_free(ct3d->media_op_sanitize); > > + ct3d->media_op_sanitize = NULL; > > + return; > > +} > > + > > +static int get_sanitize_duration(uint64_t total_mem) > > Where did this come from? Factor out the existing code > in cmd_santize_overwrite() instead of duplicating this stack > of if/else if > > Ideally do that as a precursor patch as it's just code movement. > > > > +{ > > + int secs = 0; > > + > > + if (total_mem <= 512) { > > + secs = 4; > > + } else if (total_mem <= 1024) { > > + secs = 8; > > + } else if (total_mem <= 2 * 1024) { > > + secs = 15; > > + } else if (total_mem <= 4 * 1024) { > > + secs = 30; > > + } else if (total_mem <= 8 * 1024) { > > + secs = 60; > > + } else if (total_mem <= 16 * 1024) { > > + secs = 2 * 60; > > + } else if (total_mem <= 32 * 1024) { > > + secs = 4 * 60; > > + } else if (total_mem <= 64 * 1024) { > > + secs = 8 * 60; > > + } else if (total_mem <= 128 * 1024) { > > + secs = 15 * 60; > > + } else if (total_mem <= 256 * 1024) { > > + secs = 30 * 60; > > + } else if (total_mem <= 512 * 1024) { > > + secs = 60 * 60; > > + } else if (total_mem <= 1024 * 1024) { > > + secs = 120 * 60; > > + } else { > > + secs = 240 * 60; /* max 4 hrs */ > > + } > > + > > + return secs; > > +} > > + > > enum { > > MEDIA_OP_GENERAL = 0x0, > > MEDIA_OP_SANITIZE = 0x1, > > @@ -1729,10 +1868,9 @@ enum { > > } MEDIA_OPERATION_CLASS; > > > > enum { > > - MEDIA_OP_SUB_DISCOVERY = 0x0, > > - MEDIA_OP_SUB_SANITIZE = 0x0, > > - MEDIA_OP_SUB_ZERO = 0x1, > > - MEDIA_OP_SUB_CLASS_MAX > > + MEDIA_OP_GEN_DISCOVERY = 0x0, > > + MEDIA_OP_SAN_SANITIZE = 0x0, > > + MEDIA_OP_SAN_ZERO = 0x1, > > See naming suggestions in first patch. Make sure to tidy up so you don't introduce > them there then change them here! > > > } MEDIA_OPERATION_SUB_CLASS; > > > > struct media_op_supported_list_entry { > > @@ -1777,9 +1915,14 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, > > }; > > } QEMU_PACKED *media_op_in_pl = (void *)payload_in; > > > > + > > Blank line here doesn't add anything. > > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); > > uint8_t media_op_cl = media_op_in_pl->media_operation_class; > > uint8_t media_op_subclass = media_op_in_pl->media_operation_subclass; > > uint32_t dpa_range_count = media_op_in_pl->dpa_range_count; > > + uint8_t fill_value = 0; > > + uint64_t total_mem = 0; > > + int secs = 0; > > > > if (len_in < sizeof(*media_op_in_pl)) { > > return CXL_MBOX_INVALID_PAYLOAD_LENGTH; > > @@ -1788,7 +1931,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, > > switch (media_op_cl) { > > case MEDIA_OP_GENERAL: > > switch (media_op_subclass) { > > - case MEDIA_OP_SUB_DISCOVERY: > > + case MEDIA_OP_GEN_DISCOVERY: > > int count = 0; > > struct media_op_discovery_out_pl *media_out_pl = > > (void *)payload_out; > > @@ -1806,7 +1949,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, > > return CXL_MBOX_INVALID_INPUT; > > } > > > > - media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER; > > + media_out_pl->dpa_range_granularity = DPA_RANGE_GRANULARITY; > > media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS; > > if (num_ops > 0) { > > for (int i = start_index; i < MAX_SUPPORTED_OPS; i++) { > > @@ -1824,22 +1967,73 @@ disc_out: > > media_out_pl->num_of_supported_operations = count; > > *len_out = sizeof(struct media_op_discovery_out_pl) + > > (sizeof(struct media_op_supported_list_entry) * count); > > - break; > > + goto exit; > return CXL_MBOX_SUCCESS; No purpose in having the exit label as nothing > to be done. > > > > default: > > return CXL_MBOX_UNSUPPORTED; > > } > > break; > > case MEDIA_OP_SANITIZE: > > + { > > From code here not obvious why scoping {} needed. > > > switch (media_op_subclass) { > > - > > + case MEDIA_OP_SAN_SANITIZE: > > + if (len_in < (sizeof(*media_op_in_pl) + > > + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { > > + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; > > + } > The check applies to all current cases. So move it outside this switch statement > to remove duplication. Here all you should do is set the fill_value; > > > + fill_value = 0xF; > > + goto sanitize_range; > > + case MEDIA_OP_SAN_ZERO: > > + if (len_in < (sizeof(*media_op_in_pl) + > > + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { > > + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; > > + } > > + fill_value = 0; > > + goto sanitize_range; > Why goto. Just break... > > default: > > return CXL_MBOX_UNSUPPORTED; > > } > and have the stuff below under the sanitize_range label here. > > > break; > > + } > > default: > > return CXL_MBOX_UNSUPPORTED; > > } > > > > +sanitize_range: > > + if (dpa_range_count > 0) { > > + for (int i = 0; i < dpa_range_count; i++) { > > + if (validate_dpa_addr(ct3d, > > + media_op_in_pl->dpa_range_list[i].starting_dpa, > > + media_op_in_pl->dpa_range_list[i].length)) { > > + return CXL_MBOX_INVALID_INPUT; > > + } > > + total_mem += media_op_in_pl->dpa_range_list[i].length; > > + } > > + ct3d->media_op_sanitize = g_malloc0(sizeof(struct CXLSanitizeInfo) + > > + (dpa_range_count * > > + sizeof(struct dpa_range_list_entry))); > > + > > + if (ct3d->media_op_sanitize) { > > + ct3d->media_op_sanitize->dpa_range_count = dpa_range_count; > > + ct3d->media_op_sanitize->fill_value = fill_value; > > + memcpy(ct3d->media_op_sanitize->dpa_range_list, > > + media_op_in_pl->dpa_range_list, > > + (dpa_range_count * > > + sizeof(struct dpa_range_list_entry))); > > + secs = get_sanitize_duration(total_mem >> 20); > > + goto start_bg; > > + } > > + } else if (dpa_range_count == 0) { > > + goto exit; > if (dpa_range_count == 0) { > return CXL_MBOX_SUCCESS; > } > for (i = 0; i < dpa_range_count; i++) > > //that is return early in the simple case the no need for else > and the extra level of indent on these long lines. > > > > + } > > + > > +start_bg: > > + /* EBUSY other bg cmds as of now */ > > + cci->bg.runtime = secs * 1000UL; > > + *len_out = 0; > > + /* sanitize when done */ > > + cxl_dev_disable_media(&ct3d->cxl_dstate); > Why? This is santizing part of the device. As I undestand it the > main aim is to offload cleanup when the device is in use. Definitely > don't want to disable media. If I'm missing a reason please give > a spec reference. Table 8-164, sanitize description mentions to follow method in 8.2.10.9.5.1, which does call out placing device in disabled state, but I'm not sure if method refers to all text in 8.2.10.9.5.1 or the method devices uses to sanitize internally. I would imagine since sanitize is destructive we would not want to return any data from device ranges impacted by sanitize. I believe a simple way to achieve this is to disable entire device. > > > + return CXL_MBOX_BG_STARTED; > > +exit: > > return CXL_MBOX_SUCCESS; > > } > > > > @@ -3154,6 +3348,13 @@ static void bg_timercb(void *opaque) > > cxl_dev_enable_media(&ct3d->cxl_dstate); > > } > > break; > > + case 0x4402: /* Media Operations sanitize */ > > + { > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); > > + __do_sanitize(ct3d); > > + cxl_dev_enable_media(&ct3d->cxl_dstate); > Ah. You turn it back on here. Specification reference needed. > > + } > > + break; > > case 0x4304: /* scan media */ > > { > > CXLType3Dev *ct3d = CXL_TYPE3(cci->d); > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > index a64739be25..6d82eb266a 100644 > > --- a/include/hw/cxl/cxl_device.h > > +++ b/include/hw/cxl/cxl_device.h > > @@ -581,6 +581,15 @@ typedef struct CXLSetFeatureInfo { > > size_t data_size; > > } CXLSetFeatureInfo; > > > > +struct CXLSanitizeInfo { > > + uint32_t dpa_range_count; > > + uint8_t fill_value; > > + struct { > > + uint64_t starting_dpa; > > + uint64_t length; > > + } dpa_range_list[0]; > [] > > +}; > > + > > struct CXLType3Dev { > > /* Private */ > > PCIDevice parent_obj; > > @@ -651,6 +660,8 @@ struct CXLType3Dev { > > uint8_t num_regions; /* 0-8 regions */ > > CXLDCRegion regions[DCD_MAX_NUM_REGION]; > > } dc; > > + > > + struct CXLSanitizeInfo *media_op_sanitize; > Here we just need to know there is a definition of struct > CXLSantizeInfo not what form it takes. So use a forwards > defintion and push the definition of struct CXLSanitizeInfo > down to where it is needed only (in the c file). > > Thanks, > > Jonathan > > > }; > > > > #define TYPE_CXL_TYPE3 "cxl-type3" >
> > > > > + int dpa_range_count = san_info->dpa_range_count; > > > + int rc = 0; > > > + > > > + for (int i = 0; i < dpa_range_count; i++) { > > > + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, > > > + san_info->dpa_range_list[i].length, san_info->fill_value); > > > + if (rc) { > > > + goto exit; > > > + } > > > + } > > > + cxl_discard_all_event_records(&ct3d->cxl_dstate); > > > > Add a comment on why we are deleting event records when sanitizing a small > > part of memory? > > > > See response below for disabling the media state. Same section referenced > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome > depends on how we interpret "follow the method described in 8.2.10.9.5.1". > This also sounds like reading too much into that comment. > > > + } > > > + > > > +start_bg: > > > + /* EBUSY other bg cmds as of now */ > > > + cci->bg.runtime = secs * 1000UL; > > > + *len_out = 0; > > > + /* sanitize when done */ > > > + cxl_dev_disable_media(&ct3d->cxl_dstate); > > Why? This is santizing part of the device. As I undestand it the > > main aim is to offload cleanup when the device is in use. Definitely > > don't want to disable media. If I'm missing a reason please give > > a spec reference. > > Table 8-164, sanitize description mentions to follow method > in 8.2.10.9.5.1, which does call out placing device in disabled > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1 > or the method devices uses to sanitize internally. I think it is meant to just be the method of sanitizing. > > I would imagine since sanitize is destructive we would not want to return > any data from device ranges impacted by sanitize. I believe a simple > way to achieve this is to disable entire device. Hmm. That rather destroys the main use case I'm aware of for this (unlike the general santize commands from earlier CXL versions)/ Superficially sounds like we need a spec clarification as clearly not super clear! > Jonathan
On Mon, Feb 03, 2025 at 11:33:54AM +0000, Jonathan Cameron wrote: > > > > > > > > + int dpa_range_count = san_info->dpa_range_count; > > > > + int rc = 0; > > > > + > > > > + for (int i = 0; i < dpa_range_count; i++) { > > > > + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, > > > > + san_info->dpa_range_list[i].length, san_info->fill_value); > > > > + if (rc) { > > > > + goto exit; > > > > + } > > > > + } > > > > + cxl_discard_all_event_records(&ct3d->cxl_dstate); > > > > > > Add a comment on why we are deleting event records when sanitizing a small > > > part of memory? > > > > > > > See response below for disabling the media state. Same section referenced > > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome > > depends on how we interpret "follow the method described in 8.2.10.9.5.1". > > > > This also sounds like reading too much into that comment. > Agreed, Vinayak let's drop the discard of all event records from this patch. > > > > + } > > > > + > > > > +start_bg: > > > > + /* EBUSY other bg cmds as of now */ > > > > + cci->bg.runtime = secs * 1000UL; > > > > + *len_out = 0; > > > > + /* sanitize when done */ > > > > + cxl_dev_disable_media(&ct3d->cxl_dstate); > > > Why? This is santizing part of the device. As I undestand it the > > > main aim is to offload cleanup when the device is in use. Definitely > > > don't want to disable media. If I'm missing a reason please give > > > a spec reference. > > > > Table 8-164, sanitize description mentions to follow method > > in 8.2.10.9.5.1, which does call out placing device in disabled > > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1 > > or the method devices uses to sanitize internally. > > I think it is meant to just be the method of sanitizing. > Ok, let's use this interpretation. Vinayak, can you remove this as well and then we put a comment in the patch that media op sanitize is targeted so no need to disable media or clear event logs. > > > > I would imagine since sanitize is destructive we would not want to return > > any data from device ranges impacted by sanitize. I believe a simple > > way to achieve this is to disable entire device. > > Hmm. That rather destroys the main use case I'm aware of for this > (unlike the general santize commands from earlier CXL versions)/ > Superficially sounds like we need a spec clarification as > clearly not super clear! > For this series, let's drive the work with the use case you have in mind. We will start a thread with the consortium, but I don't think that should delay this work. > > > > Jonathan > >
On 24/01/25 03:19PM, Jonathan Cameron wrote: >On Thu, 23 Jan 2025 10:39:03 +0530 >Vinayak Holikatti <vinayak.kh@samsung.com> wrote: > >> CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands. >> CXL devices supports media operations Sanitize and Write zero command. > >As before, don't indent this. > >> >> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com> >> --- >> hw/cxl/cxl-mailbox-utils.c | 217 ++++++++++++++++++++++++++++++++++-- >> include/hw/cxl/cxl_device.h | 11 ++ >> 2 files changed, 220 insertions(+), 8 deletions(-) >> >> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c >> index 2315d07fb1..89847ddd9d 100644 >> --- a/hw/cxl/cxl-mailbox-utils.c >> +++ b/hw/cxl/cxl-mailbox-utils.c >> @@ -1722,6 +1722,145 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd, >> return CXL_MBOX_BG_STARTED; >> } >> >> +#define DPA_RANGE_GRANULARITY 64 > >Could use existing CXL_CACHELINE_SIZE definition for this, though I guess >strictly speaking it could be unrelated. > >> +struct dpa_range_list_entry { >> + uint64_t starting_dpa; >> + uint64_t length; >> +}; >> + >> +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr, >> + size_t length) >> +{ >> + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; >> + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; >> + int rc = 0; >> + >> + if ((dpa_addr % DPA_RANGE_GRANULARITY) || >> + (length % DPA_RANGE_GRANULARITY)) { >Probably makes sense to also check for length 0 here as that would >be very odd if sent. >> + return -EINVAL; >> + } >> + >> + if (ct3d->hostvmem) { >> + vmr = host_memory_backend_get_memory(ct3d->hostvmem); >> + vmr_size = memory_region_size(vmr); >> + } >> + if (ct3d->hostpmem) { >> + pmr = host_memory_backend_get_memory(ct3d->hostpmem); >> + pmr_size = memory_region_size(pmr); >> + } >> + if (ct3d->dc.host_dc) { >> + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); >> + dc_size = memory_region_size(dc_mr); >> + } >> + >> + if (!vmr && !pmr && !dc_mr) { >> + return -ENODEV; >> + } >> + >> + if (dpa_addr >= vmr_size + pmr_size + dc_size || >> + (dpa_addr + length) > vmr_size + pmr_size + dc_size) { > >If length is checked as non zero above, only the second test is needed. > >> + return -EINVAL; >> + } >> + >> + if (dpa_addr > vmr_size + pmr_size) { >> + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { >> + return -ENODEV; >> + } >> + } >> + >> + >> + return rc; > >return 0. rc is never set to anything else. > >> +} >> + >> +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length, >> + uint8_t fill_value) >> +{ >> + >> + MemoryRegion *vmr = NULL, *pmr = NULL; >> + uint64_t vmr_size = 0, pmr_size = 0; >> + AddressSpace *as = NULL; >> + MemTxAttrs mem_attrs = {0}; >> + >> + if (ct3d->hostvmem) { >> + vmr = host_memory_backend_get_memory(ct3d->hostvmem); >> + vmr_size = memory_region_size(vmr); >> + } >> + if (ct3d->hostpmem) { >> + pmr = host_memory_backend_get_memory(ct3d->hostpmem); >> + pmr_size = memory_region_size(pmr); >> + } >> + >> + if (dpa_addr < vmr_size) { >> + as = &ct3d->hostvmem_as; >> + } else if (dpa_addr < vmr_size + pmr_size) { >> + as = &ct3d->hostpmem_as; >> + } else { >> + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { >> + return -ENODEV; >> + } >> + as = &ct3d->dc.host_dc_as; >> + } > >You could factor out everything down to here and then use that >for the validate_dpa_addr() as finding an address space means >we also checked the address is valid. Otherwise it does not match. Didnt get what you meant, validate_dpa_addr is meant for checking valid dpa address and sanitize_range is get the address space handle to do actual sanitize of dpa address, so two are different purposes, > >> + >> + return address_space_set(as, dpa_addr, > >odd spacing after return. Should just be one space. > >> + fill_value, length, mem_attrs); >> +} >> + >> +/* Perform the actual device zeroing */ >> +static void __do_sanitize(CXLType3Dev *ct3d) >> +{ >> + struct CXLSanitizeInfo *san_info = ct3d->media_op_sanitize; > >Single space only before *san_info > >> + int dpa_range_count = san_info->dpa_range_count; >> + int rc = 0; >> + >> + for (int i = 0; i < dpa_range_count; i++) { >> + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, >> + san_info->dpa_range_list[i].length, san_info->fill_value); >> + if (rc) { >> + goto exit; >> + } >> + } >> + cxl_discard_all_event_records(&ct3d->cxl_dstate); > >Add a comment on why we are deleting event records when sanitizing a small >part of memory? > >> +exit: >> + g_free(ct3d->media_op_sanitize); >> + ct3d->media_op_sanitize = NULL; >> + return; >> +} >> + >> +static int get_sanitize_duration(uint64_t total_mem) > >Where did this come from? Factor out the existing code >in cmd_santize_overwrite() instead of duplicating this stack >of if/else if > >Ideally do that as a precursor patch as it's just code movement. > > >> +{ >> + int secs = 0; >> + >> + if (total_mem <= 512) { >> + secs = 4; >> + } else if (total_mem <= 1024) { >> + secs = 8; >> + } else if (total_mem <= 2 * 1024) { >> + secs = 15; >> + } else if (total_mem <= 4 * 1024) { >> + secs = 30; >> + } else if (total_mem <= 8 * 1024) { >> + secs = 60; >> + } else if (total_mem <= 16 * 1024) { >> + secs = 2 * 60; >> + } else if (total_mem <= 32 * 1024) { >> + secs = 4 * 60; >> + } else if (total_mem <= 64 * 1024) { >> + secs = 8 * 60; >> + } else if (total_mem <= 128 * 1024) { >> + secs = 15 * 60; >> + } else if (total_mem <= 256 * 1024) { >> + secs = 30 * 60; >> + } else if (total_mem <= 512 * 1024) { >> + secs = 60 * 60; >> + } else if (total_mem <= 1024 * 1024) { >> + secs = 120 * 60; >> + } else { >> + secs = 240 * 60; /* max 4 hrs */ >> + } >> + >> + return secs; >> +} >> + >> enum { >> MEDIA_OP_GENERAL = 0x0, >> MEDIA_OP_SANITIZE = 0x1, >> @@ -1729,10 +1868,9 @@ enum { >> } MEDIA_OPERATION_CLASS; >> >> enum { >> - MEDIA_OP_SUB_DISCOVERY = 0x0, >> - MEDIA_OP_SUB_SANITIZE = 0x0, >> - MEDIA_OP_SUB_ZERO = 0x1, >> - MEDIA_OP_SUB_CLASS_MAX >> + MEDIA_OP_GEN_DISCOVERY = 0x0, >> + MEDIA_OP_SAN_SANITIZE = 0x0, >> + MEDIA_OP_SAN_ZERO = 0x1, > >See naming suggestions in first patch. Make sure to tidy up so you don't introduce >them there then change them here! > >> } MEDIA_OPERATION_SUB_CLASS; >> >> struct media_op_supported_list_entry { >> @@ -1777,9 +1915,14 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, >> }; >> } QEMU_PACKED *media_op_in_pl = (void *)payload_in; >> >> + > >Blank line here doesn't add anything. > >> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); >> uint8_t media_op_cl = media_op_in_pl->media_operation_class; >> uint8_t media_op_subclass = media_op_in_pl->media_operation_subclass; >> uint32_t dpa_range_count = media_op_in_pl->dpa_range_count; >> + uint8_t fill_value = 0; >> + uint64_t total_mem = 0; >> + int secs = 0; >> >> if (len_in < sizeof(*media_op_in_pl)) { >> return CXL_MBOX_INVALID_PAYLOAD_LENGTH; >> @@ -1788,7 +1931,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, >> switch (media_op_cl) { >> case MEDIA_OP_GENERAL: >> switch (media_op_subclass) { >> - case MEDIA_OP_SUB_DISCOVERY: >> + case MEDIA_OP_GEN_DISCOVERY: >> int count = 0; >> struct media_op_discovery_out_pl *media_out_pl = >> (void *)payload_out; >> @@ -1806,7 +1949,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, >> return CXL_MBOX_INVALID_INPUT; >> } >> >> - media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER; >> + media_out_pl->dpa_range_granularity = DPA_RANGE_GRANULARITY; >> media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS; >> if (num_ops > 0) { >> for (int i = start_index; i < MAX_SUPPORTED_OPS; i++) { >> @@ -1824,22 +1967,73 @@ disc_out: >> media_out_pl->num_of_supported_operations = count; >> *len_out = sizeof(struct media_op_discovery_out_pl) + >> (sizeof(struct media_op_supported_list_entry) * count); >> - break; >> + goto exit; >return CXL_MBOX_SUCCESS; No purpose in having the exit label as nothing >to be done. > > >> default: >> return CXL_MBOX_UNSUPPORTED; >> } >> break; >> case MEDIA_OP_SANITIZE: >> + { > >From code here not obvious why scoping {} needed. > >> switch (media_op_subclass) { >> - >> + case MEDIA_OP_SAN_SANITIZE: >> + if (len_in < (sizeof(*media_op_in_pl) + >> + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { >> + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; >> + } >The check applies to all current cases. So move it outside this switch statement >to remove duplication. Here all you should do is set the fill_value; > >> + fill_value = 0xF; >> + goto sanitize_range; >> + case MEDIA_OP_SAN_ZERO: >> + if (len_in < (sizeof(*media_op_in_pl) + >> + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { >> + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; >> + } >> + fill_value = 0; >> + goto sanitize_range; >Why goto. Just break... >> default: >> return CXL_MBOX_UNSUPPORTED; >> } >and have the stuff below under the sanitize_range label here. > >> break; >> + } >> default: >> return CXL_MBOX_UNSUPPORTED; >> } >> >> +sanitize_range: >> + if (dpa_range_count > 0) { >> + for (int i = 0; i < dpa_range_count; i++) { >> + if (validate_dpa_addr(ct3d, >> + media_op_in_pl->dpa_range_list[i].starting_dpa, >> + media_op_in_pl->dpa_range_list[i].length)) { >> + return CXL_MBOX_INVALID_INPUT; >> + } >> + total_mem += media_op_in_pl->dpa_range_list[i].length; >> + } >> + ct3d->media_op_sanitize = g_malloc0(sizeof(struct CXLSanitizeInfo) + >> + (dpa_range_count * >> + sizeof(struct dpa_range_list_entry))); >> + >> + if (ct3d->media_op_sanitize) { >> + ct3d->media_op_sanitize->dpa_range_count = dpa_range_count; >> + ct3d->media_op_sanitize->fill_value = fill_value; >> + memcpy(ct3d->media_op_sanitize->dpa_range_list, >> + media_op_in_pl->dpa_range_list, >> + (dpa_range_count * >> + sizeof(struct dpa_range_list_entry))); >> + secs = get_sanitize_duration(total_mem >> 20); >> + goto start_bg; >> + } >> + } else if (dpa_range_count == 0) { >> + goto exit; > if (dpa_range_count == 0) { > return CXL_MBOX_SUCCESS; > } > for (i = 0; i < dpa_range_count; i++) > >//that is return early in the simple case the no need for else >and the extra level of indent on these long lines. > > >> + } >> + >> +start_bg: >> + /* EBUSY other bg cmds as of now */ >> + cci->bg.runtime = secs * 1000UL; >> + *len_out = 0; >> + /* sanitize when done */ >> + cxl_dev_disable_media(&ct3d->cxl_dstate); >Why? This is santizing part of the device. As I undestand it the >main aim is to offload cleanup when the device is in use. Definitely >don't want to disable media. If I'm missing a reason please give >a spec reference. > >> + return CXL_MBOX_BG_STARTED; >> +exit: >> return CXL_MBOX_SUCCESS; >> } >> >> @@ -3154,6 +3348,13 @@ static void bg_timercb(void *opaque) >> cxl_dev_enable_media(&ct3d->cxl_dstate); >> } >> break; >> + case 0x4402: /* Media Operations sanitize */ >> + { >> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); >> + __do_sanitize(ct3d); >> + cxl_dev_enable_media(&ct3d->cxl_dstate); >Ah. You turn it back on here. Specification reference needed. >> + } >> + break; >> case 0x4304: /* scan media */ >> { >> CXLType3Dev *ct3d = CXL_TYPE3(cci->d); >> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h >> index a64739be25..6d82eb266a 100644 >> --- a/include/hw/cxl/cxl_device.h >> +++ b/include/hw/cxl/cxl_device.h >> @@ -581,6 +581,15 @@ typedef struct CXLSetFeatureInfo { >> size_t data_size; >> } CXLSetFeatureInfo; >> >> +struct CXLSanitizeInfo { >> + uint32_t dpa_range_count; >> + uint8_t fill_value; >> + struct { >> + uint64_t starting_dpa; >> + uint64_t length; >> + } dpa_range_list[0]; >[] >> +}; >> + >> struct CXLType3Dev { >> /* Private */ >> PCIDevice parent_obj; >> @@ -651,6 +660,8 @@ struct CXLType3Dev { >> uint8_t num_regions; /* 0-8 regions */ >> CXLDCRegion regions[DCD_MAX_NUM_REGION]; >> } dc; >> + >> + struct CXLSanitizeInfo *media_op_sanitize; >Here we just need to know there is a definition of struct >CXLSantizeInfo not what form it takes. So use a forwards >defintion and push the definition of struct CXLSanitizeInfo >down to where it is needed only (in the c file). > >Thanks, > >Jonathan > >> }; >> >> #define TYPE_CXL_TYPE3 "cxl-type3" >
On 03/02/25 05:02PM, Adam Manzanares wrote: >On Mon, Feb 03, 2025 at 11:33:54AM +0000, Jonathan Cameron wrote: >> >> > > >> > > > + int dpa_range_count = san_info->dpa_range_count; >> > > > + int rc = 0; >> > > > + >> > > > + for (int i = 0; i < dpa_range_count; i++) { >> > > > + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, >> > > > + san_info->dpa_range_list[i].length, san_info->fill_value); >> > > > + if (rc) { >> > > > + goto exit; >> > > > + } >> > > > + } >> > > > + cxl_discard_all_event_records(&ct3d->cxl_dstate); >> > > >> > > Add a comment on why we are deleting event records when sanitizing a small >> > > part of memory? >> > > >> > >> > See response below for disabling the media state. Same section referenced >> > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome >> > depends on how we interpret "follow the method described in 8.2.10.9.5.1". >> > >> >> This also sounds like reading too much into that comment. >> > >Agreed, Vinayak let's drop the discard of all event records >from this patch. Ok Adam will drop the discard of all event records > >> > > > + } >> > > > + >> > > > +start_bg: >> > > > + /* EBUSY other bg cmds as of now */ >> > > > + cci->bg.runtime = secs * 1000UL; >> > > > + *len_out = 0; >> > > > + /* sanitize when done */ >> > > > + cxl_dev_disable_media(&ct3d->cxl_dstate); >> > > Why? This is santizing part of the device. As I undestand it the >> > > main aim is to offload cleanup when the device is in use. Definitely >> > > don't want to disable media. If I'm missing a reason please give >> > > a spec reference. >> > >> > Table 8-164, sanitize description mentions to follow method >> > in 8.2.10.9.5.1, which does call out placing device in disabled >> > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1 >> > or the method devices uses to sanitize internally. >> >> I think it is meant to just be the method of sanitizing. >> > >Ok, let's use this interpretation. Vinayak, can you remove this as well >and then we put a comment in the patch that media op sanitize is targeted >so no need to disable media or clear event logs. ok Adam, will remove this as well and comment as needed > > >> > >> > I would imagine since sanitize is destructive we would not want to return >> > any data from device ranges impacted by sanitize. I believe a simple >> > way to achieve this is to disable entire device. >> >> Hmm. That rather destroys the main use case I'm aware of for this >> (unlike the general santize commands from earlier CXL versions)/ >> Superficially sounds like we need a spec clarification as >> clearly not super clear! >> > >For this series, let's drive the work with the use case you have in >mind. We will start a thread with the consortium, but I don't think >that should delay this work. > >> > >> >> Jonathan >> >> Vinayak Holikatti
> >> +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length, > >> + uint8_t fill_value) > >> +{ > >> + > >> + MemoryRegion *vmr = NULL, *pmr = NULL; > >> + uint64_t vmr_size = 0, pmr_size = 0; > >> + AddressSpace *as = NULL; > >> + MemTxAttrs mem_attrs = {0}; > >> + > >> + if (ct3d->hostvmem) { > >> + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > >> + vmr_size = memory_region_size(vmr); > >> + } > >> + if (ct3d->hostpmem) { > >> + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > >> + pmr_size = memory_region_size(pmr); > >> + } > >> + > >> + if (dpa_addr < vmr_size) { > >> + as = &ct3d->hostvmem_as; > >> + } else if (dpa_addr < vmr_size + pmr_size) { > >> + as = &ct3d->hostpmem_as; > >> + } else { > >> + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { > >> + return -ENODEV; > >> + } > >> + as = &ct3d->dc.host_dc_as; > >> + } > > > >You could factor out everything down to here and then use that > >for the validate_dpa_addr() as finding an address space means > >we also checked the address is valid. Otherwise it does not match. > > Didnt get what you meant, validate_dpa_addr is meant for > checking valid dpa address and sanitize_range is > get the address space handle to do actual sanitize > of dpa address, so two are different purposes, Different purposes but the actual checks mostly overlap. If we can find the address space then there are only a few extra checks on alignment with granual size etc needed. I'd just like to avoid that duplication so factor out the shared code to a helper called in both functions. > > > > >> + > >> + return address_space_set(as, dpa_addr,
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 2315d07fb1..89847ddd9d 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -1722,6 +1722,145 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd, return CXL_MBOX_BG_STARTED; } +#define DPA_RANGE_GRANULARITY 64 +struct dpa_range_list_entry { + uint64_t starting_dpa; + uint64_t length; +}; + +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr, + size_t length) +{ + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL; + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0; + int rc = 0; + + if ((dpa_addr % DPA_RANGE_GRANULARITY) || + (length % DPA_RANGE_GRANULARITY)) { + return -EINVAL; + } + + if (ct3d->hostvmem) { + vmr = host_memory_backend_get_memory(ct3d->hostvmem); + vmr_size = memory_region_size(vmr); + } + if (ct3d->hostpmem) { + pmr = host_memory_backend_get_memory(ct3d->hostpmem); + pmr_size = memory_region_size(pmr); + } + if (ct3d->dc.host_dc) { + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc); + dc_size = memory_region_size(dc_mr); + } + + if (!vmr && !pmr && !dc_mr) { + return -ENODEV; + } + + if (dpa_addr >= vmr_size + pmr_size + dc_size || + (dpa_addr + length) > vmr_size + pmr_size + dc_size) { + return -EINVAL; + } + + if (dpa_addr > vmr_size + pmr_size) { + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { + return -ENODEV; + } + } + + + return rc; +} + +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length, + uint8_t fill_value) +{ + + MemoryRegion *vmr = NULL, *pmr = NULL; + uint64_t vmr_size = 0, pmr_size = 0; + AddressSpace *as = NULL; + MemTxAttrs mem_attrs = {0}; + + if (ct3d->hostvmem) { + vmr = host_memory_backend_get_memory(ct3d->hostvmem); + vmr_size = memory_region_size(vmr); + } + if (ct3d->hostpmem) { + pmr = host_memory_backend_get_memory(ct3d->hostpmem); + pmr_size = memory_region_size(pmr); + } + + if (dpa_addr < vmr_size) { + as = &ct3d->hostvmem_as; + } else if (dpa_addr < vmr_size + pmr_size) { + as = &ct3d->hostpmem_as; + } else { + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) { + return -ENODEV; + } + as = &ct3d->dc.host_dc_as; + } + + return address_space_set(as, dpa_addr, + fill_value, length, mem_attrs); +} + +/* Perform the actual device zeroing */ +static void __do_sanitize(CXLType3Dev *ct3d) +{ + struct CXLSanitizeInfo *san_info = ct3d->media_op_sanitize; + int dpa_range_count = san_info->dpa_range_count; + int rc = 0; + + for (int i = 0; i < dpa_range_count; i++) { + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa, + san_info->dpa_range_list[i].length, san_info->fill_value); + if (rc) { + goto exit; + } + } + cxl_discard_all_event_records(&ct3d->cxl_dstate); +exit: + g_free(ct3d->media_op_sanitize); + ct3d->media_op_sanitize = NULL; + return; +} + +static int get_sanitize_duration(uint64_t total_mem) +{ + int secs = 0; + + if (total_mem <= 512) { + secs = 4; + } else if (total_mem <= 1024) { + secs = 8; + } else if (total_mem <= 2 * 1024) { + secs = 15; + } else if (total_mem <= 4 * 1024) { + secs = 30; + } else if (total_mem <= 8 * 1024) { + secs = 60; + } else if (total_mem <= 16 * 1024) { + secs = 2 * 60; + } else if (total_mem <= 32 * 1024) { + secs = 4 * 60; + } else if (total_mem <= 64 * 1024) { + secs = 8 * 60; + } else if (total_mem <= 128 * 1024) { + secs = 15 * 60; + } else if (total_mem <= 256 * 1024) { + secs = 30 * 60; + } else if (total_mem <= 512 * 1024) { + secs = 60 * 60; + } else if (total_mem <= 1024 * 1024) { + secs = 120 * 60; + } else { + secs = 240 * 60; /* max 4 hrs */ + } + + return secs; +} + enum { MEDIA_OP_GENERAL = 0x0, MEDIA_OP_SANITIZE = 0x1, @@ -1729,10 +1868,9 @@ enum { } MEDIA_OPERATION_CLASS; enum { - MEDIA_OP_SUB_DISCOVERY = 0x0, - MEDIA_OP_SUB_SANITIZE = 0x0, - MEDIA_OP_SUB_ZERO = 0x1, - MEDIA_OP_SUB_CLASS_MAX + MEDIA_OP_GEN_DISCOVERY = 0x0, + MEDIA_OP_SAN_SANITIZE = 0x0, + MEDIA_OP_SAN_ZERO = 0x1, } MEDIA_OPERATION_SUB_CLASS; struct media_op_supported_list_entry { @@ -1777,9 +1915,14 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, }; } QEMU_PACKED *media_op_in_pl = (void *)payload_in; + + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); uint8_t media_op_cl = media_op_in_pl->media_operation_class; uint8_t media_op_subclass = media_op_in_pl->media_operation_subclass; uint32_t dpa_range_count = media_op_in_pl->dpa_range_count; + uint8_t fill_value = 0; + uint64_t total_mem = 0; + int secs = 0; if (len_in < sizeof(*media_op_in_pl)) { return CXL_MBOX_INVALID_PAYLOAD_LENGTH; @@ -1788,7 +1931,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, switch (media_op_cl) { case MEDIA_OP_GENERAL: switch (media_op_subclass) { - case MEDIA_OP_SUB_DISCOVERY: + case MEDIA_OP_GEN_DISCOVERY: int count = 0; struct media_op_discovery_out_pl *media_out_pl = (void *)payload_out; @@ -1806,7 +1949,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd, return CXL_MBOX_INVALID_INPUT; } - media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER; + media_out_pl->dpa_range_granularity = DPA_RANGE_GRANULARITY; media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS; if (num_ops > 0) { for (int i = start_index; i < MAX_SUPPORTED_OPS; i++) { @@ -1824,22 +1967,73 @@ disc_out: media_out_pl->num_of_supported_operations = count; *len_out = sizeof(struct media_op_discovery_out_pl) + (sizeof(struct media_op_supported_list_entry) * count); - break; + goto exit; default: return CXL_MBOX_UNSUPPORTED; } break; case MEDIA_OP_SANITIZE: + { switch (media_op_subclass) { - + case MEDIA_OP_SAN_SANITIZE: + if (len_in < (sizeof(*media_op_in_pl) + + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; + } + fill_value = 0xF; + goto sanitize_range; + case MEDIA_OP_SAN_ZERO: + if (len_in < (sizeof(*media_op_in_pl) + + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) { + return CXL_MBOX_INVALID_PAYLOAD_LENGTH; + } + fill_value = 0; + goto sanitize_range; default: return CXL_MBOX_UNSUPPORTED; } break; + } default: return CXL_MBOX_UNSUPPORTED; } +sanitize_range: + if (dpa_range_count > 0) { + for (int i = 0; i < dpa_range_count; i++) { + if (validate_dpa_addr(ct3d, + media_op_in_pl->dpa_range_list[i].starting_dpa, + media_op_in_pl->dpa_range_list[i].length)) { + return CXL_MBOX_INVALID_INPUT; + } + total_mem += media_op_in_pl->dpa_range_list[i].length; + } + ct3d->media_op_sanitize = g_malloc0(sizeof(struct CXLSanitizeInfo) + + (dpa_range_count * + sizeof(struct dpa_range_list_entry))); + + if (ct3d->media_op_sanitize) { + ct3d->media_op_sanitize->dpa_range_count = dpa_range_count; + ct3d->media_op_sanitize->fill_value = fill_value; + memcpy(ct3d->media_op_sanitize->dpa_range_list, + media_op_in_pl->dpa_range_list, + (dpa_range_count * + sizeof(struct dpa_range_list_entry))); + secs = get_sanitize_duration(total_mem >> 20); + goto start_bg; + } + } else if (dpa_range_count == 0) { + goto exit; + } + +start_bg: + /* EBUSY other bg cmds as of now */ + cci->bg.runtime = secs * 1000UL; + *len_out = 0; + /* sanitize when done */ + cxl_dev_disable_media(&ct3d->cxl_dstate); + return CXL_MBOX_BG_STARTED; +exit: return CXL_MBOX_SUCCESS; } @@ -3154,6 +3348,13 @@ static void bg_timercb(void *opaque) cxl_dev_enable_media(&ct3d->cxl_dstate); } break; + case 0x4402: /* Media Operations sanitize */ + { + CXLType3Dev *ct3d = CXL_TYPE3(cci->d); + __do_sanitize(ct3d); + cxl_dev_enable_media(&ct3d->cxl_dstate); + } + break; case 0x4304: /* scan media */ { CXLType3Dev *ct3d = CXL_TYPE3(cci->d); diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h index a64739be25..6d82eb266a 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -581,6 +581,15 @@ typedef struct CXLSetFeatureInfo { size_t data_size; } CXLSetFeatureInfo; +struct CXLSanitizeInfo { + uint32_t dpa_range_count; + uint8_t fill_value; + struct { + uint64_t starting_dpa; + uint64_t length; + } dpa_range_list[0]; +}; + struct CXLType3Dev { /* Private */ PCIDevice parent_obj; @@ -651,6 +660,8 @@ struct CXLType3Dev { uint8_t num_regions; /* 0-8 regions */ CXLDCRegion regions[DCD_MAX_NUM_REGION]; } dc; + + struct CXLSanitizeInfo *media_op_sanitize; }; #define TYPE_CXL_TYPE3 "cxl-type3"
CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands. CXL devices supports media operations Sanitize and Write zero command. Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com> --- hw/cxl/cxl-mailbox-utils.c | 217 ++++++++++++++++++++++++++++++++++-- include/hw/cxl/cxl_device.h | 11 ++ 2 files changed, 220 insertions(+), 8 deletions(-)