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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

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];
>