Re: [PATCH 2/3] hw/cxl/cxl-mailbox-utils.c: Added support for Clear Log (Opcode 0403h)

2025-02-12 Thread Arpit Kumar 

On 04/02/25 10:53AM, Jonathan Cameron wrote:

On Mon,  3 Feb 2025 11:29:49 +0530
Arpit Kumar  wrote:

Add some description of what is being added here.

Key issue in here is that clearing the CEL doesn't make
sense. It is a description of what the device supports, there
is no state to clear in it.  To add this command you need
to pick a different log.

Jonathan

Thanks for the review Jonathan, will modify the code accordingly 
in V2 patch.





Signed-off-by: Arpit Kumar 
Reviewed-by: Alok Rathore 
Reviewed-by: Krishna Kanth Reddy 
---
 hw/cxl/cxl-mailbox-utils.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3d66a425a9..5fd7f850c4 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -77,6 +77,7 @@ enum {
 #define GET_SUPPORTED 0x0
 #define GET_LOG   0x1
 #define GET_LOG_CAPABILITIES   0x2
+#define CLEAR_LOG 0x3
 FEATURES= 0x05,
 #define GET_SUPPORTED 0x0
 #define GET_FEATURE   0x1
@@ -1115,6 +1116,39 @@ static CXLRetCode cmd_logs_get_log_capabilities(const 
struct cxl_cmd *cmd,
 return CXL_MBOX_SUCCESS;
 }

+/* CXL r3.1 Section 8.2.9.5.4: Clear Log (Opcode 0403h) */
+static CXLRetCode cmd_logs_clear_log(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) * clear_log = (void *)payload_in;
+
+cap_id = valid_log_check(&clear_log->uuid, cci);
+if (cap_id == -1) {
+return CXL_MBOX_INVALID_LOG;
+}


Follow on from previous patch, if this returns the cap pointer,
the following code wont have to index the array and should end up simpler.


okay


+
+if (cci->supported_log_cap[cap_id].param_flags.clear_log_supported) {

I would flip this.
   if (!(cap->param_flags & PARAM_FLAG_CLEAR_LOG_SUPPORTED)) {
   return CXL_MBOX_UNSUPPORTED;
   }



+switch (cap_id) {
+case CEL:


So if we return the cap as suggested, it will have to reference what it is
or provide a callback (which might be cleaner as this grows).

However, what does clearly the command effects log mean?
This makes no sense.  So if you want to implement clear_log you
need to implement a different log to clear.


okay


+memset(cci->cel_log, 0, (1 << 16) * sizeof(struct cel_log));
+cci->cel_size = 0;
+break;
+default:
+return CXL_MBOX_UNSUPPORTED;
+}
+} else {
+return CXL_MBOX_UNSUPPORTED;
+}
+return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.1 section 8.2.9.6: Features */
 /*
  * Get Supported Features output payload
@@ -2882,6 +2916,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
 [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 },
+[LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10,
+  CXL_MBOX_IMMEDIATE_LOG_CHANGE},
 [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
   cmd_features_get_supported, 0x8, 0 },
 [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",

Re: [PATCH 2/3] hw/cxl/cxl-mailbox-utils.c: Added support for Clear Log (Opcode 0403h)

2025-02-04 Thread Jonathan Cameron via 
On Mon,  3 Feb 2025 11:29:49 +0530
Arpit Kumar  wrote:

Add some description of what is being added here.

Key issue in here is that clearing the CEL doesn't make
sense. It is a description of what the device supports, there
is no state to clear in it.  To add this command you need
to pick a different log.

Jonathan


> Signed-off-by: Arpit Kumar 
> Reviewed-by: Alok Rathore 
> Reviewed-by: Krishna Kanth Reddy 
> ---
>  hw/cxl/cxl-mailbox-utils.c | 36 
>  1 file changed, 36 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 3d66a425a9..5fd7f850c4 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -77,6 +77,7 @@ enum {
>  #define GET_SUPPORTED 0x0
>  #define GET_LOG   0x1
>  #define GET_LOG_CAPABILITIES   0x2
> +#define CLEAR_LOG 0x3
>  FEATURES= 0x05,
>  #define GET_SUPPORTED 0x0
>  #define GET_FEATURE   0x1
> @@ -1115,6 +1116,39 @@ static CXLRetCode cmd_logs_get_log_capabilities(const 
> struct cxl_cmd *cmd,
>  return CXL_MBOX_SUCCESS;
>  }
>  
> +/* CXL r3.1 Section 8.2.9.5.4: Clear Log (Opcode 0403h) */
> +static CXLRetCode cmd_logs_clear_log(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) * clear_log = (void *)payload_in;
> +
> +cap_id = valid_log_check(&clear_log->uuid, cci);
> +if (cap_id == -1) {
> +return CXL_MBOX_INVALID_LOG;
> +}

Follow on from previous patch, if this returns the cap pointer,
the following code wont have to index the array and should end up simpler.

> +
> +if (cci->supported_log_cap[cap_id].param_flags.clear_log_supported) {
I would flip this.
if (!(cap->param_flags & PARAM_FLAG_CLEAR_LOG_SUPPORTED)) {
return CXL_MBOX_UNSUPPORTED;
}


> +switch (cap_id) {
> +case CEL:

So if we return the cap as suggested, it will have to reference what it is
or provide a callback (which might be cleaner as this grows).

However, what does clearly the command effects log mean?
This makes no sense.  So if you want to implement clear_log you
need to implement a different log to clear.

> +memset(cci->cel_log, 0, (1 << 16) * sizeof(struct cel_log));
> +cci->cel_size = 0;
> +break;
> +default:
> +return CXL_MBOX_UNSUPPORTED;
> +}
> +} else {
> +return CXL_MBOX_UNSUPPORTED;
> +}
> +return CXL_MBOX_SUCCESS;
> +}
> +
>  /* CXL r3.1 section 8.2.9.6: Features */
>  /*
>   * Get Supported Features output payload
> @@ -2882,6 +2916,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>  [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 
> },
> +[LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10,
> +  CXL_MBOX_IMMEDIATE_LOG_CHANGE},
>  [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
>cmd_features_get_supported, 0x8, 0 },
>  [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",

[PATCH 2/3] hw/cxl/cxl-mailbox-utils.c: Added support for Clear Log (Opcode 0403h)

2025-02-03 Thread Arpit Kumar 
Signed-off-by: Arpit Kumar 
Reviewed-by: Alok Rathore 
Reviewed-by: Krishna Kanth Reddy 
---
 hw/cxl/cxl-mailbox-utils.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3d66a425a9..5fd7f850c4 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -77,6 +77,7 @@ enum {
 #define GET_SUPPORTED 0x0
 #define GET_LOG   0x1
 #define GET_LOG_CAPABILITIES   0x2
+#define CLEAR_LOG 0x3
 FEATURES= 0x05,
 #define GET_SUPPORTED 0x0
 #define GET_FEATURE   0x1
@@ -1115,6 +1116,39 @@ static CXLRetCode cmd_logs_get_log_capabilities(const 
struct cxl_cmd *cmd,
 return CXL_MBOX_SUCCESS;
 }
 
+/* CXL r3.1 Section 8.2.9.5.4: Clear Log (Opcode 0403h) */
+static CXLRetCode cmd_logs_clear_log(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) * clear_log = (void *)payload_in;
+
+cap_id = valid_log_check(&clear_log->uuid, cci);
+if (cap_id == -1) {
+return CXL_MBOX_INVALID_LOG;
+}
+
+if (cci->supported_log_cap[cap_id].param_flags.clear_log_supported) {
+switch (cap_id) {
+case CEL:
+memset(cci->cel_log, 0, (1 << 16) * sizeof(struct cel_log));
+cci->cel_size = 0;
+break;
+default:
+return CXL_MBOX_UNSUPPORTED;
+}
+} else {
+return CXL_MBOX_UNSUPPORTED;
+}
+return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.1 section 8.2.9.6: Features */
 /*
  * Get Supported Features output payload
@@ -2882,6 +2916,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
 [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 },
+[LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10,
+  CXL_MBOX_IMMEDIATE_LOG_CHANGE},
 [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
   cmd_features_get_supported, 0x8, 0 },
 [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
-- 
2.34.1