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

[Arpit Kumar <arpit1.kumar@samsung.com>]

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