Re: [PATCH 2/2] hw/cxl/cxl-mailbox-utils: Add support for Media operations Sanitize and Write Zeros commands (8.2.9.9.5.3)

[Vinayak Holikatti <vinayak.kh@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 13986 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]