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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 20 Feb 2025 10:57:24 +0530
Vinayak Holikatti <vinayak.kh@samsung.com> wrote:

> CXL spec 3.2 section 8.2.10.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>

Another one where I made some minor tweaks whilst applying. Let me know if I
messed up!  Comments on why folow.

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 4aa695cfa9..a48a551bfc 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1796,7 +1796,7 @@ static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length,

     uint64_t vmr_size, pmr_size;
     AddressSpace *as = NULL;
-    MemTxAttrs mem_attrs = {0};
+    MemTxAttrs mem_attrs = {};

     vmr_size = get_vmr_size(ct3d, NULL);
     pmr_size = get_pmr_size(ct3d, NULL);
@@ -1820,7 +1820,8 @@ 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, i;
+    int rc = 0;
+    int i;

     for (i = 0; i < dpa_range_count; i++) {
         rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa,
@@ -1933,21 +1934,19 @@ static CXLRetCode media_operations_sanitize(CXLType3Dev *ct3d,
         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[];
+        struct dpa_range_list_entry 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;
+    size_t dpa_range_list_size;
     int secs = 0, i;

     if (dpa_range_count == 0) {
         return CXL_MBOX_SUCCESS;
     }

-    if (len_in < (sizeof(*media_op_in_sanitize_pl) +
-           (dpa_range_count * sizeof(struct dpa_range_list_entry)))) {
+    dpa_range_list_size = dpa_range_count * sizeof(struct dpa_range_list_entry);
+    if (len_in < (sizeof(*media_op_in_sanitize_pl) + dpa_range_list_size)) {
         return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
     }

@@ -1955,24 +1954,21 @@ static CXLRetCode media_operations_sanitize(CXLType3Dev *ct3d,
         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, start_dpa, length)) {
             return CXL_MBOX_INVALID_INPUT;
         }
-        total_mem += media_op_in_sanitize_pl->dpa_range_list[i].length;
+        total_mem += 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);
-    }
+                                        dpa_range_list_size);
+
+    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_list_size);
+    secs = get_sanitize_duration(total_mem >> 20);

     /* EBUSY other bg cmds as of now */
     cci->bg.runtime = secs * 1000UL;
> ---
>  hw/cxl/cxl-mailbox-utils.c  | 210 +++++++++++++++++++++++++++++++++++-
>  include/hw/cxl/cxl_device.h |   4 +
>  2 files changed, 213 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 2428d85fef..7b4e4daeb2 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1731,6 +1731,130 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>      return CXL_MBOX_BG_STARTED;
>  }
>  
> +struct dpa_range_list_entry {
> +    uint64_t starting_dpa;
> +    uint64_t length;
> +} QEMU_PACKED;
> +
> +struct CXLSanitizeInfo {
> +    uint32_t dpa_range_count;
> +    uint8_t fill_value;
> +    struct dpa_range_list_entry dpa_range_list[];
> +} QEMU_PACKED;
> +
> +static uint64_t get_vmr_size(CXLType3Dev *ct3d, MemoryRegion **vmr)
> +{
> +    MemoryRegion *mr;
> +    if (ct3d->hostvmem) {
> +        mr = host_memory_backend_get_memory(ct3d->hostvmem);
> +        if (vmr) {
> +            *vmr = mr;
> +        }
> +        return memory_region_size(mr);
> +    }
> +    return 0;
> +}
> +
> +static uint64_t get_pmr_size(CXLType3Dev *ct3d, MemoryRegion **pmr)
> +{
> +    MemoryRegion *mr;
> +    if (ct3d->hostpmem) {
> +        mr = host_memory_backend_get_memory(ct3d->hostpmem);
> +        if (pmr) {
> +            *pmr = mr;
> +        }
> +        return memory_region_size(mr);
> +    }
> +    return 0;
> +}
> +
> +static uint64_t get_dc_size(CXLType3Dev *ct3d, MemoryRegion **dc_mr)
> +{
> +    MemoryRegion *mr;
> +    if (ct3d->dc.host_dc) {
> +        mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
> +        if (dc_mr) {
> +            *dc_mr = mr;
> +        }
> +        return memory_region_size(mr);
> +    }
> +    return 0;
> +}
> +
> +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr,
> +                             size_t length)
> +{
> +    uint64_t vmr_size, pmr_size, dc_size;
> +
> +    if ((dpa_addr % CXL_CACHE_LINE_SIZE) ||
> +        (length % CXL_CACHE_LINE_SIZE)  ||

extra spce before the ||

> +        (length <= 0)) {
> +        return -EINVAL;
> +    }
> +
> +    vmr_size = get_vmr_size(ct3d, NULL);
> +    pmr_size = get_pmr_size(ct3d, NULL);
> +    dc_size = get_dc_size(ct3d, NULL);
> +
> +    if (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 0;
> +}
> +
> +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t length,
> +                          uint8_t fill_value)
> +{
> +
> +    uint64_t vmr_size, pmr_size;
> +    AddressSpace *as = NULL;
> +    MemTxAttrs mem_attrs = {0};

No need for the 0.  {} is same thing.

> +
> +    vmr_size = get_vmr_size(ct3d, NULL);
> +    pmr_size = get_pmr_size(ct3d, NULL);
> +
> +    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, i;

I'll spit this.  Generally mixing assignment and non assignment declarations
can be a small readabilty issue.

> +
> +    for (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;
> +        }
> +    }
> +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
> @@ -1823,6 +1947,70 @@ 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[];

