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...@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...@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"
>
