Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)
> >>
> >> +static const struct CXLLogCapabilities cxl_get_log_cap[MAX_LOG_TYPE] = {
> >> +[CEL] = {.param_flags.pflags = CXL_LOG_CAP_CLEAR |
> >> CXL_LOG_CAP_POPULATE,
> >> + .uuid = cel_uuid},
> >> +};
> >> +
> >> +static void cxl_init_get_log(CXLCCI *cci)
> >> +{
> >> +for (int set = CEL; set < MAX_LOG_TYPE; set++) {
> >> +cci->supported_log_cap[set] = cxl_get_log_cap[set];
> >
> >As below. Can we just use a static const pointer in cci?
>
> static const pointer in cci gives compilation error as it used below
> to assign value:
> cci->supported_log_cap[set] = cxl_get_log_cap[set];
True. That is what I am suggesting changing.
cci->supported_log_cap = cxl_get_log_cap;
This is currently iterating over a list and copying everything. Why
bother, just set a pointer to the source list. If there
are choices for that, pick between them but keep all those lists const.
>
> >> +typedef struct CXLLogCapabilities {
> >> +CXLParamFlags param_flags;
> >> +QemuUUID uuid;
> >> +} CXLLogCapabilities;
> >> +
> >> typedef struct CXLCCI {
> >> struct cxl_cmd cxl_cmd_set[256][256];
> >> struct cel_log {
> >> @@ -202,6 +230,9 @@ typedef struct CXLCCI {
> >> } cel_log[1 << 16];
> >> size_t cel_size;
> >>
> >> +/* get log capabilities */
> >> +CXLLogCapabilities supported_log_cap[MAX_LOG_TYPE];
> >Perhaps a const pointer is appropriate?
>
> const pointer here gives compilation error as it is used below
> to assign value:
> cci->supported_log_cap[set] = cxl_get_log_cap[set];
As above. This set is the thing I'm suggesting changing.
One general review process thing is it is much more productive
to just crop anything you agree with and just have the discussion
focused on questions etc that are outstanding.
Jonathan
Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)
On 04/02/25 10:28AM, Jonathan Cameron wrote:
On Mon, 3 Feb 2025 11:29:48 +0530
Arpit Kumar wrote:
Please add some descriptive teext here.
Sure, will append here in V2 patch.
Signed-off-by: Arpit Kumar
Reviewed-by: Alok Rathore
Reviewed-by: Krishna Kanth Reddy
Hi Arpit,
Whilst it is good to do internal reviews, I'd prefer to see feedback
on the mailing list if possible. Hard for a maintainer to tell
what a RB tag given before posting means unfortunately so in most
cases preference is to not add RB tags based on internal review.
If your colleagues have greatly affected the code, a
Co-developed-by: and additional sign off may be a good way to
reflect that.
Great to have you working on these features. Some comments inline.
Main ones are around naming (always tricky!) and suggestion to
handle the arrays of log capabilities as static const pointers.
Thanks,
Jonathan
Thanks for the review Jonathan, will make changes accordingly in the
next iteration-V2 of the patch.
---
hw/cxl/cxl-mailbox-utils.c | 55
include/hw/cxl/cxl_device.h | 31
include/hw/cxl/cxl_mailbox.h | 5
3 files changed, 91 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 9c7ea5bc35..3d66a425a9 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -76,6 +76,7 @@ enum {
LOGS= 0x04,
#define GET_SUPPORTED 0x0
#define GET_LOG 0x1
+#define GET_LOG_CAPABILITIES 0x2
FEATURES= 0x05,
#define GET_SUPPORTED 0x0
#define GET_FEATURE 0x1
@@ -1075,6 +1076,45 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd
*cmd,
return CXL_MBOX_SUCCESS;
}
+static int32_t valid_log_check(QemuUUID *uuid, CXLCCI *cci)
Perhaps find_log_index() or something like that?
I would return &ccx->supported_log_cap[i] / NULL if not found
as then can avoid long reference into array below.
okay, got it.
+{
+int32_t id = -1;
+for (int i = CEL; i < MAX_LOG_TYPE; i++) {
+if (qemu_uuid_is_equal(uuid,
+&cci->supported_log_cap[i].uuid)) {
+id = i;
+break;
+}
+}
+return id;
+}
+
+/* CXL r3.1 Section 8.2.9.5.3: Get Log Capabilities (Opcode 0402h) */
For new documentation please use latest spec.
This is a bit different to many other spec where the request is the
earliest one. That is due to the consortium only making the latest
version available (currently r3.2)
okay
+static CXLRetCode cmd_logs_get_log_capabilities(const struct cxl_cmd *cmd,
+uint8_t *payload_in,
+size_t len_in,
+uint8_t *payload_out,
+size_t *len_out,
+CXLCCI *cci)
+{
+int32_t cap_id;
+struct {
+QemuUUID uuid;
+} QEMU_PACKED QEMU_ALIGNED(8) * get_log_capabilities_in = (void
*)payload_in;
+
+CXLParamFlags *get_log_capabilities_out = (void *)payload_out;
+
+cap_id = valid_log_check(&get_log_capabilities_in->uuid, cci);
CXLLogCapabilities *cap;
cap = find_log_cap(&get_log_capabilities_in->uuid, cci);
if (!cap) {
return CXL_MBOX_INVALID_LOG);
}
mempcy(get_log_capabilities_out, cap->param_flags.pflags,
sizeof(cap->param_flags.pflags));
...
got it.
+if (cap_id == -1) {
+return CXL_MBOX_INVALID_LOG;
+}
+
+memcpy(get_log_capabilities_out,
&cci->supported_log_cap[cap_id].param_flags.pflags,
+ sizeof(CXLParamFlags));
+*len_out = sizeof(*get_log_capabilities_out);
+return CXL_MBOX_SUCCESS;
+}
+
/* CXL r3.1 section 8.2.9.6: Features */
/*
* Get Supported Features output payload
@@ -2840,6 +2880,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
[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_LOG_CAPABILITIES] = { "LOGS_GET_LOG_CAPABILITIES",
+ cmd_logs_get_log_capabilities, 0x10, 0 },
[FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
cmd_features_get_supported, 0x8, 0 },
[FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
@@ -3084,10 +3126,23 @@ static void cxl_rebuild_cel(CXLCCI *cci)
}
}
+static const struct CXLLogCapabilities cxl_get_log_cap[MAX_LOG_TYPE] = {
+[CEL] = {.param_flags.pflags = CXL_LOG_CAP_CLEAR | CXL_LOG_CAP_POPULATE,
+ .uuid = cel_uuid},
+};
+
+static void cxl_init_get_log(CXLCCI *cci)
+{
+for (int set = CEL; set < MAX_LOG_TYPE; set++) {
+cci->supported_log_cap[set] = cxl_get_log_cap[set];
As below. Can we just use a static const pointer in cci?
static const pointer in cci gives
Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)
On Mon, 3 Feb 2025 11:29:48 +0530
Arpit Kumar wrote:
Please add some descriptive teext here.
> Signed-off-by: Arpit Kumar
> Reviewed-by: Alok Rathore
> Reviewed-by: Krishna Kanth Reddy
Hi Arpit,
Whilst it is good to do internal reviews, I'd prefer to see feedback
on the mailing list if possible. Hard for a maintainer to tell
what a RB tag given before posting means unfortunately so in most
cases preference is to not add RB tags based on internal review.
If your colleagues have greatly affected the code, a
Co-developed-by: and additional sign off may be a good way to
reflect that.
Great to have you working on these features. Some comments inline.
Main ones are around naming (always tricky!) and suggestion to
handle the arrays of log capabilities as static const pointers.
Thanks,
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 55
> include/hw/cxl/cxl_device.h | 31
> include/hw/cxl/cxl_mailbox.h | 5
> 3 files changed, 91 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 9c7ea5bc35..3d66a425a9 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -76,6 +76,7 @@ enum {
> LOGS= 0x04,
> #define GET_SUPPORTED 0x0
> #define GET_LOG 0x1
> +#define GET_LOG_CAPABILITIES 0x2
> FEATURES= 0x05,
> #define GET_SUPPORTED 0x0
> #define GET_FEATURE 0x1
> @@ -1075,6 +1076,45 @@ static CXLRetCode cmd_logs_get_log(const struct
> cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static int32_t valid_log_check(QemuUUID *uuid, CXLCCI *cci)
Perhaps find_log_index() or something like that?
I would return &ccx->supported_log_cap[i] / NULL if not found
as then can avoid long reference into array below.
> +{
> +int32_t id = -1;
> +for (int i = CEL; i < MAX_LOG_TYPE; i++) {
> +if (qemu_uuid_is_equal(uuid,
> +&cci->supported_log_cap[i].uuid)) {
> +id = i;
> +break;
> +}
> +}
> +return id;
> +}
> +
> +/* CXL r3.1 Section 8.2.9.5.3: Get Log Capabilities (Opcode 0402h) */
For new documentation please use latest spec.
This is a bit different to many other spec where the request is the
earliest one. That is due to the consortium only making the latest
version available (currently r3.2)
> +static CXLRetCode cmd_logs_get_log_capabilities(const struct cxl_cmd *cmd,
> +uint8_t *payload_in,
> +size_t len_in,
> +uint8_t *payload_out,
> +size_t *len_out,
> +CXLCCI *cci)
> +{
> +int32_t cap_id;
> +struct {
> +QemuUUID uuid;
> +} QEMU_PACKED QEMU_ALIGNED(8) * get_log_capabilities_in = (void
> *)payload_in;
> +
> +CXLParamFlags *get_log_capabilities_out = (void *)payload_out;
> +
> +cap_id = valid_log_check(&get_log_capabilities_in->uuid, cci);
CXLLogCapabilities *cap;
cap = find_log_cap(&get_log_capabilities_in->uuid, cci);
if (!cap) {
return CXL_MBOX_INVALID_LOG);
}
mempcy(get_log_capabilities_out, cap->param_flags.pflags,
sizeof(cap->param_flags.pflags));
...
> +if (cap_id == -1) {
> +return CXL_MBOX_INVALID_LOG;
> +}
> +
> +memcpy(get_log_capabilities_out,
> &cci->supported_log_cap[cap_id].param_flags.pflags,
> + sizeof(CXLParamFlags));
> +*len_out = sizeof(*get_log_capabilities_out);
> +return CXL_MBOX_SUCCESS;
> +}
> +
> /* CXL r3.1 section 8.2.9.6: Features */
> /*
> * Get Supported Features output payload
> @@ -2840,6 +2880,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [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_LOG_CAPABILITIES] = { "LOGS_GET_LOG_CAPABILITIES",
> + cmd_logs_get_log_capabilities, 0x10, 0
> },
> [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
>cmd_features_get_supported, 0x8, 0 },
> [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
> @@ -3084,10 +3126,23 @@ static void cxl_rebuild_cel(CXLCCI *cci)
> }
> }
>
> +static const struct CXLLogCapabilities cxl_get_log_cap[MAX_LOG_TYPE] = {
> +[CEL] = {.param_flags.pflags = CXL_LOG_CAP_CLEAR | CXL_LOG_CAP_POPULATE,
> + .uuid = cel_uuid},
> +};
> +
> +static void cxl_init_get_log(CXLCCI *cci)
> +{
> +for (int set = CEL; set < MAX_LOG_TYPE; set++) {
> +cci->supported_log_cap[set] = cxl_get_log_cap[set];
As below. Can we just use a static const pointer in cci?
Seems relatively unlikely we'll have lots of different log combinations
d
[PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)
Signed-off-by: Arpit Kumar
Reviewed-by: Alok Rathore
Reviewed-by: Krishna Kanth Reddy
---
hw/cxl/cxl-mailbox-utils.c | 55
include/hw/cxl/cxl_device.h | 31
include/hw/cxl/cxl_mailbox.h | 5
3 files changed, 91 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 9c7ea5bc35..3d66a425a9 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -76,6 +76,7 @@ enum {
LOGS= 0x04,
#define GET_SUPPORTED 0x0
#define GET_LOG 0x1
+#define GET_LOG_CAPABILITIES 0x2
FEATURES= 0x05,
#define GET_SUPPORTED 0x0
#define GET_FEATURE 0x1
@@ -1075,6 +1076,45 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd
*cmd,
return CXL_MBOX_SUCCESS;
}
+static int32_t valid_log_check(QemuUUID *uuid, CXLCCI *cci)
+{
+int32_t id = -1;
+for (int i = CEL; i < MAX_LOG_TYPE; i++) {
+if (qemu_uuid_is_equal(uuid,
+&cci->supported_log_cap[i].uuid)) {
+id = i;
+break;
+}
+}
+return id;
+}
+
+/* CXL r3.1 Section 8.2.9.5.3: Get Log Capabilities (Opcode 0402h) */
+static CXLRetCode cmd_logs_get_log_capabilities(const struct cxl_cmd *cmd,
+uint8_t *payload_in,
+size_t len_in,
+uint8_t *payload_out,
+size_t *len_out,
+CXLCCI *cci)
+{
+int32_t cap_id;
+struct {
+QemuUUID uuid;
+} QEMU_PACKED QEMU_ALIGNED(8) * get_log_capabilities_in = (void
*)payload_in;
+
+CXLParamFlags *get_log_capabilities_out = (void *)payload_out;
+
+cap_id = valid_log_check(&get_log_capabilities_in->uuid, cci);
+if (cap_id == -1) {
+return CXL_MBOX_INVALID_LOG;
+}
+
+memcpy(get_log_capabilities_out,
&cci->supported_log_cap[cap_id].param_flags.pflags,
+ sizeof(CXLParamFlags));
+*len_out = sizeof(*get_log_capabilities_out);
+return CXL_MBOX_SUCCESS;
+}
+
/* CXL r3.1 section 8.2.9.6: Features */
/*
* Get Supported Features output payload
@@ -2840,6 +2880,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
[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_LOG_CAPABILITIES] = { "LOGS_GET_LOG_CAPABILITIES",
+ cmd_logs_get_log_capabilities, 0x10, 0 },
[FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
cmd_features_get_supported, 0x8, 0 },
[FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
@@ -3084,10 +3126,23 @@ static void cxl_rebuild_cel(CXLCCI *cci)
}
}
+static const struct CXLLogCapabilities cxl_get_log_cap[MAX_LOG_TYPE] = {
+[CEL] = {.param_flags.pflags = CXL_LOG_CAP_CLEAR | CXL_LOG_CAP_POPULATE,
+ .uuid = cel_uuid},
+};
+
+static void cxl_init_get_log(CXLCCI *cci)
+{
+for (int set = CEL; set < MAX_LOG_TYPE; set++) {
+cci->supported_log_cap[set] = cxl_get_log_cap[set];
+}
+}
+
void cxl_init_cci(CXLCCI *cci, size_t payload_max)
{
cci->payload_max = payload_max;
cxl_rebuild_cel(cci);
+cxl_init_get_log(cci);
cci->bg.complete_pct = 0;
cci->bg.starttime = 0;
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index a64739be25..e7cb99a1d2 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -164,6 +164,18 @@ typedef enum {
CXL_MBOX_MAX = 0x20
} CXLRetCode;
+/* types of logs */
+enum Logs {
+CEL,
+VDL,
+CSDL,
+ECSL,
+MTCL,
+MTRSL,
+MTRLL,
+MAX_LOG_TYPE
+};
+
typedef struct CXLCCI CXLCCI;
typedef struct cxl_device_state CXLDeviceState;
struct cxl_cmd;
@@ -194,6 +206,22 @@ typedef struct CXLEventLog {
QSIMPLEQ_HEAD(, CXLEvent) events;
} CXLEventLog;
+typedef struct CXLParamFlags {
+union {
+uint32_t clear_log_supported:1;
+uint32_t populate_log_supported:1;
+uint32_t auto_populate_supported:1;
+uint32_t persistent_across_cold_reset:1;
+uint32_t reserved:28;
+uint32_t pflags;
+};
+} CXLParamFlags;
+
+typedef struct CXLLogCapabilities {
+CXLParamFlags param_flags;
+QemuUUID uuid;
+} CXLLogCapabilities;
+
typedef struct CXLCCI {
struct cxl_cmd cxl_cmd_set[256][256];
struct cel_log {
@@ -202,6 +230,9 @@ typedef struct CXLCCI {
} cel_log[1 << 16];
size_t cel_size;
+/* get log capabilities */
+CXLLogCapabilities supported_log_cap[MAX_LOG_TYPE];
+
/* background command handling (times in ms) */
struct {
uint16_t opcode;
diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
index
