Re: [PATCH v2 3/3] 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: 12444 bytes --]

On 14/02/25 02:40PM, Jonathan Cameron wrote:
>On Thu, 13 Feb 2025 14:45:58 +0530
>Vinayak Holikatti <vinayak.kh@samsung.com> wrote:
>
>> CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands.
>As in previous - please update to the r3.2 spec.
>
ok will update as per 3.2
>A few comments inline.
>
>Thanks,
>
>Jonathan
>
Thank you for feedback will address them in V3 patch

>> 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 |   4 +
>>  2 files changed, 216 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index d58c20842f..2d8d1171b4 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -1732,6 +1732,130 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>>  }
>>
>>  #define CXL_CACHELINE_SIZE 64
>> +struct CXLSanitizeInfo {
>> +    uint32_t dpa_range_count;
>> +    uint8_t fill_value;
>> +    struct {
>> +            uint64_t starting_dpa;
>> +            uint64_t length;
>> +    } dpa_range_list[];
>> +};
>> +
>> +struct dpa_range_list_entry {
>> +    uint64_t starting_dpa;
>> +    uint64_t length;
>> +};
>
>Declare it above and use in CXLSanitizeInfo
>
ok
>> +
>> +static uint64_t get_vmr_size(CXLType3Dev *ct3d, MemoryRegion **vmr)
>> +{
>> +    uint64_t vmr_size = 0;
>> +    if (ct3d->hostvmem) {
>> +        *vmr = host_memory_backend_get_memory(ct3d->hostvmem);
>> +        vmr_size = memory_region_size(*vmr);
>> +    }
>> +    return vmr_size;
>> +}
>> +
>
>I would write as
>
>static uint64_t get_pmr_size(CXLTYpe3Dev *ct3d, MemoryRegion **pmr)
>{
>    MemoryRegion *mr;
>    if (ct3d->hostpmem) {
>        mr = host_memory_region_backend_get_memory(ct3d->hostpmem);
>        if (pmr) {
>            *pmr = mr;
>        }
>	return memory_region_size(mr);
>    }
>    return 0;
>}
>
ok
>Making the pmr argument optional for when you don't need it.
>
>> +static uint64_t get_pmr_size(CXLType3Dev *ct3d, MemoryRegion **pmr)
>> +{
>> +    uint64_t pmr_size = 0;
>> +    if (ct3d->hostpmem) {
>> +        *pmr = host_memory_backend_get_memory(ct3d->hostpmem);
>> +        pmr_size = memory_region_size(*pmr);
>> +    }
>> +    return pmr_size;
>> +}
>> +
>> +static uint64_t get_dc_size(CXLType3Dev *ct3d, MemoryRegion **dc_mr)
>> +{
>> +    uint64_t dc_size = 0;
>> +    if (ct3d->dc.host_dc) {
>> +        *dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
>> +        dc_size = memory_region_size(*dc_mr);
>> +    }
>> +    return dc_size;
>> +}
>> +
>> +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;
>
>overwritten in all paths were we use them. So don't assign initial values.
>
ok
>> +
>> +    if ((dpa_addr % CXL_CACHELINE_SIZE) ||
>> +         (length % CXL_CACHELINE_SIZE)  ||
>> +         (length <= 0)) {
>Align as
>    if ((dpa_addr % CXL_CACHELINE_SIZE) ||
>        (length % CXL_CACHELINE_SIZE) ||
>        (length <= 0)) {
>
ok
>> +        return -EINVAL;
>> +    }
>> +
>> +    vmr_size = get_vmr_size(ct3d, &vmr);
>> +    pmr_size = get_pmr_size(ct3d, &pmr);
>> +    dc_size = get_dc_size(ct3d, &dc_mr);
>> +
>> +    if (!vmr && !pmr && !dc_mr) {
>
>That's a bit late given you used them to get the sizes.
>Do this before filling sizes.
>
ok
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ((dpa_addr + length) > vmr_size + pmr_size + dc_size) {
>Skip inner brackets.
>
ok
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (dpa_addr > vmr_size + pmr_size) {
>> +        if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) {
>> +            return -ENODEV;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +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};
>> +
>> +    vmr_size = get_vmr_size(ct3d, &vmr);
>> +    pmr_size = get_pmr_size(ct3d, &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);
>
>Odd wrap.  Put as much as fits on line under 80 chars on first line
>then align next line to just after (
>
ok
>> +}
>> +
>> +/* 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++) {
>
>Declare i outside loop (match local style).
>
ok
>> +        rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa,
>> +                san_info->dpa_range_list[i].length, san_info->fill_value);
>
>Either align 4 spaces after start of line above, or immediately after (
>
>> +        if (rc) {
>> +            goto exit;
>> +        }
>> +    }
>> +exit:
>> +    g_free(ct3d->media_op_sanitize);
>> +    ct3d->media_op_sanitize = NULL;
>> +    return;
>> +}
>> +
>>  enum {
>>      MEDIA_OP_CLASS_GENERAL  = 0x0,
>>          #define MEDIA_OP_GEN_SUBC_DISCOVERY 0x0
>> @@ -1760,9 +1884,9 @@ static const struct media_op_supported_list_entry media_op_matrix[] = {
>>  };
>>
>>  static CXLRetCode media_operations_discovery(uint8_t *payload_in,
>> -                                                size_t len_in,
>> -                                                uint8_t *payload_out,
>> -                                                size_t *len_out)
>> +                                             size_t len_in,
>> +                                             uint8_t *payload_out,
>> +                                             size_t *len_out)
>
>Ah. Drag this to earlier patch.
>
ok
>>  {
>>      struct {
>>          uint8_t media_operation_class;
>> @@ -1811,6 +1935,66 @@ static CXLRetCode media_operations_discovery(uint8_t *payload_in,
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> +static CXLRetCode media_operations_sanitize(CXLType3Dev *ct3d,
>> +                                            uint8_t *payload_in,
>> +                                            size_t len_in,
>> +                                            uint8_t *payload_out,
>> +                                            size_t *len_out,
>> +                                            uint8_t fill_value,
>> +                                            CXLCCI *cci)
>> +{
>> +    struct media_operations_sanitize {
>> +        uint8_t media_operation_class;
>> +        uint8_t media_operation_subclass;
>> +        uint8_t rsvd[2];
>> +        uint32_t dpa_range_count;
>> +        struct {
>> +            uint64_t starting_dpa;
>> +            uint64_t length;
>> +        } dpa_range_list[];
>> +    } QEMU_PACKED *media_op_in_sanitize_pl = (void *)payload_in;
>> +    uint32_t dpa_range_count = media_op_in_sanitize_pl->dpa_range_count;
>> +    uint64_t total_mem = 0;
>> +    int secs = 0;
>> +
>> +    if (len_in < (sizeof(*media_op_in_sanitize_pl) +
>> +           (dpa_range_count * sizeof(struct dpa_range_list_entry)))) {
>> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>> +    }
>> +
>> +    for (int i = 0; i < dpa_range_count; i++) {
>
>Declare outside of there (to match local style)
>
ok
>> +        if (validate_dpa_addr(ct3d,
>> +            media_op_in_sanitize_pl->dpa_range_list[i].starting_dpa,
>
>Hmm. This is tricky to read because of alignment.  I'd have local
>start_dpa nad length variables.
>
ok
>    for (i = 0; i < dpa_range_count; i++) {
>        uint64_t start_dpa = media_op_in_sanitize_pl->dpa_range_list[i].starting_dpa;
>        uint64_t length = media_op_in_sanitize_pl->dpa_range_list[i].length;
>
>	if (validate_dpa_addr(ct3d, dpa_start, length)) {
>
>> +            media_op_in_sanitize_pl->dpa_range_list[i].length)) {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +        total_mem += media_op_in_sanitize_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_sanitize_pl->dpa_range_list,
>> +                  (dpa_range_count *
>> +                  sizeof(struct dpa_range_list_entry)));
>> +        secs = get_sanitize_duration(total_mem >> 20);
>> +    }
>> +
>> +    /* EBUSY other bg cmds as of now */
>> +    cci->bg.runtime = secs * 1000UL;
>> +    *len_out = 0;
>> +    /*
>> +     * media op sanitize is targeted so no need to disable media or
>> +     * clear event logs
>> +     */
>> +    return CXL_MBOX_BG_STARTED;
>> +
>
>No blank line here.
>
ok
>> +}
>> +
>>  static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>>                                           uint8_t *payload_in,
>>                                           size_t len_in,
>> @@ -1825,6 +2009,9 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>>          uint32_t dpa_range_count;
>>      } QEMU_PACKED *media_op_in_common_pl = (void *)payload_in;
>>
>> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>> +    uint8_t fill_value = 0;
>
>Maybe just put this value in directly into where it is used?
>
ok
>> +
>>      if (len_in < sizeof(*media_op_in_common_pl)) {
>>          return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>>      }
>> @@ -1851,15 +2038,29 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>>          return media_operations_discovery(payload_in, len_in, payload_out,
>>                                               len_out);
>>      case MEDIA_OP_CLASS_SANITIZE:
>> +        if (dpa_range_count == 0) {
>> +            return CXL_MBOX_SUCCESS;
>> +        }
>>          switch (media_op_subclass) {
>> +        case MEDIA_OP_SAN_SUBC_SANITIZE:
>> +            fill_value = 0xF;
>> +            return media_operations_sanitize(ct3d, payload_in, len_in,
>> +                                             payload_out, len_out, fill_value,
>> +                                             cci);
>> +            break;
>
>Can reach this break so remove it.
>
ok
>> +        case MEDIA_OP_SAN_SUBC_ZERO:
>> +            fill_value = 0;
>> +            return media_operations_sanitize(ct3d, payload_in, len_in,
>> +                                             payload_out, len_out, fill_value,
>> +                                             cci);
>> +            break;
>
>Can't reach this break either.
>
ok
>>          default:
>>              return CXL_MBOX_UNSUPPORTED;
>>          }
>> +        break;
>>      default:
>>          return CXL_MBOX_UNSUPPORTED;
>>      }
>> -
>> -    return CXL_MBOX_SUCCESS;
>
>This removal belongs in patch 1
>
>>  }
>
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index a64739be25..b391a293a8 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -581,6 +581,8 @@ typedef struct CXLSetFeatureInfo {
>>      size_t data_size;
>>  } CXLSetFeatureInfo;
>>
>> +struct CXLSanitizeInfo;
>> +
>>  struct CXLType3Dev {
>>      /* Private */
>>      PCIDevice parent_obj;
>> @@ -651,6 +653,8 @@ struct CXLType3Dev {
>>          uint8_t num_regions; /* 0-8 regions */
>>          CXLDCRegion regions[DCD_MAX_NUM_REGION];
>>      } dc;
>> +
>> +    struct CXLSanitizeInfo  *media_op_sanitize;
>
>Only one place before *
>
ok

>>  };
>>
>>  #define TYPE_CXL_TYPE3 "cxl-type3"
>

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