[PATCH 0/3] CXL CCI Log Commands implementation

[PATCH 0/3] CXL CCI Log Commands implementation

[Arpit Kumar]

CXL CCI log commands implmented as per CXL Specification 3.1  8.2.9.5
1) get_log_capabilities (Opcode 0402h)
2) clear_log (Opcode 0403h)
3) populate_log (Opcode 0404h)

The patches are generated against the Johnathan's tree
https://gitlab.com/jic23/qemu.git and branch cxl-2024-11-27.

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>

Arpit Kumar (3):
  hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities
    (Opcode 0402h)
  hw/cxl/cxl-mailbox-utils.c: Added support for Clear Log (Opcode 0403h)
  hw/cxl/cxl-mailbox-utils.c: Added support for Populate Log (Opcode
    0404h) as background operation

 hw/cxl/cxl-mailbox-utils.c   | 186 +++++++++++++++++++++++++++++++++++
 include/hw/cxl/cxl_device.h  |  33 +++++++
 include/hw/cxl/cxl_mailbox.h |   5 +
 3 files changed, 224 insertions(+)

-- 
2.34.1

[PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)

[Arpit Kumar]

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
---
 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 9008402d1c..f3502c3f68 100644
--- a/include/hw/cxl/cxl_mailbox.h
+++ b/include/hw/cxl/cxl_mailbox.h
@@ -16,4 +16,9 @@
 #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
 #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
 
+#define CXL_LOG_CAP_CLEAR 1
+#define CXL_LOG_CAP_POPULATE (1 << 1)
+#define CXL_LOG_CAP_AUTO_POPULATE (1 << 2)
+#define CXL_LOG_CAP_PERSISTENT_COLD_RESET (1 << 3)
+
 #endif
-- 
2.34.1

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

[Arpit Kumar]

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
---
 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

[PATCH 3/3] hw/cxl/cxl-mailbox-utils.c: Added support for Populate Log (Opcode 0404h) as background operation

