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

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

[Jonathan Cameron]

On Thu, 23 Jan 2025 10:39:02 +0530
Vinayak Holikatti <vinayak.kh@samsung.com> wrote:

Hi Vinayak,

Thanks for your patch!  Good to add support for this.

Various comments inline, but all fairly minor things.

thanks,

Jonathan


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

Please don't indent the commit message. Maybe this is a side effect
of some tooling but definitely clean it up before sending a v2.

> 
> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
+CC linux-cxl to increase chance of review and let people know this
exists.

> ---
>  hw/cxl/cxl-mailbox-utils.c | 130 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 128 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 9c7ea5bc35..2315d07fb1 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -87,8 +87,9 @@ enum {
>          #define GET_LSA       0x2
>          #define SET_LSA       0x3
>      SANITIZE    = 0x44,
> -        #define OVERWRITE     0x0
> -        #define SECURE_ERASE  0x1
> +        #define OVERWRITE        0x0
> +        #define SECURE_ERASE     0x1
> +        #define MEDIA_OPERATIONS 0x2

Trivial but I've given up trying to keep these aligned.
It's a fools game as the names get steadily longer.

As such better to just leave the existing pair alone.

>      PERSISTENT_MEM = 0x45,
>          #define GET_SECURITY_STATE     0x0
>      MEDIA_AND_POISON = 0x43,
> @@ -1721,6 +1722,127 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>      return CXL_MBOX_BG_STARTED;
>  }
>  
> +enum {
> +    MEDIA_OP_GENERAL  = 0x0,
I'd name them so the field id explicit.

MEDIA_OP_CLASS_GENERAL
etc

> +    MEDIA_OP_SANITIZE = 0x1,
> +    MEDIA_OP_CLASS_MAX,
No comma on terminating entry. We don't want it to be easy to add
stuff after it.

> +} MEDIA_OPERATION_CLASS;
The enum type is never used.  So might as well keep it anonymous
like we do for other enums in this file.

> +
> +enum {
> +    MEDIA_OP_SUB_DISCOVERY = 0x0,
This set of class and subcalss is similar to the enum you add
the MEDIA_OPERATIONS define to above.
I'd take a similar strategy with

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

or something like that.
}
> +    MEDIA_OP_SUB_SANITIZE = 0x0,
> +    MEDIA_OP_SUB_ZERO     = 0x1,
> +    MEDIA_OP_SUB_CLASS_MAX
No need for SUB_CLASS_MAX as you don't seem to use it.

> +} MEDIA_OPERATION_SUB_CLASS;
> +
> +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[0];
entry[] 

which is the c spec defined way to do variable length last elements.
The [0] was I think a weird extension that we have moved away from.

> +};

Not strictly necessary but I'd mark it packed as chances of future breakage
are high with a structure starting at byte 0xC.

> +
> +#define MAX_SUPPORTED_OPS 3
I'd avoid explicit define for this and just use ARRAY_SIZE() on the
array of structures to find out.

> +struct media_op_supported_list_entry media_op_matrix[MAX_SUPPORTED_OPS] = {

Use the defines above rather than the numeric values.
Then it's obvious what this is, also mark it static const.

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

> +                                                            {0, 0},
> +                                                            {1, 0},
> +                                                            {1, 1} };
> +
> +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;
    struct {
        uint8_t media_operation_class;

etc for alignment.

> +    uint8_t media_operation_subclass;
> +    uint8_t rsvd[2];
> +    uint32_t dpa_range_count;
> +    union {
> +        struct {
> +            uint64_t starting_dpa;
> +            uint64_t length;
> +        } dpa_range_list[0];
[]

> +        struct {
> +            uint16_t start_index;
> +            uint16_t num_supported_ops;
> +        } discovery_osa;
> +    };

This is a little tricky as in theory you can have a variable number
of DPA Range List elements and then the operation specific arguments.

However, general always provides a range count of 0.  Also both sanitize
and zero have no osa elemetns.  Add a comment
about this so we don't think it looks wrong in future + do notice that
this approach doesn't generalize if a new operation allows dpa ranges
and operation specific parameters.


> +    } QEMU_PACKED *media_op_in_pl = (void *)payload_in;
> +
> +    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;
> +
> +    if (len_in < sizeof(*media_op_in_pl)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }

