* [qemu PATCH 0/2] cxl: Handle GSL Sub-List @ 2023-07-28 16:57 Davidlohr Bueso 2023-07-28 16:57 ` [PATCH 1/2] hw/cxl: Update comments for Get Log Davidlohr Bueso 2023-07-28 16:57 ` [PATCH 2/2] hw/cxl: Add the Get Supported Logs Sub-List cmd Davidlohr Bueso 0 siblings, 2 replies; 5+ messages in thread From: Davidlohr Bueso @ 2023-07-28 16:57 UTC (permalink / raw) To: jonathan.cameron; +Cc: dan.j.williams, fan.ni, a.manzanares, dave, linux-cxl Hi, The following is an initial implementation for the GSL sublist command, which came up while reviewing some of the switch cci stuff in the kernel. I have not yet finished the kernel side of this (for which sublist should be a first attempt then fallback to full gsl upon error/no support), but wanted to get the qemu side out there at least. Regular gsl remains unchanged. Applies on top of Jonathan's tree: https://gitlab.com/jic23/qemu/-/tree/cxl-2023-07-17 Thanks! Davidlohr Bueso (2): hw/cxl: Update comments for Get Log hw/cxl: Add the Get Supported Logs Sub-List cmd hw/cxl/cxl-mailbox-utils.c | 111 ++++++++++++++++++++++++++++++++----- 1 file changed, 96 insertions(+), 15 deletions(-) -- 2.41.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] hw/cxl: Update comments for Get Log 2023-07-28 16:57 [qemu PATCH 0/2] cxl: Handle GSL Sub-List Davidlohr Bueso @ 2023-07-28 16:57 ` Davidlohr Bueso 2023-08-04 13:29 ` Jonathan Cameron 2023-07-28 16:57 ` [PATCH 2/2] hw/cxl: Add the Get Supported Logs Sub-List cmd Davidlohr Bueso 1 sibling, 1 reply; 5+ messages in thread From: Davidlohr Bueso @ 2023-07-28 16:57 UTC (permalink / raw) To: jonathan.cameron; +Cc: dan.j.williams, fan.ni, a.manzanares, dave, linux-cxl The current notes regarding some of the literature in the spec no longer applies, update from 2.0 to 3.0. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- hw/cxl/cxl-mailbox-utils.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 2819914e8d09..5152a83c6fdd 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -608,7 +608,7 @@ static const QemuUUID cel_uuid = { 0x96, 0xb1, 0x62, 0x3b, 0x3f, 0x17) }; -/* 8.2.9.4.1 */ +/* CXL 3.0 8.2.9.5.1 */ static CXLRetCode cmd_logs_get_supported(const struct cxl_cmd *cmd, uint8_t *payload_in, size_t len_in, @@ -634,7 +634,7 @@ static CXLRetCode cmd_logs_get_supported(const struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } -/* 8.2.9.4.2 */ +/* CXL 3.0 8.2.9.5.2 */ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, uint8_t *payload_in, size_t len_in, @@ -651,12 +651,6 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, get_log = (void *)payload_in; /* - * 8.2.9.4.2 - * The device shall return Invalid Parameter if the Offset or Length - * fields attempt to access beyond the size of the log as reported by Get - * Supported Logs. - * - * XXX: Spec is wrong, "Invalid Parameter" isn't a thing. * XXX: Spec doesn't address incorrect UUID incorrectness. * * The CEL buffer is large enough to fit all commands in the emulation, so -- 2.41.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hw/cxl: Update comments for Get Log 2023-07-28 16:57 ` [PATCH 1/2] hw/cxl: Update comments for Get Log Davidlohr Bueso @ 2023-08-04 13:29 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2023-08-04 13:29 UTC (permalink / raw) To: Davidlohr Bueso; +Cc: dan.j.williams, fan.ni, a.manzanares, linux-cxl On Fri, 28 Jul 2023 09:57:04 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > The current notes regarding some of the literature in the spec > no longer applies, update from 2.0 to 3.0. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> Hi Davidlohr, Thanks for tidying this bit up. We aren't doing great on consistency of how we list the version numbers in the QEMU code, but I'd like to definitely make it easier to tell what is revision and what is section. So I've tweak your changes here to the format /* CXL r3.0 section xxxxxx: Section title */ and put it on top of my tree (I'll push out once I've finished queuing anything else that is ready up on top. Thanks, Jonathan > --- > hw/cxl/cxl-mailbox-utils.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index 2819914e8d09..5152a83c6fdd 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -608,7 +608,7 @@ static const QemuUUID cel_uuid = { > 0x96, 0xb1, 0x62, 0x3b, 0x3f, 0x17) > }; > > -/* 8.2.9.4.1 */ > +/* CXL 3.0 8.2.9.5.1 */ > static CXLRetCode cmd_logs_get_supported(const struct cxl_cmd *cmd, > uint8_t *payload_in, > size_t len_in, > @@ -634,7 +634,7 @@ static CXLRetCode cmd_logs_get_supported(const struct cxl_cmd *cmd, > return CXL_MBOX_SUCCESS; > } > > -/* 8.2.9.4.2 */ > +/* CXL 3.0 8.2.9.5.2 */ > static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, > uint8_t *payload_in, > size_t len_in, > @@ -651,12 +651,6 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, > get_log = (void *)payload_in; > > /* > - * 8.2.9.4.2 > - * The device shall return Invalid Parameter if the Offset or Length > - * fields attempt to access beyond the size of the log as reported by Get > - * Supported Logs. > - * > - * XXX: Spec is wrong, "Invalid Parameter" isn't a thing. > * XXX: Spec doesn't address incorrect UUID incorrectness. > * > * The CEL buffer is large enough to fit all commands in the emulation, so ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] hw/cxl: Add the Get Supported Logs Sub-List cmd 2023-07-28 16:57 [qemu PATCH 0/2] cxl: Handle GSL Sub-List Davidlohr Bueso 2023-07-28 16:57 ` [PATCH 1/2] hw/cxl: Update comments for Get Log Davidlohr Bueso @ 2023-07-28 16:57 ` Davidlohr Bueso 2023-08-04 13:55 ` Jonathan Cameron 1 sibling, 1 reply; 5+ messages in thread From: Davidlohr Bueso @ 2023-07-28 16:57 UTC (permalink / raw) To: jonathan.cameron; +Cc: dan.j.williams, fan.ni, a.manzanares, dave, linux-cxl 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(-) 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", -- 2.41.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hw/cxl: Add the Get Supported Logs Sub-List cmd 2023-07-28 16:57 ` [PATCH 2/2] hw/cxl: Add the Get Supported Logs Sub-List cmd Davidlohr Bueso @ 2023-08-04 13:55 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2023-08-04 13:55 UTC (permalink / raw) To: Davidlohr Bueso; +Cc: dan.j.williams, fan.ni, a.manzanares, linux-cxl 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; > +} > + ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-04 13:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-28 16:57 [qemu PATCH 0/2] cxl: Handle GSL Sub-List Davidlohr Bueso 2023-07-28 16:57 ` [PATCH 1/2] hw/cxl: Update comments for Get Log Davidlohr Bueso 2023-08-04 13:29 ` Jonathan Cameron 2023-07-28 16:57 ` [PATCH 2/2] hw/cxl: Add the Get Supported Logs Sub-List cmd Davidlohr Bueso 2023-08-04 13:55 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox