[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] hw/cxl/cxl-mailbox-utils: Add support for Media operatio
|
From: |
Jonathan Cameron |
|
Subject: |
Re: [PATCH 2/2] hw/cxl/cxl-mailbox-utils: Add support for Media operations Sanitize and Write Zeros commands (8.2.9.9.5.3) |
|
Date: |
Fri, 24 Jan 2025 15:19:46 +0000 |
On Thu, 23 Jan 2025 10:39:03 +0530
Vinayak Holikatti <vinayak.kh@samsung.com> wrote:
> CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands.
> CXL devices supports media operations Sanitize and Write zero command.
As before, don't indent this.
>
> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 217 ++++++++++++++++++++++++++++++++++--
> include/hw/cxl/cxl_device.h | 11 ++
> 2 files changed, 220 insertions(+), 8 deletions(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 2315d07fb1..89847ddd9d 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1722,6 +1722,145 @@ static CXLRetCode cmd_sanitize_overwrite(const struct
> cxl_cmd *cmd,
> return CXL_MBOX_BG_STARTED;
> }
>
> +#define DPA_RANGE_GRANULARITY 64
Could use existing CXL_CACHELINE_SIZE definition for this, though I guess
strictly speaking it could be unrelated.
> +struct dpa_range_list_entry {
> + uint64_t starting_dpa;
> + uint64_t length;
> +};
> +
> +static int validate_dpa_addr(CXLType3Dev *ct3d, uint64_t dpa_addr,
> + size_t length)
> +{
> + MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL;
> + uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0;
> + int rc = 0;
> +
> + if ((dpa_addr % DPA_RANGE_GRANULARITY) ||
> + (length % DPA_RANGE_GRANULARITY)) {
Probably makes sense to also check for length 0 here as that would
be very odd if sent.
> + return -EINVAL;
> + }
> +
> + if (ct3d->hostvmem) {
> + vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> + vmr_size = memory_region_size(vmr);
> + }
> + if (ct3d->hostpmem) {
> + pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> + pmr_size = memory_region_size(pmr);
> + }
> + if (ct3d->dc.host_dc) {
> + dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
> + dc_size = memory_region_size(dc_mr);
> + }
> +
> + if (!vmr && !pmr && !dc_mr) {
> + return -ENODEV;
> + }
> +
> + if (dpa_addr >= vmr_size + pmr_size + dc_size ||
> + (dpa_addr + length) > vmr_size + pmr_size + dc_size) {
If length is checked as non zero above, only the second test is needed.
> + return -EINVAL;
> + }
> +
> + if (dpa_addr > vmr_size + pmr_size) {
> + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) {
> + return -ENODEV;
> + }
> + }
> +
> +
> + return rc;
return 0. rc is never set to anything else.
> +}
> +
> +static int sanitize_range(CXLType3Dev *ct3d, uint64_t dpa_addr, size_t
> length,
> + uint8_t fill_value)
> +{
> +
> + MemoryRegion *vmr = NULL, *pmr = NULL;
> + uint64_t vmr_size = 0, pmr_size = 0;
> + AddressSpace *as = NULL;
> + MemTxAttrs mem_attrs = {0};
> +
> + if (ct3d->hostvmem) {
> + vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> + vmr_size = memory_region_size(vmr);
> + }
> + if (ct3d->hostpmem) {
> + pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> + pmr_size = memory_region_size(pmr);
> + }
> +
> + if (dpa_addr < vmr_size) {
> + as = &ct3d->hostvmem_as;
> + } else if (dpa_addr < vmr_size + pmr_size) {
> + as = &ct3d->hostpmem_as;
> + } else {
> + if (!ct3_test_region_block_backed(ct3d, dpa_addr, length)) {
> + return -ENODEV;
> + }
> + as = &ct3d->dc.host_dc_as;
> + }
You could factor out everything down to here and then use that
for the validate_dpa_addr() as finding an address space means
we also checked the address is valid. Otherwise it does not match.
> +
> + return address_space_set(as, dpa_addr,
odd spacing after return. Should just be one space.
> + fill_value, length, mem_attrs);
> +}
> +
> +/* Perform the actual device zeroing */
> +static void __do_sanitize(CXLType3Dev *ct3d)
> +{
> + struct CXLSanitizeInfo *san_info = ct3d->media_op_sanitize;
Single space only before *san_info
> + int dpa_range_count = san_info->dpa_range_count;
> + int rc = 0;
> +
> + for (int i = 0; i < dpa_range_count; i++) {
> + rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa,
> + san_info->dpa_range_list[i].length, san_info->fill_value);
> + if (rc) {
> + goto exit;
> + }
> + }
> + cxl_discard_all_event_records(&ct3d->cxl_dstate);
Add a comment on why we are deleting event records when sanitizing a small
part of memory?
> +exit:
> + g_free(ct3d->media_op_sanitize);
> + ct3d->media_op_sanitize = NULL;
> + return;
> +}
> +
> +static int get_sanitize_duration(uint64_t total_mem)
Where did this come from? Factor out the existing code
in cmd_santize_overwrite() instead of duplicating this stack
of if/else if
Ideally do that as a precursor patch as it's just code movement.
> +{
> + int secs = 0;
> +
> + if (total_mem <= 512) {
> + secs = 4;
> + } else if (total_mem <= 1024) {
> + secs = 8;
> + } else if (total_mem <= 2 * 1024) {
> + secs = 15;
> + } else if (total_mem <= 4 * 1024) {
> + secs = 30;
> + } else if (total_mem <= 8 * 1024) {
> + secs = 60;
> + } else if (total_mem <= 16 * 1024) {
> + secs = 2 * 60;
> + } else if (total_mem <= 32 * 1024) {
> + secs = 4 * 60;
> + } else if (total_mem <= 64 * 1024) {
> + secs = 8 * 60;
> + } else if (total_mem <= 128 * 1024) {
> + secs = 15 * 60;
> + } else if (total_mem <= 256 * 1024) {
> + secs = 30 * 60;
> + } else if (total_mem <= 512 * 1024) {
> + secs = 60 * 60;
> + } else if (total_mem <= 1024 * 1024) {
> + secs = 120 * 60;
> + } else {
> + secs = 240 * 60; /* max 4 hrs */
> + }
> +
> + return secs;
> +}
> +
> enum {
> MEDIA_OP_GENERAL = 0x0,
> MEDIA_OP_SANITIZE = 0x1,
> @@ -1729,10 +1868,9 @@ enum {
> } MEDIA_OPERATION_CLASS;
>
> enum {
> - MEDIA_OP_SUB_DISCOVERY = 0x0,
> - MEDIA_OP_SUB_SANITIZE = 0x0,
> - MEDIA_OP_SUB_ZERO = 0x1,
> - MEDIA_OP_SUB_CLASS_MAX
> + MEDIA_OP_GEN_DISCOVERY = 0x0,
> + MEDIA_OP_SAN_SANITIZE = 0x0,
> + MEDIA_OP_SAN_ZERO = 0x1,
See naming suggestions in first patch. Make sure to tidy up so you don't
introduce
them there then change them here!
> } MEDIA_OPERATION_SUB_CLASS;
>
> struct media_op_supported_list_entry {
> @@ -1777,9 +1915,14 @@ static CXLRetCode cmd_media_operations(const struct
> cxl_cmd *cmd,
> };
> } QEMU_PACKED *media_op_in_pl = (void *)payload_in;
>
> +
Blank line here doesn't add anything.
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> uint8_t media_op_cl = media_op_in_pl->media_operation_class;
> uint8_t media_op_subclass = media_op_in_pl->media_operation_subclass;
> uint32_t dpa_range_count = media_op_in_pl->dpa_range_count;
> + uint8_t fill_value = 0;
> + uint64_t total_mem = 0;
> + int secs = 0;
>
> if (len_in < sizeof(*media_op_in_pl)) {
> return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> @@ -1788,7 +1931,7 @@ static CXLRetCode cmd_media_operations(const struct
> cxl_cmd *cmd,
> switch (media_op_cl) {
> case MEDIA_OP_GENERAL:
> switch (media_op_subclass) {
> - case MEDIA_OP_SUB_DISCOVERY:
> + case MEDIA_OP_GEN_DISCOVERY:
> int count = 0;
> struct media_op_discovery_out_pl *media_out_pl =
> (void *)payload_out;
> @@ -1806,7 +1949,7 @@ static CXLRetCode cmd_media_operations(const struct
> cxl_cmd *cmd,
> return CXL_MBOX_INVALID_INPUT;
> }
>
> - media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER;
> + media_out_pl->dpa_range_granularity = DPA_RANGE_GRANULARITY;
> media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS;
> if (num_ops > 0) {
> for (int i = start_index; i < MAX_SUPPORTED_OPS; i++) {
> @@ -1824,22 +1967,73 @@ disc_out:
> media_out_pl->num_of_supported_operations = count;
> *len_out = sizeof(struct media_op_discovery_out_pl) +
> (sizeof(struct media_op_supported_list_entry) * count);
> - break;
> + goto exit;
return CXL_MBOX_SUCCESS; No purpose in having the exit label as nothing
to be done.
> default:
> return CXL_MBOX_UNSUPPORTED;
> }
> break;
> case MEDIA_OP_SANITIZE:
> + {
From code here not obvious why scoping {} needed.
> switch (media_op_subclass) {
> -
> + case MEDIA_OP_SAN_SANITIZE:
> + if (len_in < (sizeof(*media_op_in_pl) +
> + (dpa_range_count * sizeof(struct dpa_range_list_entry))))
> {
> + return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> + }
The check applies to all current cases. So move it outside this switch statement
to remove duplication. Here all you should do is set the fill_value;
> + fill_value = 0xF;
> + goto sanitize_range;
> + case MEDIA_OP_SAN_ZERO:
> + if (len_in < (sizeof(*media_op_in_pl) +
> + (dpa_range_count * sizeof(struct dpa_range_list_entry)))) {
> + return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> + }
> + fill_value = 0;
> + goto sanitize_range;
Why goto. Just break...
> default:
> return CXL_MBOX_UNSUPPORTED;
> }
and have the stuff below under the sanitize_range label here.
> break;
> + }
> default:
> return CXL_MBOX_UNSUPPORTED;
> }
>
> +sanitize_range:
> + if (dpa_range_count > 0) {
> + for (int i = 0; i < dpa_range_count; i++) {
> + if (validate_dpa_addr(ct3d,
> + media_op_in_pl->dpa_range_list[i].starting_dpa,
> + media_op_in_pl->dpa_range_list[i].length)) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> + total_mem += media_op_in_pl->dpa_range_list[i].length;
> + }
> + ct3d->media_op_sanitize = g_malloc0(sizeof(struct CXLSanitizeInfo) +
> + (dpa_range_count *
> + sizeof(struct dpa_range_list_entry)));
> +
> + if (ct3d->media_op_sanitize) {
> + ct3d->media_op_sanitize->dpa_range_count = dpa_range_count;
> + ct3d->media_op_sanitize->fill_value = fill_value;
> + memcpy(ct3d->media_op_sanitize->dpa_range_list,
> + media_op_in_pl->dpa_range_list,
> + (dpa_range_count *
> + sizeof(struct dpa_range_list_entry)));
> + secs = get_sanitize_duration(total_mem >> 20);
> + goto start_bg;
> + }
> + } else if (dpa_range_count == 0) {
> + goto exit;
if (dpa_range_count == 0) {
return CXL_MBOX_SUCCESS;
}
for (i = 0; i < dpa_range_count; i++)
//that is return early in the simple case the no need for else
and the extra level of indent on these long lines.
> + }
> +
> +start_bg:
> + /* EBUSY other bg cmds as of now */
> + cci->bg.runtime = secs * 1000UL;
> + *len_out = 0;
> + /* sanitize when done */
> + cxl_dev_disable_media(&ct3d->cxl_dstate);
Why? This is santizing part of the device. As I undestand it the
main aim is to offload cleanup when the device is in use. Definitely
don't want to disable media. If I'm missing a reason please give
a spec reference.
> + return CXL_MBOX_BG_STARTED;
> +exit:
> return CXL_MBOX_SUCCESS;
> }
>
> @@ -3154,6 +3348,13 @@ static void bg_timercb(void *opaque)
> cxl_dev_enable_media(&ct3d->cxl_dstate);
> }
> break;
> + case 0x4402: /* Media Operations sanitize */
> + {
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + __do_sanitize(ct3d);
> + cxl_dev_enable_media(&ct3d->cxl_dstate);
Ah. You turn it back on here. Specification reference needed.
> + }
> + break;
> case 0x4304: /* scan media */
> {
> CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index a64739be25..6d82eb266a 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -581,6 +581,15 @@ typedef struct CXLSetFeatureInfo {
> size_t data_size;
> } CXLSetFeatureInfo;
>
> +struct CXLSanitizeInfo {
> + uint32_t dpa_range_count;
> + uint8_t fill_value;
> + struct {
> + uint64_t starting_dpa;
> + uint64_t length;
> + } dpa_range_list[0];
[]
> +};
> +
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -651,6 +660,8 @@ struct CXLType3Dev {
> uint8_t num_regions; /* 0-8 regions */
> CXLDCRegion regions[DCD_MAX_NUM_REGION];
> } dc;
> +
> + struct CXLSanitizeInfo *media_op_sanitize;
Here we just need to know there is a definition of struct
CXLSantizeInfo not what form it takes. So use a forwards
defintion and push the definition of struct CXLSanitizeInfo
down to where it is needed only (in the c file).
Thanks,
Jonathan
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"