struct dpa_range_list_entry dpa_range_list[] which makes the 

> +    } 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, i;
As above.
> +
> +    if (dpa_range_count == 0) {
> +        return CXL_MBOX_SUCCESS;
> +    }
> +
> +    if (len_in < (sizeof(*media_op_in_sanitize_pl) +
> +           (dpa_range_count * sizeof(struct dpa_range_list_entry)))) {
Lots of use is made of this size of the dpa_range list.
Deserves a local variable.

> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
> +    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, start_dpa, length)) {
> +            return CXL_MBOX_INVALID_INPUT;
> +        }
> +        total_mem += media_op_in_sanitize_pl->dpa_range_list[i].length;
totalmem += length.

> +    }
> +    ct3d->media_op_sanitize = g_malloc0(sizeof(struct CXLSanitizeInfo) +
> +                                  (dpa_range_count *
> +                                  sizeof(struct dpa_range_list_entry)));
> +
> +    if (ct3d->media_op_sanitize) {
This can't fail.  g_malloc0 failing kills qemu anyway (makes memory
handling simple ;)

> +        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,
Align after the (
> +                  (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;
> +}
> +
>  static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>                                         uint8_t *payload_in,
>                                         size_t len_in,
> @@ -1836,6 +2024,7 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>          uint8_t rsvd[2];
>          uint32_t dpa_range_count;
>      } QEMU_PACKED *media_op_in_common_pl = (void *)payload_in;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>      uint8_t media_op_cl = 0;
>      uint8_t media_op_subclass = 0;
>  
> @@ -1853,7 +2042,20 @@ static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>          }
>  
>          return media_operations_discovery(payload_in, len_in, payload_out,
> -                                             len_out);
> +                                          len_out);
> +    case MEDIA_OP_CLASS_SANITIZE:
> +        switch (media_op_subclass) {
> +        case MEDIA_OP_SAN_SUBC_SANITIZE:
> +            return media_operations_sanitize(ct3d, payload_in, len_in,
> +                                             payload_out, len_out, 0xF,
> +                                             cci);
> +        case MEDIA_OP_SAN_SUBC_ZERO:
> +            return media_operations_sanitize(ct3d, payload_in, len_in,
> +                                             payload_out, len_out, 0,
> +                                             cci);
> +        default:
> +            return CXL_MBOX_UNSUPPORTED;
> +        }
>      default:
>          return CXL_MBOX_UNSUPPORTED;
>      }
> @@ -3170,6 +3372,12 @@ 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);
> +        }
> +        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..3c5711249b 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;
>  };
>  
>  #define TYPE_CXL_TYPE3 "cxl-type3"