| Message ID | 20230728165705.5889-3-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Handle GSL Sub-List | expand |
On Fri, 28 Jul 2023 09:57:05 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > The spec is quite clear that system software should try using > this instead of the traditional GSL - albeit both qemu and driver > only have CEL. In addition make the already existing commands a > bit more generic for any addition of future logs. > > As noted in the code, the spec is also not explicit about all > input scan range errors - for which qemu can return 0 entries but > still set the total number of entries available. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> Hi Davidlohr Comments inline. I think we should look at implementing some other randomly chosen log so as to be able to test the loops etc in here. I guess could be either the really generic vendor debug log, or the slightly more specific component state dump log. Or do them both and return suitable a text quote of your choosing as the debug information for now. Jonathan > --- > hw/cxl/cxl-mailbox-utils.c | 101 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 94 insertions(+), 7 deletions(-) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index 5152a83c6fdd..0110797b7b52 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -64,6 +64,7 @@ enum { > LOGS = 0x04, > #define GET_SUPPORTED 0x0 > #define GET_LOG 0x1 > + #define GET_SUPPORTED_SUBLIST 0x5 > IDENTIFY = 0x40, > #define MEMORY_DEVICE 0x0 > CCLS = 0x41, > @@ -602,6 +603,11 @@ static CXLRetCode cmd_timestamp_set(const struct cxl_cmd *cmd, > return CXL_MBOX_SUCCESS; > } > > +enum { > + CXL_LOGS_CEL, > + CXL_MAX_SUPPORTED_LOGS, > +}; > + > /* CXL 3.0 8.2.9.5.2.1 Command Effects Log (CEL) */ > static const QemuUUID cel_uuid = { > .data = UUID(0x0da9c0b5, 0xbf41, 0x4b78, 0x8f, 0x79, > @@ -616,20 +622,28 @@ static CXLRetCode cmd_logs_get_supported(const struct cxl_cmd *cmd, > size_t *len_out, > CXLCCI *cci) > { > + uint16_t i; > struct { > uint16_t entries; > uint8_t rsvd[6]; > struct { > QemuUUID uuid; > uint32_t size; > - } log_entries[1]; > + } log_entries[CXL_MAX_SUPPORTED_LOGS]; > } QEMU_PACKED *supported_logs = (void *)payload_out; > QEMU_BUILD_BUG_ON(sizeof(*supported_logs) != 0x1c); > > - supported_logs->entries = 1; > - supported_logs->log_entries[0].uuid = cel_uuid; > - supported_logs->log_entries[0].size = 4 * cci->cel_size; > - > + supported_logs->entries = CXL_MAX_SUPPORTED_LOGS; > + for (i = 0; i < CXL_MAX_SUPPORTED_LOGS; i++) { /* all */ > + switch (i) { > + case CXL_LOGS_CEL: > + supported_logs->log_entries[i].uuid = cel_uuid; > + supported_logs->log_entries[i].size = 4 * cci->cel_size; > + break; > + default: > + break; > + } > + } Arguably premature flexibility given it's a loop that always goes around once only. > *len_out = sizeof(*supported_logs); > return CXL_MBOX_SUCCESS; > } > @@ -642,6 +656,8 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, > size_t *len_out, > CXLCCI *cci) > { > + uint8_t i; > + bool found = false; > struct { > QemuUUID uuid; > uint32_t offset; > @@ -661,8 +677,15 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, > return CXL_MBOX_INVALID_INPUT; > } > > - if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) { > - return CXL_MBOX_UNSUPPORTED; > + for (i = 0; i < CXL_MAX_SUPPORTED_LOGS; i++) { /* all */ > + if (i == CXL_LOGS_CEL && > + qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) { > + found = true; > + break; > + } > + } > + if (!found) { > + return CXL_MBOX_UNSUPPORTED; > } > > /* Store off everything to local variables so we can wipe out the payload */ > @@ -673,6 +696,66 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, > return CXL_MBOX_SUCCESS; > } > > +/* CXL r3.0 8.2.9.5.6 */ Please update as per previous patch comment. > +static CXLRetCode cmd_logs_get_supported_sublist(const struct cxl_cmd *cmd, > + uint8_t *payload_in, > + size_t len_in, > + uint8_t *payload_out, > + size_t *len_out, > + CXLCCI *cci) > +{ > + uint8_t i, entries, start; > + struct inject_poison_pl { ? :) > + uint8_t max_entries; > + uint8_t start_idx; > + } QEMU_PACKED *in = (void *)payload_in; > + struct { > + uint8_t entries; > + uint8_t rsvd1; > + uint16_t total_entries; > + uint8_t start_idx; > + uint8_t rsvd2[3]; > + struct { > + QemuUUID uuid; > + uint32_t size; > + } log_entries[CXL_MAX_SUPPORTED_LOGS]; > + } QEMU_PACKED *supported_logs = (void *)payload_out; > + QEMU_BUILD_BUG_ON(sizeof(*supported_logs) != 0x1c); Why the size check (which will break the moment the number of logs changes?) Those don't really make sense for variable length replies. > + > + if (in->max_entries < 1) { > + return CXL_MBOX_INVALID_INPUT; > + } > + /* > + * XXX: Handle other bogus input by returning zero entries but > + * setting the total_entries such that software user can > + * get it right next time(?) CXL spec mentions nothing about > + * handling this. In my view, software that doesn't first issue in->start_idx == 0 has shot itself in the foot. Return CXL_MBOX_INVALID_INPUT. I don't think ti's valid to return start = 0... However I agree there may be a space for a spec clarification. > + */ > + if (in->start_idx > CXL_MAX_SUPPORTED_LOGS - 1) { > + start = 0; > + entries = 0; > + } else { > + start = in->start_idx; > + entries = MIN(CXL_MAX_SUPPORTED_LOGS - start, in->max_entries); > + } > + supported_logs->entries = entries; > + supported_logs->total_entries = CXL_MAX_SUPPORTED_LOGS; > + supported_logs->start_idx = start; > + for (i = start; i < entries; i++) { > + switch (i) { > + case CXL_LOGS_CEL: > + supported_logs->log_entries[i].uuid = cel_uuid; Need a second index counting in log_entries as that needs to always start at 0. Also, I think we should move to an array of data structures containing the uuids and sizes for each log type so the switch statement here goes away as does the if stuff above. > + supported_logs->log_entries[i].size = 4 * cci->cel_size; > + break; > + default: > + break; > + } > + } > + > + *len_out = sizeof(*supported_logs); > + return CXL_MBOX_SUCCESS; > +} > +
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 5152a83c6fdd..0110797b7b52 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -64,6 +64,7 @@ enum { LOGS = 0x04, #define GET_SUPPORTED 0x0 #define GET_LOG 0x1 + #define GET_SUPPORTED_SUBLIST 0x5 IDENTIFY = 0x40, #define MEMORY_DEVICE 0x0 CCLS = 0x41, @@ -602,6 +603,11 @@ static CXLRetCode cmd_timestamp_set(const struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +enum { + CXL_LOGS_CEL, + CXL_MAX_SUPPORTED_LOGS, +}; + /* CXL 3.0 8.2.9.5.2.1 Command Effects Log (CEL) */ static const QemuUUID cel_uuid = { .data = UUID(0x0da9c0b5, 0xbf41, 0x4b78, 0x8f, 0x79, @@ -616,20 +622,28 @@ static CXLRetCode cmd_logs_get_supported(const struct cxl_cmd *cmd, size_t *len_out, CXLCCI *cci) { + uint16_t i; struct { uint16_t entries; uint8_t rsvd[6]; struct { QemuUUID uuid; uint32_t size; - } log_entries[1]; + } log_entries[CXL_MAX_SUPPORTED_LOGS]; } QEMU_PACKED *supported_logs = (void *)payload_out; QEMU_BUILD_BUG_ON(sizeof(*supported_logs) != 0x1c); - supported_logs->entries = 1; - supported_logs->log_entries[0].uuid = cel_uuid; - supported_logs->log_entries[0].size = 4 * cci->cel_size; - + supported_logs->entries = CXL_MAX_SUPPORTED_LOGS; + for (i = 0; i < CXL_MAX_SUPPORTED_LOGS; i++) { /* all */ + switch (i) { + case CXL_LOGS_CEL: + supported_logs->log_entries[i].uuid = cel_uuid; + supported_logs->log_entries[i].size = 4 * cci->cel_size; + break; + default: + break; + } + } *len_out = sizeof(*supported_logs); return CXL_MBOX_SUCCESS; } @@ -642,6 +656,8 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, size_t *len_out, CXLCCI *cci) { + uint8_t i; + bool found = false; struct { QemuUUID uuid; uint32_t offset; @@ -661,8 +677,15 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, return CXL_MBOX_INVALID_INPUT; } - if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) { - return CXL_MBOX_UNSUPPORTED; + for (i = 0; i < CXL_MAX_SUPPORTED_LOGS; i++) { /* all */ + if (i == CXL_LOGS_CEL && + qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) { + found = true; + break; + } + } + if (!found) { + return CXL_MBOX_UNSUPPORTED; } /* Store off everything to local variables so we can wipe out the payload */ @@ -673,6 +696,66 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +/* CXL r3.0 8.2.9.5.6 */ +static CXLRetCode cmd_logs_get_supported_sublist(const struct cxl_cmd *cmd, + uint8_t *payload_in, + size_t len_in, + uint8_t *payload_out, + size_t *len_out, + CXLCCI *cci) +{ + uint8_t i, entries, start; + struct inject_poison_pl { + uint8_t max_entries; + uint8_t start_idx; + } QEMU_PACKED *in = (void *)payload_in; + struct { + uint8_t entries; + uint8_t rsvd1; + uint16_t total_entries; + uint8_t start_idx; + uint8_t rsvd2[3]; + struct { + QemuUUID uuid; + uint32_t size; + } log_entries[CXL_MAX_SUPPORTED_LOGS]; + } QEMU_PACKED *supported_logs = (void *)payload_out; + QEMU_BUILD_BUG_ON(sizeof(*supported_logs) != 0x1c); + + if (in->max_entries < 1) { + return CXL_MBOX_INVALID_INPUT; + } + /* + * XXX: Handle other bogus input by returning zero entries but + * setting the total_entries such that software user can + * get it right next time(?) CXL spec mentions nothing about + * handling this. + */ + if (in->start_idx > CXL_MAX_SUPPORTED_LOGS - 1) { + start = 0; + entries = 0; + } else { + start = in->start_idx; + entries = MIN(CXL_MAX_SUPPORTED_LOGS - start, in->max_entries); + } + supported_logs->entries = entries; + supported_logs->total_entries = CXL_MAX_SUPPORTED_LOGS; + supported_logs->start_idx = start; + for (i = start; i < entries; i++) { + switch (i) { + case CXL_LOGS_CEL: + supported_logs->log_entries[i].uuid = cel_uuid; + supported_logs->log_entries[i].size = 4 * cci->cel_size; + break; + default: + break; + } + } + + *len_out = sizeof(*supported_logs); + return CXL_MBOX_SUCCESS; +} + /* 8.2.9.5.1.1 */ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd, uint8_t *payload_in, @@ -1172,6 +1255,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = { [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, IMMEDIATE_POLICY_CHANGE }, [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 }, [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 }, + [LOGS][GET_SUPPORTED_SUBLIST] = { "LOGS_GET_SUPPORTED_SUBLIST", + cmd_logs_get_supported_sublist, 0x02, 0 }, [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE", cmd_identify_memory_device, 0, 0 }, [CCLS][GET_PARTITION_INFO] = { "CCLS_GET_PARTITION_INFO", @@ -1203,6 +1288,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = { [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, IMMEDIATE_POLICY_CHANGE }, [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 }, [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 }, + [LOGS][GET_SUPPORTED_SUBLIST] = { "LOGS_GET_SUPPORTED_SUBLIST", + cmd_logs_get_supported_sublist, 0x02, 0 }, [PHYSICAL_SWITCH][IDENTIFY_SWITCH_DEVICE] = {"IDENTIFY_SWITCH_DEVICE", cmd_identify_switch_device, 0, 0x49 }, [PHYSICAL_SWITCH][GET_PHYSICAL_PORT_STATE] = { "SWITCH_PHYSICAL_PORT_STATS",
The spec is quite clear that system software should try using this instead of the traditional GSL - albeit both qemu and driver only have CEL. In addition make the already existing commands a bit more generic for any addition of future logs. As noted in the code, the spec is also not explicit about all input scan range errors - for which qemu can return 0 entries but still set the total number of entries available. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- hw/cxl/cxl-mailbox-utils.c | 101 ++++++++++++++++++++++++++++++++++--- 1 file changed, 94 insertions(+), 7 deletions(-)