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

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

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