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

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

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