[PATCH v2 0/3] CXL CCI Media Operations

[PATCH v2 0/3] CXL CCI Media Operations

[Vinayak Holikatti]

CXL CCI media operations commands implmented as per CXL Specification
3.1 8.2.9.9.5.3
1) General(00h) - Discovery (00h)
2) Sanitize(01h - Sanitize (00h)
                  Write zeros (01h)

The patches are generated against the Johnathan's tree
https://gitlab.com/jic23/qemu.git and branch cxl-2024-11-27.

Changes V1->V2
1) Addressed the review comments from Jonathan Cameron.

2) Modularied the media operations class & subclass handling
into separate functions.

Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>

Vinayak Holikatti (3):
  hw/cxl/cxl-mailbox-utils: Add support for Media operations discovery
    commands (8.2.9.9.5.3)
  hw/cxl: factor out calculation of sanitize duration from
    cmd_santize_overwrite
  hw/cxl/cxl-mailbox-utils: Add support for Media operations Sanitize
    and Write Zeros commands (8.2.9.9.5.3)

 hw/cxl/cxl-mailbox-utils.c  | 404 +++++++++++++++++++++++++++++++++---
 include/hw/cxl/cxl_device.h |   4 +
 2 files changed, 382 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] hw/cxl/cxl-mailbox-utils: Add support for Media operations discovery commands (8.2.9.9.5.3)

[Vinayak Holikatti]

CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands.
CXL devices supports media operations discovery command.

Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c | 136 +++++++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 9c7ea5bc35..fa38ecf507 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -89,6 +89,7 @@ enum {
     SANITIZE    = 0x44,
         #define OVERWRITE     0x0
         #define SECURE_ERASE  0x1
+        #define MEDIA_OPERATIONS 0x2
     PERSISTENT_MEM = 0x45,
         #define GET_SECURITY_STATE     0x0
     MEDIA_AND_POISON = 0x43,
@@ -1721,6 +1722,137 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
     return CXL_MBOX_BG_STARTED;
 }
 
