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