[Arpit Kumar]

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c  | 95 +++++++++++++++++++++++++++++++++++++
 include/hw/cxl/cxl_device.h |  2 +
 2 files changed, 97 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 5fd7f850c4..115c2dc66b 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -78,6 +78,7 @@ enum {
         #define GET_LOG       0x1
         #define GET_LOG_CAPABILITIES   0x2
         #define CLEAR_LOG     0x3
+        #define POPULATE_LOG  0x4
     FEATURES    = 0x05,
         #define GET_SUPPORTED 0x0
         #define GET_FEATURE   0x1
@@ -1149,6 +1150,94 @@ static CXLRetCode cmd_logs_clear_log(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+static void cxl_rebuild_cel(CXLCCI *cci);
+
+static int get_populate_duration(uint32_t total_mem)
+{
+    int secs = 0;
+
+    if (total_mem <= 512) {
+        secs = 4;
+    } else if (total_mem <= 1024) {
+        secs = 8;
+    } else if (total_mem <= 2 * 1024) {
+        secs = 15;
+    } else if (total_mem <= 4 * 1024) {
+        secs = 30;
+    } else if (total_mem <= 8 * 1024) {
+        secs = 60;
+    } else if (total_mem <= 16 * 1024) {
+        secs = 2 * 60;
+    } else if (total_mem <= 32 * 1024) {
+        secs = 4 * 60;
+    } else if (total_mem <= 64 * 1024) {
+        secs = 8 * 60;
+    } else if (total_mem <= 128 * 1024) {
+        secs = 15 * 60;
+    } else if (total_mem <= 256 * 1024) {
+        secs = 30 * 60;
+    } else if (total_mem <= 512 * 1024) {
+        secs = 60 * 60;
+    } else if (total_mem <= 1024 * 1024) {
+        secs = 120 * 60;
+    } else {
+        secs = 240 * 60; /* max 4 hrs */
+    }
+
+    return secs;
+}
+
+/* CXL r3.1 Section 8.2.9.5.5: Populate log (Opcode 0404h) */
+static CXLRetCode cmd_logs_populate_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;
+    uint32_t total_mem = 0;
+    int secs = 0;
+    struct {
+        QemuUUID uuid;
+    } QEMU_PACKED QEMU_ALIGNED(8) * populate_log = (void *)payload_in;
+
+    cap_id = valid_log_check(&populate_log->uuid, cci);
+    if (cap_id == -1) {
+        return CXL_MBOX_INVALID_LOG;
+    }
+
+    if (cci->supported_log_cap[cap_id].param_flags.populate_log_supported) {
+        switch (cap_id) {
+        case CEL:
+            cci->log = CEL;
+            total_mem = (1 << 16) * sizeof(struct cel_log);
+            secs = get_populate_duration(total_mem >> 20);
+            goto start_bg;
+            break;
+        default:
+            return CXL_MBOX_UNSUPPORTED;
+        }
+    }
+    return CXL_MBOX_UNSUPPORTED;
+
+start_bg:
+    cci->bg.runtime = secs * 1000UL;
+    *len_out = 0;
+    return CXL_MBOX_BG_STARTED;
+}
+
+static void __do_populate(CXLCCI *cci)
+{
+    switch (cci->log) {
+    case CEL:
+        cxl_rebuild_cel(cci);
+        break;
+    default:
+        break;
+    }
+}
+
 /* CXL r3.1 section 8.2.9.6: Features */
 /*
  * Get Supported Features output payload
@@ -2918,6 +3007,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
                                      cmd_logs_get_log_capabilities, 0x10, 0 },
     [LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10,
                           CXL_MBOX_IMMEDIATE_LOG_CHANGE},
+    [LOGS][POPULATE_LOG] = { "LOGS_POPULATE_LOG", cmd_logs_populate_log, 0x10,
+                             (CXL_MBOX_IMMEDIATE_LOG_CHANGE |
+                              CXL_MBOX_BACKGROUND_OPERATION)},
     [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
                                   cmd_features_get_supported, 0x8, 0 },
     [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
@@ -3098,6 +3190,9 @@ static void bg_timercb(void *opaque)
         case 0x0201: /* fw transfer */
             __do_firmware_xfer(cci);
             break;
+        case 0x0404: /* populate_ log */
+            __do_populate(cci);
+            break;
         case 0x4400: /* sanitize */
         {
             CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index e7cb99a1d2..d90d0d4a8f 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -230,6 +230,8 @@ typedef struct CXLCCI {
     } cel_log[1 << 16];
     size_t cel_size;
 
+    enum Logs log;
+
     /* get log capabilities */
     CXLLogCapabilities supported_log_cap[MAX_LOG_TYPE];
 
-- 
2.34.1

Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)

[Jonathan Cameron via]

On Mon,  3 Feb 2025 11:29:48 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:

Please add some descriptive teext here.

> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
> Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
> Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>

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
depending on runtime configuration.  So may be better to just pick
between a few const arrays like cxl_get_log_cap.

Good to also store the length of that log or use a terminating entry
of some type so that we don't need to iterate over empty entries.

> +    }
> +}
> +
>  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 {

For things in the header please prefix with CXL.  The chances
of a naming clash in future drivers that include this is too high
without that! 

Also QEMU tends to use typedefs for enums

typedef enum {
	CXL_LOG_CEL,
	CXL_LOG_VENDOR,
} CXLLogType;
or something like that.

Some of these abbreviations are a bit too compact and
don't line up with spec like CEL does.,




> +    CEL,
> +    VDL,
> +    CSDL,
CXL_LOG_COMPONENT_STATE_DUMP,
> +    ECSL,
CXL_LOG_ECS, //this one is standard acronym
> +    MTCL,
CXL_LOG_MEDIA_TEST_CAP
> +    MTRSL,
CXL_LOG_MEDIA_TEST_SHORT
> +    MTRLL,
CXL_LOG_MEDIA_TEST_LONG

perhaps?
> +    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 {

Unless I'm reading this wrong this is a union of lots of flags
on top of each other.

Also, don't use bitfields. They don't play well with
different endian architectures. 
Defines for the various bits are a more reliable solution.
Similar to the ones you have below in cxl_mailbox.h


> +        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];
Perhaps a const pointer is appropriate?
> +
>      /* 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 9008402d1c..f3502c3f68 100644
> --- a/include/hw/cxl/cxl_mailbox.h
> +++ b/include/hw/cxl/cxl_mailbox.h
> @@ -16,4 +16,9 @@
>  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
>  #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
>  
> +#define CXL_LOG_CAP_CLEAR 1
> +#define CXL_LOG_CAP_POPULATE (1 << 1)
> +#define CXL_LOG_CAP_AUTO_POPULATE (1 << 2)
> +#define CXL_LOG_CAP_PERSISTENT_COLD_RESET (1 << 3)
> +
>  #endif

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

[Jonathan Cameron via]

On Mon,  3 Feb 2025 11:29:49 +0530
Arpit Kumar <arpit1.kumar@samsung.com> 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 <arpit1.kumar@samsung.com>
> Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
> Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
> ---
>  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",

Re: [PATCH 3/3] hw/cxl/cxl-mailbox-utils.c: Added support for Populate Log (Opcode 0404h) as background operation

[Jonathan Cameron via]

On Mon,  3 Feb 2025 11:29:50 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:

> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
> Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
> Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>

Likewise, the CEL isn't a dynamic thing.  Asking to populate
it makes little sense so the log capability should always
say this is not supported.

I suspect you had this running with a different log and flipped
to CEL for purposes of up streaming?

Anyhow, choose something else.  Maybe component state dump or ECS log?

> ---
>  hw/cxl/cxl-mailbox-utils.c  | 95 +++++++++++++++++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h |  2 +
>  2 files changed, 97 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 5fd7f850c4..115c2dc66b 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -78,6 +78,7 @@ enum {
>          #define GET_LOG       0x1
>          #define GET_LOG_CAPABILITIES   0x2
>          #define CLEAR_LOG     0x3
> +        #define POPULATE_LOG  0x4
>      FEATURES    = 0x05,
>          #define GET_SUPPORTED 0x0
>          #define GET_FEATURE   0x1
> @@ -1149,6 +1150,94 @@ static CXLRetCode cmd_logs_clear_log(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static void cxl_rebuild_cel(CXLCCI *cci);
> +
> +static int get_populate_duration(uint32_t total_mem)
> +{
> +    int secs = 0;
> +
> +    if (total_mem <= 512) {
> +        secs = 4;
> +    } else if (total_mem <= 1024) {
> +        secs = 8;
> +    } else if (total_mem <= 2 * 1024) {
> +        secs = 15;
> +    } else if (total_mem <= 4 * 1024) {
> +        secs = 30;
> +    } else if (total_mem <= 8 * 1024) {
> +        secs = 60;
> +    } else if (total_mem <= 16 * 1024) {
> +        secs = 2 * 60;
> +    } else if (total_mem <= 32 * 1024) {
> +        secs = 4 * 60;
> +    } else if (total_mem <= 64 * 1024) {
> +        secs = 8 * 60;
> +    } else if (total_mem <= 128 * 1024) {
> +        secs = 15 * 60;
> +    } else if (total_mem <= 256 * 1024) {
> +        secs = 30 * 60;
> +    } else if (total_mem <= 512 * 1024) {
> +        secs = 60 * 60;
> +    } else if (total_mem <= 1024 * 1024) {
> +        secs = 120 * 60;
> +    } else {
> +        secs = 240 * 60; /* max 4 hrs */
> +    }
> +
> +    return secs;
> +}
> +
> +/* CXL r3.1 Section 8.2.9.5.5: Populate log (Opcode 0404h) */
> +static CXLRetCode cmd_logs_populate_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;
> +    uint32_t total_mem = 0;
> +    int secs = 0;
> +    struct {
> +        QemuUUID uuid;
> +    } QEMU_PACKED QEMU_ALIGNED(8) * populate_log = (void *)payload_in;
> +
> +    cap_id = valid_log_check(&populate_log->uuid, cci);
> +    if (cap_id == -1) {
> +        return CXL_MBOX_INVALID_LOG;
> +    }
> +
> +    if (cci->supported_log_cap[cap_id].param_flags.populate_log_supported) {
> +        switch (cap_id) {
> +        case CEL:
Similar to before, a callback to fill the log inside the cap entry
is probably going to be the most extensible way to do this rather
than using an ID and a switch statement that gets steadily more
complex and doesn't easily allow for different device emulation to
make different choices on what to fill with (e.g. faking component
state dump for a type 3 vs a type 2 device - once supported in qemu).

> +            cci->log = CEL;
> +            total_mem = (1 << 16) * sizeof(struct cel_log);
> +            secs = get_populate_duration(total_mem >> 20);

Why would the CEL be based on memory size?

> +            goto start_bg;
> +            break;
> +        default:
> +            return CXL_MBOX_UNSUPPORTED;
> +        }
> +    }
> +    return CXL_MBOX_UNSUPPORTED;
> +
> +start_bg:
> +    cci->bg.runtime = secs * 1000UL;
> +    *len_out = 0;
> +    return CXL_MBOX_BG_STARTED;
> +}
> +
> +static void __do_populate(CXLCCI *cci)
> +{
> +    switch (cci->log) {
> +    case CEL:
> +        cxl_rebuild_cel(cci);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
>  /* CXL r3.1 section 8.2.9.6: Features */
>  /*
>   * Get Supported Features output payload
> @@ -2918,6 +3007,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>                                       cmd_logs_get_log_capabilities, 0x10, 0 },
>      [LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10,
>                            CXL_MBOX_IMMEDIATE_LOG_CHANGE},
> +    [LOGS][POPULATE_LOG] = { "LOGS_POPULATE_LOG", cmd_logs_populate_log, 0x10,
> +                             (CXL_MBOX_IMMEDIATE_LOG_CHANGE |
> +                              CXL_MBOX_BACKGROUND_OPERATION)},
>      [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
>                                    cmd_features_get_supported, 0x8, 0 },
>      [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
> @@ -3098,6 +3190,9 @@ static void bg_timercb(void *opaque)
>          case 0x0201: /* fw transfer */
>              __do_firmware_xfer(cci);
>              break;
> +        case 0x0404: /* populate_ log */
> +            __do_populate(cci);
> +            break;
>          case 0x4400: /* sanitize */
>          {
>              CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index e7cb99a1d2..d90d0d4a8f 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -230,6 +230,8 @@ typedef struct CXLCCI {
>      } cel_log[1 << 16];
>      size_t cel_size;
>  
> +    enum Logs log;
> +
>      /* get log capabilities */
>      CXLLogCapabilities supported_log_cap[MAX_LOG_TYPE];
>  

Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)

[Arpit Kumar]

[-- Attachment #1: Type: text/plain, Size: 9374 bytes --]

On 04/02/25 10:28AM, Jonathan Cameron wrote:
>On Mon,  3 Feb 2025 11:29:48 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>Please add some descriptive teext here.
>
Sure, will append here in V2 patch.

>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>> Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
>> Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
>
>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 compilation error as it used below
to assign value:
     cci->supported_log_cap[set] = cxl_get_log_cap[set];

>Seems relatively unlikely we'll have lots of different log combinations
>depending on runtime configuration.  So may be better to just pick
>between a few const arrays like cxl_get_log_cap.
>
okay, will fix it in V2 patch.

>Good to also store the length of that log or use a terminating entry
>of some type so that we don't need to iterate over empty entries.
>
okay, will use terminating entry for the same.

>> +    }
>> +}
>> +
>>  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 {
>
>For things in the header please prefix with CXL.  The chances
>of a naming clash in future drivers that include this is too high
>without that!
>
>Also QEMU tends to use typedefs for enums
>
okay

>typedef enum {
>	CXL_LOG_CEL,
>	CXL_LOG_VENDOR,
>} CXLLogType;
>or something like that.
>
>Some of these abbreviations are a bit too compact and
>don't line up with spec like CEL does.,
>
okay, will update the abbreviations as per your suggestion in
V2 patch.

>
>
>
>> +    CEL,
>> +    VDL,
>> +    CSDL,
>CXL_LOG_COMPONENT_STATE_DUMP,
>> +    ECSL,
>CXL_LOG_ECS, //this one is standard acronym
>> +    MTCL,
>CXL_LOG_MEDIA_TEST_CAP
>> +    MTRSL,
>CXL_LOG_MEDIA_TEST_SHORT
>> +    MTRLL,
>CXL_LOG_MEDIA_TEST_LONG
>
>perhaps?
>> +    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 {
>
>Unless I'm reading this wrong this is a union of lots of flags
>on top of each other.
>
>Also, don't use bitfields. They don't play well with
>different endian architectures.
>Defines for the various bits are a more reliable solution.
>Similar to the ones you have below in cxl_mailbox.h
>
okay, will fix this in V2 patch.

>
>> +        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];
>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];
>> +
>>      /* 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 9008402d1c..f3502c3f68 100644
>> --- a/include/hw/cxl/cxl_mailbox.h
>> +++ b/include/hw/cxl/cxl_mailbox.h
>> @@ -16,4 +16,9 @@
>>  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
>>  #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
>>
>> +#define CXL_LOG_CAP_CLEAR 1
>> +#define CXL_LOG_CAP_POPULATE (1 << 1)
>> +#define CXL_LOG_CAP_AUTO_POPULATE (1 << 2)
>> +#define CXL_LOG_CAP_PERSISTENT_COLD_RESET (1 << 3)
>> +
>>  #endif
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Arpit Kumar]

[-- Attachment #1: Type: text/plain, Size: 3915 bytes --]

On 04/02/25 10:53AM, Jonathan Cameron wrote:
>On Mon,  3 Feb 2025 11:29:49 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> 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 <arpit1.kumar@samsung.com>
>> Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
>> Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
>> ---
>>  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",
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] hw/cxl/cxl-mailbox-utils.c: Added support for Populate Log (Opcode 0404h) as background operation

[Arpit Kumar]

[-- Attachment #1: Type: text/plain, Size: 6419 bytes --]

On 04/02/25 10:58AM, Jonathan Cameron wrote:
>On Mon,  3 Feb 2025 11:29:50 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>> Reviewed-by: Alok Rathore <alok.rathore@samsung.com>
>> Reviewed-by: Krishna Kanth Reddy <krish.reddy@samsung.com>
>
>Likewise, the CEL isn't a dynamic thing.  Asking to populate
>it makes little sense so the log capability should always
>say this is not supported.
>
>I suspect you had this running with a different log and flipped
>to CEL for purposes of up streaming?
>
>Anyhow, choose something else.  Maybe component state dump or ECS log?
>
Thanks for the review Jonathan, will update the same in V2 patch 
and return unsupported for CEL.
>> ---
>>  hw/cxl/cxl-mailbox-utils.c  | 95 +++++++++++++++++++++++++++++++++++++
>>  include/hw/cxl/cxl_device.h |  2 +
>>  2 files changed, 97 insertions(+)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 5fd7f850c4..115c2dc66b 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -78,6 +78,7 @@ enum {
>>          #define GET_LOG       0x1
>>          #define GET_LOG_CAPABILITIES   0x2
>>          #define CLEAR_LOG     0x3
>> +        #define POPULATE_LOG  0x4
>>      FEATURES    = 0x05,
>>          #define GET_SUPPORTED 0x0
>>          #define GET_FEATURE   0x1
>> @@ -1149,6 +1150,94 @@ static CXLRetCode cmd_logs_clear_log(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> +static void cxl_rebuild_cel(CXLCCI *cci);
>> +
>> +static int get_populate_duration(uint32_t total_mem)
>> +{
>> +    int secs = 0;
>> +
>> +    if (total_mem <= 512) {
>> +        secs = 4;
>> +    } else if (total_mem <= 1024) {
>> +        secs = 8;
>> +    } else if (total_mem <= 2 * 1024) {
>> +        secs = 15;
>> +    } else if (total_mem <= 4 * 1024) {
>> +        secs = 30;
>> +    } else if (total_mem <= 8 * 1024) {
>> +        secs = 60;
>> +    } else if (total_mem <= 16 * 1024) {
>> +        secs = 2 * 60;
>> +    } else if (total_mem <= 32 * 1024) {
>> +        secs = 4 * 60;
>> +    } else if (total_mem <= 64 * 1024) {
>> +        secs = 8 * 60;
>> +    } else if (total_mem <= 128 * 1024) {
>> +        secs = 15 * 60;
>> +    } else if (total_mem <= 256 * 1024) {
>> +        secs = 30 * 60;
>> +    } else if (total_mem <= 512 * 1024) {
>> +        secs = 60 * 60;
>> +    } else if (total_mem <= 1024 * 1024) {
>> +        secs = 120 * 60;
>> +    } else {
>> +        secs = 240 * 60; /* max 4 hrs */
>> +    }
>> +
>> +    return secs;
>> +}
>> +
>> +/* CXL r3.1 Section 8.2.9.5.5: Populate log (Opcode 0404h) */
>> +static CXLRetCode cmd_logs_populate_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;
>> +    uint32_t total_mem = 0;
>> +    int secs = 0;
>> +    struct {
>> +        QemuUUID uuid;
>> +    } QEMU_PACKED QEMU_ALIGNED(8) * populate_log = (void *)payload_in;
>> +
>> +    cap_id = valid_log_check(&populate_log->uuid, cci);
>> +    if (cap_id == -1) {
>> +        return CXL_MBOX_INVALID_LOG;
>> +    }
>> +
>> +    if (cci->supported_log_cap[cap_id].param_flags.populate_log_supported) {
>> +        switch (cap_id) {
>> +        case CEL:
>Similar to before, a callback to fill the log inside the cap entry
>is probably going to be the most extensible way to do this rather
>than using an ID and a switch statement that gets steadily more
>complex and doesn't easily allow for different device emulation to
>make different choices on what to fill with (e.g. faking component
>state dump for a type 3 vs a type 2 device - once supported in qemu).
>
okay

>> +            cci->log = CEL;
>> +            total_mem = (1 << 16) * sizeof(struct cel_log);
>> +            secs = get_populate_duration(total_mem >> 20);
>
>Why would the CEL be based on memory size?
>
CEL is not dependent on memory size. It is representing CEL buffer size. 
total_mem variable is misleading in this case. will take care in next
iteration.
>> +            goto start_bg;
>> +            break;
>> +        default:
>> +            return CXL_MBOX_UNSUPPORTED;
>> +        }
>> +    }
>> +    return CXL_MBOX_UNSUPPORTED;
>> +
>> +start_bg:
>> +    cci->bg.runtime = secs * 1000UL;
>> +    *len_out = 0;
>> +    return CXL_MBOX_BG_STARTED;
>> +}
>> +
>> +static void __do_populate(CXLCCI *cci)
>> +{
>> +    switch (cci->log) {
>> +    case CEL:
>> +        cxl_rebuild_cel(cci);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>>  /* CXL r3.1 section 8.2.9.6: Features */
>>  /*
>>   * Get Supported Features output payload
>> @@ -2918,6 +3007,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>>                                       cmd_logs_get_log_capabilities, 0x10, 0 },
>>      [LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10,
>>                            CXL_MBOX_IMMEDIATE_LOG_CHANGE},
>> +    [LOGS][POPULATE_LOG] = { "LOGS_POPULATE_LOG", cmd_logs_populate_log, 0x10,
>> +                             (CXL_MBOX_IMMEDIATE_LOG_CHANGE |
>> +                              CXL_MBOX_BACKGROUND_OPERATION)},
>>      [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED",
>>                                    cmd_features_get_supported, 0x8, 0 },
>>      [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
>> @@ -3098,6 +3190,9 @@ static void bg_timercb(void *opaque)
>>          case 0x0201: /* fw transfer */
>>              __do_firmware_xfer(cci);
>>              break;
>> +        case 0x0404: /* populate_ log */
>> +            __do_populate(cci);
>> +            break;
>>          case 0x4400: /* sanitize */
>>          {
>>              CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index e7cb99a1d2..d90d0d4a8f 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -230,6 +230,8 @@ typedef struct CXLCCI {
>>      } cel_log[1 << 16];
>>      size_t cel_size;
>>
>> +    enum Logs log;
>> +
>>      /* get log capabilities */
>>      CXLLogCapabilities supported_log_cap[MAX_LOG_TYPE];
>>
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)

[Jonathan Cameron via]

> >>
> >> +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