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.1On 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"
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"
>
© 2016 - 2025 Red Hat, Inc.