Test this before getting values to fill in media_op_cl local variables etc.
It's both logically correct and may constrain the compiler not to get too smart
if it can see enough to realize what len_in is.

> +
> +    switch (media_op_cl) {
> +    case MEDIA_OP_GENERAL:
> +        switch (media_op_subclass) {
> +        case MEDIA_OP_SUB_DISCOVERY:
Given there is only one element, maybe cleaner as
           if (media_op_subclass != MEDIA_OP_SUB_DISCOVERY) {
                return CXL_MBOX_UNSUPPORTED;
           }
AS reduces indent of the following, helping readability a litle.

> +            int count = 0;
> +            struct media_op_discovery_out_pl *media_out_pl =
> +                (void *)payload_out;
> +            int num_ops = media_op_in_pl->discovery_osa.num_supported_ops;
> +            int start_index = media_op_in_pl->discovery_osa.start_index;
> +
> +            /* As per spec CXL 3.1 8.2.9.9.5.3 dpa_range_count */
> +            /* should be zero for discovery sub class command */
Local style is multiline comment as
               /*
                * As per spec CXL 3.1...
                * should be zero...
                */

> +            if (dpa_range_count) {
> +                return CXL_MBOX_INVALID_INPUT;
> +            }
> +
> +            if ((start_index >= MEDIA_OP_CLASS_MAX) ||
> +                (num_ops > MAX_SUPPORTED_OPS)) {

Check here should be for num_ops + start_index > MAX_SUPPORTED OPS
Comparing start_index against MEDIA_OP_CLASS_MAX doesn't make sense to me
as I believe it's an index into the array of Class / subclass pairs not
the class array.


> +                return CXL_MBOX_INVALID_INPUT;
> +            }
> +
> +            media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER;
> +            media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS;
> +            if (num_ops > 0) {
> +                for (int i = start_index; i < MAX_SUPPORTED_OPS; 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) {
> +                        goto disc_out;

break should be enough and removes need for goto and label.

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

indent this line.

> +            break;
I'd
        return CXL_MBOX_SUCCESS;

> +        default:
> +            return CXL_MBOX_UNSUPPORTED;
> +        }
> +        break;
then this break isn't needed.
> +    case MEDIA_OP_SANITIZE:
> +        switch (media_op_subclass) {
> +
No blank line here yet.
> +        default:
> +            return CXL_MBOX_UNSUPPORTED;
> +        }
Similar. Return in all paths so no break.
> +        break;
> +    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 +2986,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",

Re: [PATCH 1/2] 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: 10474 bytes --]

On 24/01/25 02:56PM, Jonathan Cameron wrote:
>On Thu, 23 Jan 2025 10:39:02 +0530
>Vinayak Holikatti <vinayak.kh@samsung.com> wrote:
>
>Hi Vinayak,
>
>Thanks for your patch!  Good to add support for this.
>
>Various comments inline, but all fairly minor things.
>
>thanks,
>
>Jonathan
>
>
>>     CXL spec 3.1 section 8.2.9.9.5.3 describes media operations commands.
>>     CXL devices supports media operations discovery command.
>
>Please don't indent the commit message. Maybe this is a side effect
>of some tooling but definitely clean it up before sending a v2.
>
>>
>> Signed-off-by: Vinayak Holikatti <vinayak.kh@samsung.com>
>+CC linux-cxl to increase chance of review and let people know this
>exists.
>
>> ---
>>  hw/cxl/cxl-mailbox-utils.c | 130 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 128 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 9c7ea5bc35..2315d07fb1 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -87,8 +87,9 @@ enum {
>>          #define GET_LSA       0x2
>>          #define SET_LSA       0x3
>>      SANITIZE    = 0x44,
>> -        #define OVERWRITE     0x0
>> -        #define SECURE_ERASE  0x1
>> +        #define OVERWRITE        0x0
>> +        #define SECURE_ERASE     0x1
>> +        #define MEDIA_OPERATIONS 0x2
>
>Trivial but I've given up trying to keep these aligned.
>It's a fools game as the names get steadily longer.
Ok Will Fix in V2
>
>As such better to just leave the existing pair alone.
>
>>      PERSISTENT_MEM = 0x45,
>>          #define GET_SECURITY_STATE     0x0
>>      MEDIA_AND_POISON = 0x43,
>> @@ -1721,6 +1722,127 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_BG_STARTED;
>>  }
>>
>> +enum {
>> +    MEDIA_OP_GENERAL  = 0x0,
>I'd name them so the field id explicit.
>
>MEDIA_OP_CLASS_GENERAL
>etc

Ok will fix in V2
>
>> +    MEDIA_OP_SANITIZE = 0x1,
>> +    MEDIA_OP_CLASS_MAX,
>No comma on terminating entry. We don't want it to be easy to add
>stuff after it.
>
>> +} MEDIA_OPERATION_CLASS;
>The enum type is never used.  So might as well keep it anonymous
>like we do for other enums in this file.
>
>> +
>> +enum {
>> +    MEDIA_OP_SUB_DISCOVERY = 0x0,
>This set of class and subcalss is similar to the enum you add
>the MEDIA_OPERATIONS define to above.
>I'd take a similar strategy with
>
ok will fix
>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
>
>or something like that.
ok will fix 
>}
>> +    MEDIA_OP_SUB_SANITIZE = 0x0,
>> +    MEDIA_OP_SUB_ZERO     = 0x1,
>> +    MEDIA_OP_SUB_CLASS_MAX
>No need for SUB_CLASS_MAX as you don't seem to use it.
>
>> +} MEDIA_OPERATION_SUB_CLASS;
>> +
>> +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[0];
>entry[]
ok will change it in V2
>
>which is the c spec defined way to do variable length last elements.
>The [0] was I think a weird extension that we have moved away from.
>
>> +};
>
>Not strictly necessary but I'd mark it packed as chances of future breakage
>are high with a structure starting at byte 0xC.
>
ok will fix
>> +
>> +#define MAX_SUPPORTED_OPS 3
>I'd avoid explicit define for this and just use ARRAY_SIZE() on the
>array of structures to find out.
>
>> +struct media_op_supported_list_entry media_op_matrix[MAX_SUPPORTED_OPS] = {
>
>Use the defines above rather than the numeric values.
>Then it's obvious what this is, also mark it static const.
>
ok will fix 
>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 },
>};
>
>> +                                                            {0, 0},
>> +                                                            {1, 0},
>> +                                                            {1, 1} };
>> +
>> +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;
>    struct {
>        uint8_t media_operation_class;
>
>etc for alignment.
>
>> +    uint8_t media_operation_subclass;
>> +    uint8_t rsvd[2];
>> +    uint32_t dpa_range_count;
>> +    union {
>> +        struct {
>> +            uint64_t starting_dpa;
>> +            uint64_t length;
>> +        } dpa_range_list[0];
>[]
here its under union, compiler errors if not 
given the [0] number of element.
May be try some other way
>
>> +        struct {
>> +            uint16_t start_index;
>> +            uint16_t num_supported_ops;
>> +        } discovery_osa;
>> +    };
>
>This is a little tricky as in theory you can have a variable number
>of DPA Range List elements and then the operation specific arguments.
>
>However, general always provides a range count of 0.  Also both sanitize
>and zero have no osa elemetns.  Add a comment
>about this so we don't think it looks wrong in future + do notice that
>this approach doesn't generalize if a new operation allows dpa ranges
>and operation specific parameters.
>
Will add approriate comment
>
>> +    } QEMU_PACKED *media_op_in_pl = (void *)payload_in;
>> +
>> +    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;
>> +
>> +    if (len_in < sizeof(*media_op_in_pl)) {
>> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>> +    }
>
>Test this before getting values to fill in media_op_cl local variables etc.
>It's both logically correct and may constrain the compiler not to get too smart
>if it can see enough to realize what len_in is.
>
ok 
>> +
>> +    switch (media_op_cl) {
>> +    case MEDIA_OP_GENERAL:
>> +        switch (media_op_subclass) {
>> +        case MEDIA_OP_SUB_DISCOVERY:
>Given there is only one element, maybe cleaner as
>           if (media_op_subclass != MEDIA_OP_SUB_DISCOVERY) {
>                return CXL_MBOX_UNSUPPORTED;
>           }
>AS reduces indent of the following, helping readability a litle.
ok
>
>> +            int count = 0;
>> +            struct media_op_discovery_out_pl *media_out_pl =
>> +                (void *)payload_out;
>> +            int num_ops = media_op_in_pl->discovery_osa.num_supported_ops;
>> +            int start_index = media_op_in_pl->discovery_osa.start_index;
>> +
>> +            /* As per spec CXL 3.1 8.2.9.9.5.3 dpa_range_count */
>> +            /* should be zero for discovery sub class command */
>Local style is multiline comment as
>               /*
>                * As per spec CXL 3.1...
>                * should be zero...
>                */
>

ok
>> +            if (dpa_range_count) {
>> +                return CXL_MBOX_INVALID_INPUT;
>> +            }
>> +
>> +            if ((start_index >= MEDIA_OP_CLASS_MAX) ||
>> +                (num_ops > MAX_SUPPORTED_OPS)) {
>
>Check here should be for num_ops + start_index > MAX_SUPPORTED OPS
>Comparing start_index against MEDIA_OP_CLASS_MAX doesn't make sense to me
>as I believe it's an index into the array of Class / subclass pairs not
>the class array.
>

ok
>
>> +                return CXL_MBOX_INVALID_INPUT;
>> +            }
>> +
>> +            media_out_pl->dpa_range_granularity = CXL_CAPACITY_MULTIPLIER;
>> +            media_out_pl->total_supported_operations = MAX_SUPPORTED_OPS;
>> +            if (num_ops > 0) {
>> +                for (int i = start_index; i < MAX_SUPPORTED_OPS; 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) {
>> +                        goto disc_out;
>
>break should be enough and removes need for goto and label.
>

ok
>> +                    }
>> +                }
>> +            }
>> +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);
>
>indent this line.
>