+#define CXL_CACHELINE_SIZE 64
+enum {
+    MEDIA_OP_CLASS_GENERAL  = 0x0,
+        #define MEDIA_OP_GEN_SUBC_DISCOVERY 0x0
+    MEDIA_OP_CLASS_SANITIZE = 0x1,
+        #define MEDIA_OP_SAN_SUBC_SANITIZE 0x0
+        #define MEDIA_OP_SAN_SUBC_ZERO 0x1
+    MEDIA_OP_CLASS_MAX
+};
+
+struct media_op_supported_list_entry {
+    uint8_t media_op_class;
+    uint8_t media_op_subclass;
+};
+
+struct media_op_discovery_out_pl {
+    uint64_t dpa_range_granularity;
+    uint16_t total_supported_operations;
+    uint16_t num_of_supported_operations;
+    struct media_op_supported_list_entry entry[];
+} QEMU_PACKED;
+
+static const struct media_op_supported_list_entry media_op_matrix[] = {
+    {MEDIA_OP_CLASS_GENERAL, MEDIA_OP_GEN_SUBC_DISCOVERY},
+    {MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_SANITIZE},
+    {MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_ZERO}
+};
+
+static CXLRetCode media_operations_discovery(uint8_t *payload_in,
+                                                size_t len_in,
+                                                uint8_t *payload_out,
+                                                size_t *len_out)
+{
+    struct {
+        uint8_t media_operation_class;
+        uint8_t media_operation_subclass;
+        uint8_t rsvd[2];
+        uint32_t dpa_range_count;
+        struct {
+            uint16_t start_index;
+            uint16_t num_supported_ops;
+        } discovery_osa;
+    } QEMU_PACKED *media_op_in_disc_pl = (void *)payload_in;
+    int count = 0;
+
+    if (len_in < sizeof(*media_op_in_disc_pl)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    struct media_op_discovery_out_pl *media_out_pl =
+                                                  (void *)payload_out;
+    int num_ops = media_op_in_disc_pl->discovery_osa.num_supported_ops;
+    int start_index = media_op_in_disc_pl->discovery_osa.start_index;
+
+    if (start_index + num_ops > ARRAY_SIZE(media_op_matrix)) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    media_out_pl->dpa_range_granularity = CXL_CACHELINE_SIZE;
+    media_out_pl->total_supported_operations =
+                                     ARRAY_SIZE(media_op_matrix);
+    if (num_ops > 0) {
+        for (int i = start_index; i < ARRAY_SIZE(media_op_matrix); i++) {
+            media_out_pl->entry[count].media_op_class =
+                    media_op_matrix[i].media_op_class;
+            media_out_pl->entry[count].media_op_subclass =
+                        media_op_matrix[i].media_op_subclass;
+            count++;
+            if (count == num_ops) {
+                break;
+            }
+        }
+    }
+
+    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);
+    return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
+                                         uint8_t *payload_in,
+                                         size_t len_in,
+                                         uint8_t *payload_out,
+                                         size_t *len_out,
+                                         CXLCCI *cci)
+{
+    struct {
+        uint8_t media_operation_class;
+        uint8_t media_operation_subclass;
+        uint8_t rsvd[2];
+        uint32_t dpa_range_count;
+    } QEMU_PACKED *media_op_in_common_pl = (void *)payload_in;
+
+    if (len_in < sizeof(*media_op_in_common_pl)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    uint8_t media_op_cl = media_op_in_common_pl->media_operation_class;
+    uint8_t media_op_subclass =
+                      media_op_in_common_pl->media_operation_subclass;
+    uint32_t dpa_range_count = media_op_in_common_pl->dpa_range_count;
+
+    switch (media_op_cl) {
+    case MEDIA_OP_CLASS_GENERAL:
+        if (media_op_subclass != MEDIA_OP_GEN_SUBC_DISCOVERY) {
+            return CXL_MBOX_UNSUPPORTED;
+        }
+
+        /*
+         * As per spec CXL 3.1 8.2.9.9.5.3 dpa_range_count
+         * should be zero for discovery sub class command
+         */
+        if (dpa_range_count) {
+            return CXL_MBOX_INVALID_INPUT;
+        }
+
+        return media_operations_discovery(payload_in, len_in, payload_out,
+                                             len_out);
+    case MEDIA_OP_CLASS_SANITIZE:
+        switch (media_op_subclass) {
+        default:
+            return CXL_MBOX_UNSUPPORTED;
+        }
+    default:
+        return CXL_MBOX_UNSUPPORTED;
+    }
+
+    return CXL_MBOX_SUCCESS;
+}
+
 static CXLRetCode cmd_get_security_state(const struct cxl_cmd *cmd,
                                          uint8_t *payload_in,
                                          size_t len_in,
@@ -2864,6 +2996,10 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
          CXL_MBOX_SECURITY_STATE_CHANGE |
          CXL_MBOX_BACKGROUND_OPERATION |
          CXL_MBOX_BACKGROUND_OPERATION_ABORT)},
+    [SANITIZE][MEDIA_OPERATIONS] = { "MEDIA_OPERATIONS", cmd_media_operations,
+        ~0,
+        (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
+         CXL_MBOX_BACKGROUND_OPERATION)},
     [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE",
         cmd_get_security_state, 0, 0 },
     [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
-- 
2.34.1

[PATCH v2 2/3] hw/cxl: factor out calculation of sanitize duration from cmd_santize_overwrite

[Vinayak Holikatti]

Move the code of calculation of sanitize duration into function
for usability by other sanitize routines

Estimate times based on:
             https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c | 61 ++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index fa38ecf507..d58c20842f 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1656,34 +1656,10 @@ static void __do_sanitization(CXLType3Dev *ct3d)
     cxl_discard_all_event_records(&ct3d->cxl_dstate);
 }
 