ok
>> +            break;
>I'd
>        return CXL_MBOX_SUCCESS;
>
>> +        default:
>> +            return CXL_MBOX_UNSUPPORTED;
>> +        }
>> +        break;
>then this break isn't needed.

ok
>> +    case MEDIA_OP_SANITIZE:
>> +        switch (media_op_subclass) {
>> +
>No blank line here yet.
>> +        default:
>> +            return CXL_MBOX_UNSUPPORTED;
>> +        }
>Similar. Return in all paths so no break.

ok
>> +        break;
>> +    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 +2986,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",
>

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

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)

[Jonathan Cameron]

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"

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)

[Adam Manzanares]

On Fri, Jan 24, 2025 at 03:19:46PM +0000, Jonathan Cameron wrote:
> 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?
> 

See response below for disabling the media state. Same section referenced
below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome
depends on how we interpret "follow the method described in 8.2.10.9.5.1".

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

Table 8-164, sanitize description mentions to follow method
in 8.2.10.9.5.1, which does call out placing device in disabled
state, but I'm not sure if method refers to all text in 8.2.10.9.5.1
or the method devices uses to sanitize internally.

I would imagine since sanitize is destructive we would not want to return 
any data from device ranges impacted by sanitize. I believe a simple
way to achieve this is to disable entire device. 

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

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)

[Jonathan Cameron]

> >   
> > > +    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?
> >   
> 
> See response below for disabling the media state. Same section referenced
> below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome
> depends on how we interpret "follow the method described in 8.2.10.9.5.1".
> 

This also sounds like reading too much into that comment.

> > > +    }
> > > +
> > > +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.  
> 
> Table 8-164, sanitize description mentions to follow method
> in 8.2.10.9.5.1, which does call out placing device in disabled
> state, but I'm not sure if method refers to all text in 8.2.10.9.5.1
> or the method devices uses to sanitize internally.

I think it is meant to just be the method of sanitizing. 

> 
> I would imagine since sanitize is destructive we would not want to return 
> any data from device ranges impacted by sanitize. I believe a simple
> way to achieve this is to disable entire device. 

Hmm.  That rather destroys the main use case I'm aware of for this
(unlike the general santize commands from earlier CXL versions)/
Superficially sounds like we need a spec clarification as
clearly not super clear!

> 

Jonathan

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)

[Adam Manzanares]

On Mon, Feb 03, 2025 at 11:33:54AM +0000, Jonathan Cameron wrote:
> 
> > >   
> > > > +    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?
> > >   
> > 
> > See response below for disabling the media state. Same section referenced
> > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome
> > depends on how we interpret "follow the method described in 8.2.10.9.5.1".
> > 
> 
> This also sounds like reading too much into that comment.
>