-/*
- * CXL r3.1 Section 8.2.9.9.5.1: Sanitize (Opcode 4400h)
- *
- * Once the Sanitize command has started successfully, the device shall be
- * placed in the media disabled state. If the command fails or is interrupted
- * by a reset or power failure, it shall remain in the media disabled state
- * until a successful Sanitize command has been completed. During this state:
- *
- * 1. Memory writes to the device will have no effect, and all memory reads
- * will return random values (no user data returned, even for locations that
- * the failed Sanitize operation didn’t sanitize yet).
- *
- * 2. Mailbox commands shall still be processed in the disabled state, except
- * that commands that access Sanitized areas shall fail with the Media Disabled
- * error code.
- */
-static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
-                                         uint8_t *payload_in,
-                                         size_t len_in,
-                                         uint8_t *payload_out,
-                                         size_t *len_out,
-                                         CXLCCI *cci)
+static int get_sanitize_duration(uint64_t total_mem)
 {
-    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
-    uint64_t total_mem; /* in Mb */
-    int secs;
+    int secs = 0;
 
-    total_mem = (ct3d->cxl_dstate.vmem_size + ct3d->cxl_dstate.pmem_size) >> 20;
     if (total_mem <= 512) {
         secs = 4;
     } else if (total_mem <= 1024) {
@@ -1712,6 +1688,39 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
         secs = 240 * 60; /* max 4 hrs */
     }
 
+    return secs;
+}
+
+/*
+ * CXL r3.1 Section 8.2.9.9.5.1: Sanitize (Opcode 4400h)
+ *
+ * Once the Sanitize command has started successfully, the device shall be
+ * placed in the media disabled state. If the command fails or is interrupted
+ * by a reset or power failure, it shall remain in the media disabled state
+ * until a successful Sanitize command has been completed. During this state:
+ *
+ * 1. Memory writes to the device will have no effect, and all memory reads
+ * will return random values (no user data returned, even for locations that
+ * the failed Sanitize operation didn’t sanitize yet).
+ *
+ * 2. Mailbox commands shall still be processed in the disabled state, except
+ * that commands that access Sanitized areas shall fail with the Media Disabled
+ * error code.
+ */
+static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
+                                         uint8_t *payload_in,
+                                         size_t len_in,
+                                         uint8_t *payload_out,
+                                         size_t *len_out,
+                                         CXLCCI *cci)
+{
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    uint64_t total_mem; /* in Mb */
+    int secs;
+
+    total_mem = (ct3d->cxl_dstate.vmem_size + ct3d->cxl_dstate.pmem_size) >> 20;
+    secs = get_sanitize_duration(total_mem);
+
     /* EBUSY other bg cmds as of now */
     cci->bg.runtime = secs * 1000UL;
     *len_out = 0;
-- 
2.34.1

[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]

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 |   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;
+};
+
+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;
+}
+
+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;
+
+    if ((dpa_addr % CXL_CACHELINE_SIZE) ||
+         (length % CXL_CACHELINE_SIZE)  ||
+         (length <= 0)) {
+        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) {
+        return -ENODEV;
+    }
+
+    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)
+{
+
+    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);
+}
+
+/* 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;
+        }
+    }
+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)
 {
     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++) {
+        if (validate_dpa_addr(ct3d,
+            media_op_in_sanitize_pl->dpa_range_list[i].starting_dpa,
+            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;
+
+}
+
 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;
+
     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;
+        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;
         default:
             return CXL_MBOX_UNSUPPORTED;
         }
+        break;
     default:
         return CXL_MBOX_UNSUPPORTED;
     }
-
-    return CXL_MBOX_SUCCESS;
 }
 
 static CXLRetCode cmd_get_security_state(const struct cxl_cmd *cmd,
@@ -3173,6 +3374,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..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;
 };
 
 #define TYPE_CXL_TYPE3 "cxl-type3"
-- 
2.34.1

Re: [PATCH v2 1/3] hw/cxl/cxl-mailbox-utils: Add support for Media operations discovery commands (8.2.9.9.5.3)

[Jonathan Cameron via]

On Thu, 13 Feb 2025 14:45:56 +0530
Vinayak Holikatti <vinayak.kh@samsung.com> wrote:

> CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands.

Given the CXL consortium only makes the latest spec available,
generally we try to reference that.
It's move to 8.2.10.9.5.3 in r3.2

Otherwise mostly minor style comments inline.

Thanks,

Jonathan



> CXL devices supports media operations discovery command.
> 
> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 136 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 9c7ea5bc35..fa38ecf507 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -89,6 +89,7 @@ enum {
>      SANITIZE    = 0x44,
>          #define OVERWRITE     0x0
>          #define SECURE_ERASE  0x1
> +        #define MEDIA_OPERATIONS 0x2
>      PERSISTENT_MEM = 0x45,
>          #define GET_SECURITY_STATE     0x0
>      MEDIA_AND_POISON = 0x43,
> @@ -1721,6 +1722,137 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>      return CXL_MBOX_BG_STARTED;
>  }
>  
> +#define CXL_CACHELINE_SIZE 64

Already defined in include/hw/cxl/cxl.h

> +enum {
> +    MEDIA_OP_CLASS_GENERAL  = 0x0,
> +        #define MEDIA_OP_GEN_SUBC_DISCOVERY 0x0
> +    MEDIA_OP_CLASS_SANITIZE = 0x1,
> +        #define MEDIA_OP_SAN_SUBC_SANITIZE 0x0
> +        #define MEDIA_OP_SAN_SUBC_ZERO 0x1
> +    MEDIA_OP_CLASS_MAX
> +};
> +
> +struct media_op_supported_list_entry {
> +    uint8_t media_op_class;
> +    uint8_t media_op_subclass;
> +};
> +
> +struct media_op_discovery_out_pl {
> +    uint64_t dpa_range_granularity;
> +    uint16_t total_supported_operations;
> +    uint16_t num_of_supported_operations;
> +    struct media_op_supported_list_entry entry[];
> +} QEMU_PACKED;
> +
> +static const struct media_op_supported_list_entry media_op_matrix[] = {
> +    {MEDIA_OP_CLASS_GENERAL, MEDIA_OP_GEN_SUBC_DISCOVERY},
> +    {MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_SANITIZE},
> +    {MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_ZERO}
Add trailing comma as we may well get more of these in future.
In general use a trailing comma whenever there isn't a definite reason
we will never get them.

Also I'd prefer space after { and before } to match local style.
    { MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_ZERO },

> +};
> +
> +static CXLRetCode media_operations_discovery(uint8_t *payload_in,
> +                                                size_t len_in,
> +                                                uint8_t *payload_out,
> +                                                size_t *len_out)
Align to opening bracket (just after it)
> +{
> +    struct {
> +        uint8_t media_operation_class;
> +        uint8_t media_operation_subclass;
> +        uint8_t rsvd[2];
> +        uint32_t dpa_range_count; 
> +        struct {
> +            uint16_t start_index;
> +            uint16_t num_supported_ops;

I'd just call this num or num_ops


> +        } discovery_osa;
> +    } QEMU_PACKED *media_op_in_disc_pl = (void *)payload_in;
> +    int count = 0;
> +
> +    if (len_in < sizeof(*media_op_in_disc_pl)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
> +    struct media_op_discovery_out_pl *media_out_pl =
> +                                                  (void *)payload_out;
> +    int num_ops = media_op_in_disc_pl->discovery_osa.num_supported_ops;
> +    int start_index = media_op_in_disc_pl->discovery_osa.start_index;

Generally we don't mix declarations and code. So move these local variable
declarations up.


> +
> +    if (start_index + num_ops > ARRAY_SIZE(media_op_matrix)) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    media_out_pl->dpa_range_granularity = CXL_CACHELINE_SIZE;
> +    media_out_pl->total_supported_operations =
> +                                     ARRAY_SIZE(media_op_matrix);
> +    if (num_ops > 0) {
> +        for (int i = start_index; i < ARRAY_SIZE(media_op_matrix); i++) {

Given you already checked for going out of range, can just do 
i < start_index + num_ops
I think and avoid the need to break or keep a count.

Keep to local style and declare i outside the loop


> +            media_out_pl->entry[count].media_op_class =
> +                    media_op_matrix[i].media_op_class;
> +            media_out_pl->entry[count].media_op_subclass =
> +                        media_op_matrix[i].media_op_subclass;
> +            count++;
> +            if (count == num_ops) {
> +                break;
> +            }
> +        }
> +    }
> +
> +    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);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
> +                                         uint8_t *payload_in,

Alignment should be to opening bracket.

> +                                         size_t len_in,
> +                                         uint8_t *payload_out,
> +                                         size_t *len_out,
> +                                         CXLCCI *cci)
> +{
> +    struct {
> +        uint8_t media_operation_class;
> +        uint8_t media_operation_subclass;
> +        uint8_t rsvd[2];
> +        uint32_t dpa_range_count;
> +    } QEMU_PACKED *media_op_in_common_pl = (void *)payload_in;
> +
> +    if (len_in < sizeof(*media_op_in_common_pl)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
> +    uint8_t media_op_cl = media_op_in_common_pl->media_operation_class;
> +    uint8_t media_op_subclass =
> +                      media_op_in_common_pl->media_operation_subclass;
> +    uint32_t dpa_range_count = media_op_in_common_pl->dpa_range_count;

As above, traditional c style with declarations before code.

> +
> +    switch (media_op_cl) {
> +    case MEDIA_OP_CLASS_GENERAL:
> +        if (media_op_subclass != MEDIA_OP_GEN_SUBC_DISCOVERY) {
> +            return CXL_MBOX_UNSUPPORTED;
> +        }
> +
> +        /*
> +         * As per spec CXL 3.1 8.2.9.9.5.3 dpa_range_count
> +         * should be zero for discovery sub class command
> +         */