Agreed, Vinayak let's drop the discard of all event records
from this patch.

> > > > +    }
> > > > +
> > > > +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.  
> > 
> > Table 8-164, sanitize description mentions to follow method
> > in 8.2.10.9.5.1, which does call out placing device in disabled
> > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1
> > or the method devices uses to sanitize internally.
> 
> I think it is meant to just be the method of sanitizing. 
> 

Ok, let's use this interpretation. Vinayak, can you remove this as well
and then we put a comment in the patch that media op sanitize is targeted 
so no need to disable media or clear event logs.


> > 
> > I would imagine since sanitize is destructive we would not want to return 
> > any data from device ranges impacted by sanitize. I believe a simple
> > way to achieve this is to disable entire device. 
> 
> Hmm.  That rather destroys the main use case I'm aware of for this
> (unlike the general santize commands from earlier CXL versions)/
> Superficially sounds like we need a spec clarification as
> clearly not super clear!
> 

For this series, let's drive the work with the use case you have in
mind. We will start a thread with the consortium, but I don't think
that should delay this work.

> > 
> 
> Jonathan
> 
> 

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)

[Vinayak Holikatti]

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

On 03/02/25 05:02PM, Adam Manzanares wrote:
>On Mon, Feb 03, 2025 at 11:33:54AM +0000, Jonathan Cameron wrote:
>>
>> > >
>> > > > +    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?
>> > >
>> >
>> > See response below for disabling the media state. Same section referenced
>> > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome
>> > depends on how we interpret "follow the method described in 8.2.10.9.5.1".
>> >
>>
>> This also sounds like reading too much into that comment.
>>
>
>Agreed, Vinayak let's drop the discard of all event records
>from this patch.

Ok Adam will drop the discard of all event records

>
>> > > > +    }
>> > > > +
>> > > > +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.
>> >
>> > Table 8-164, sanitize description mentions to follow method
>> > in 8.2.10.9.5.1, which does call out placing device in disabled
>> > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1
>> > or the method devices uses to sanitize internally.
>>
>> I think it is meant to just be the method of sanitizing.
>>
>
>Ok, let's use this interpretation. Vinayak, can you remove this as well
>and then we put a comment in the patch that media op sanitize is targeted
>so no need to disable media or clear event logs.

ok Adam, will remove this as well and comment as needed

>
>
>> >
>> > I would imagine since sanitize is destructive we would not want to return
>> > any data from device ranges impacted by sanitize. I believe a simple
>> > way to achieve this is to disable entire device.
>>
>> Hmm.  That rather destroys the main use case I'm aware of for this
>> (unlike the general santize commands from earlier CXL versions)/
>> Superficially sounds like we need a spec clarification as
>> clearly not super clear!
>>
>
>For this series, let's drive the work with the use case you have in
>mind. We will start a thread with the consortium, but I don't think
>that should delay this work.
>
>> >
>>
>> Jonathan
>>
>>
Vinayak Holikatti


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

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)

[Vinayak Holikatti]

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

On 24/01/25 03:19PM, Jonathan Cameron wrote:
>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.

Didnt get what you meant, validate_dpa_addr is meant for 
checking valid dpa address and sanitize_range is 
get the address space handle to do actual sanitize
of dpa address, so two are different purposes, 

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

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

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)

[Jonathan Cameron]

> >> +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.  
> 
> Didnt get what you meant, validate_dpa_addr is meant for 
> checking valid dpa address and sanitize_range is 
> get the address space handle to do actual sanitize
> of dpa address, so two are different purposes, 

Different purposes but the actual checks mostly
overlap.  If we can find the address space then there
are only a few extra checks on alignment with granual
size etc needed.  I'd just like to avoid that duplication
so factor out the shared code to a helper called in both
functions.


> 
> >  
> >> +
> >> +    return  address_space_set(as, dpa_addr,