I would move this into media_operations_discovery.

> +        if (dpa_range_count) {
> +            return CXL_MBOX_INVALID_INPUT;
> +        }
> +
> +        return media_operations_discovery(payload_in, len_in, payload_out,
> +                                             len_out);
> +    case MEDIA_OP_CLASS_SANITIZE:

Easier to introduce this case in next patch.  Until then can just let
the default deal with it.

> +        switch (media_op_subclass) {
> +        default:
> +            return CXL_MBOX_UNSUPPORTED;
> +        }
> +    default:
> +        return CXL_MBOX_UNSUPPORTED;
> +    }
> +
> +    return CXL_MBOX_SUCCESS;
> +}
> +

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)

[Jonathan Cameron via]

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.

A few comments inline.

Thanks,

Jonathan

> 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

> +
> +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;
}

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.

> +
> +    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)) {

> +        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.

> +        return -ENODEV;
> +    }
> +
> +    if ((dpa_addr + length) > vmr_size + pmr_size + dc_size) {
Skip inner brackets.

> +        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 (

> +}
> +
> +/* 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).

> +        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.

>  {
>      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)

> +        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.

    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.

> +}
> +
>  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?

> +
>      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.

> +        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.

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

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

Re: [PATCH v2 1/3] hw/cxl/cxl-mailbox-utils: Add support for Media operations discovery commands (8.2.9.9.5.3)

[Vinayak Holikatti]

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

On 14/02/25 02:08PM, Jonathan Cameron wrote:
>On Thu, 13 Feb 2025 14:45:56 +0530
>Vinayak Holikatti <vinayak.kh@samsung.com> wrote:
>
>> CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands.
>
>Given the CXL consortium only makes the latest spec available,
>generally we try to reference that.
>It's move to 8.2.10.9.5.3 in r3.2
>
>Otherwise mostly minor style comments inline.
>
>Thanks,
>
>Jonathan
>
Thank You for feed back will update accordingly in V3
>
>
>> CXL devices supports media operations discovery command.
>>
>> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
>> ---
>>  hw/cxl/cxl-mailbox-utils.c | 136 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 136 insertions(+)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 9c7ea5bc35..fa38ecf507 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -89,6 +89,7 @@ enum {
>>      SANITIZE    = 0x44,
>>          #define OVERWRITE     0x0
>>          #define SECURE_ERASE  0x1
>> +        #define MEDIA_OPERATIONS 0x2
>>      PERSISTENT_MEM = 0x45,
>>          #define GET_SECURITY_STATE     0x0
>>      MEDIA_AND_POISON = 0x43,
>> @@ -1721,6 +1722,137 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_BG_STARTED;
>>  }
>>
>> +#define CXL_CACHELINE_SIZE 64
>
>Already defined in include/hw/cxl/cxl.h
ok 
>
>> +enum {
>> +    MEDIA_OP_CLASS_GENERAL  = 0x0,
>> +        #define MEDIA_OP_GEN_SUBC_DISCOVERY 0x0
>> +    MEDIA_OP_CLASS_SANITIZE = 0x1,
>> +        #define MEDIA_OP_SAN_SUBC_SANITIZE 0x0
>> +        #define MEDIA_OP_SAN_SUBC_ZERO 0x1
>> +    MEDIA_OP_CLASS_MAX
>> +};
>> +
>> +struct media_op_supported_list_entry {
>> +    uint8_t media_op_class;
>> +    uint8_t media_op_subclass;
>> +};
>> +
>> +struct media_op_discovery_out_pl {
>> +    uint64_t dpa_range_granularity;
>> +    uint16_t total_supported_operations;
>> +    uint16_t num_of_supported_operations;
>> +    struct media_op_supported_list_entry entry[];
>> +} QEMU_PACKED;
>> +
>> +static const struct media_op_supported_list_entry media_op_matrix[] = {
>> +    {MEDIA_OP_CLASS_GENERAL, MEDIA_OP_GEN_SUBC_DISCOVERY},
>> +    {MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_SANITIZE},
>> +    {MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_ZERO}
>Add trailing comma as we may well get more of these in future.
>In general use a trailing comma whenever there isn't a definite reason
>we will never get them.
ok
>
>Also I'd prefer space after { and before } to match local style.
>    { MEDIA_OP_CLASS_SANITIZE, MEDIA_OP_SAN_SUBC_ZERO },
>
ok
>> +};
>> +
>> +static CXLRetCode media_operations_discovery(uint8_t *payload_in,
>> +                                                size_t len_in,
>> +                                                uint8_t *payload_out,
>> +                                                size_t *len_out)
>Align to opening bracket (just after it)
ok
>> +{
>> +    struct {
>> +        uint8_t media_operation_class;
>> +        uint8_t media_operation_subclass;
>> +        uint8_t rsvd[2];
>> +        uint32_t dpa_range_count;
>> +        struct {
>> +            uint16_t start_index;
>> +            uint16_t num_supported_ops;
>
ok
>I'd just call this num or num_ops
>
ok 
>
>> +        } discovery_osa;
>> +    } QEMU_PACKED *media_op_in_disc_pl = (void *)payload_in;
>> +    int count = 0;
>> +
>> +    if (len_in < sizeof(*media_op_in_disc_pl)) {
>> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>> +    }
>> +
>> +    struct media_op_discovery_out_pl *media_out_pl =
>> +                                                  (void *)payload_out;
>> +    int num_ops = media_op_in_disc_pl->discovery_osa.num_supported_ops;
>> +    int start_index = media_op_in_disc_pl->discovery_osa.start_index;
>
>Generally we don't mix declarations and code. So move these local variable
>declarations up.
>
ok
>
>> +
>> +    if (start_index + num_ops > ARRAY_SIZE(media_op_matrix)) {
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>> +
>> +    media_out_pl->dpa_range_granularity = CXL_CACHELINE_SIZE;
>> +    media_out_pl->total_supported_operations =
>> +                                     ARRAY_SIZE(media_op_matrix);
>> +    if (num_ops > 0) {
>> +        for (int i = start_index; i < ARRAY_SIZE(media_op_matrix); i++) {
>
>Given you already checked for going out of range, can just do
>i < start_index + num_ops
>I think and avoid the need to break or keep a count.
>
ok
>Keep to local style and declare i outside the loop
>
ok
>
>> +            media_out_pl->entry[count].media_op_class =
>> +                    media_op_matrix[i].media_op_class;
>> +            media_out_pl->entry[count].media_op_subclass =
>> +                        media_op_matrix[i].media_op_subclass;
>> +            count++;
>> +            if (count == num_ops) {
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>> +    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);
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>> +static CXLRetCode cmd_media_operations(const struct cxl_cmd *cmd,
>> +                                         uint8_t *payload_in,
>
>Alignment should be to opening bracket.
>
ok
>> +                                         size_t len_in,
>> +                                         uint8_t *payload_out,
>> +                                         size_t *len_out,
>> +                                         CXLCCI *cci)
>> +{
>> +    struct {
>> +        uint8_t media_operation_class;
>> +        uint8_t media_operation_subclass;
>> +        uint8_t rsvd[2];
>> +        uint32_t dpa_range_count;
>> +    } QEMU_PACKED *media_op_in_common_pl = (void *)payload_in;
>> +
>> +    if (len_in < sizeof(*media_op_in_common_pl)) {
>> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>> +    }
>> +
>> +    uint8_t media_op_cl = media_op_in_common_pl->media_operation_class;
>> +    uint8_t media_op_subclass =
>> +                      media_op_in_common_pl->media_operation_subclass;
>> +    uint32_t dpa_range_count = media_op_in_common_pl->dpa_range_count;
>
>As above, traditional c style with declarations before code.
>
ok
>> +
>> +    switch (media_op_cl) {
>> +    case MEDIA_OP_CLASS_GENERAL:
>> +        if (media_op_subclass != MEDIA_OP_GEN_SUBC_DISCOVERY) {
>> +            return CXL_MBOX_UNSUPPORTED;
>> +        }
>> +
>> +        /*
>> +         * As per spec CXL 3.1 8.2.9.9.5.3 dpa_range_count
>> +         * should be zero for discovery sub class command
>> +         */
>
>I would move this into media_operations_discovery.
>
ok
>> +        if (dpa_range_count) {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +
>> +        return media_operations_discovery(payload_in, len_in, payload_out,
>> +                                             len_out);
>> +    case MEDIA_OP_CLASS_SANITIZE:
>
>Easier to introduce this case in next patch.  Until then can just let
>the default deal with it.
>
ok
>> +        switch (media_op_subclass) {
>> +        default:
>> +            return CXL_MBOX_UNSUPPORTED;
>> +        }
>> +    default:
>> +        return CXL_MBOX_UNSUPPORTED;
>> +    }
>> +
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>
>

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

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]

[-- 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 --]