Re: CXL CCI Get/Set Alert Configuration commands implmented as per CXL Specification 3.2 section 8.2.10.9.3

[Sweta Kumari <s5.kumari@samsung.com>]

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

On 14/02/25 05:43PM, Jonathan Cameron wrote:
>On Fri, 14 Feb 2025 18:52:11 +0530
>Sweta Kumari <s5.kumari@samsung.com> wrote:
>
>> 1)get alert configuration(Opcode 4201h)
>> 2)set alert configuration(Opcode 4202h)
>
>Move the change log to below the ---
>The key thing being git then doesn't pick it up whilst applying the patch.
>Whilst changed logs are very useful during the review process we don't
>typically want to keep them in the git history for ever!
>
>Otherwise, main comment here is shorten more names.
>
>Jonathan
>
Thank you for the feedback, will make the changes accordingly in the v3
patch.
>>
>> This v2 patch addresses the feedback from the v1 patch and include some minor new changes.
>>
>> Changes in V2:
>> - Removed cover letter as it's a single patch
>> - Added latest spec reference
>> - Fixed alignment issues
>> - Updated shorter variable names to be more descriptive
>> - Replaced field-by-field initialization in 'init_alert_config' with structured initialization for improved readability.
>> - Replaced bit fields with 'uint8_t' and added defines for individual bits.
>>
>> The patch is generated against the Johnathan's tree
>> https://gitlab.com/jic23/qemu.git and branch cxl-2024-11-27.
>>
>> Signed-off-by: Sweta Kumari <s5.kumari@samsung.com>
>> ---
>>  hw/cxl/cxl-mailbox-utils.c  | 116 ++++++++++++++++++++++++++++++++++++
>>  hw/mem/cxl_type3.c          |  16 +++++
>>  include/hw/cxl/cxl_device.h |  15 +++++
>>  3 files changed, 147 insertions(+)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 9c7ea5bc35..105c63fdec 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -28,6 +28,11 @@
>>  #define CXL_DC_EVENT_LOG_SIZE 8
>>  #define CXL_NUM_EXTENTS_SUPPORTED 512
>>  #define CXL_NUM_TAGS_SUPPORTED 0
>> +#define CXL_ALERTS_LIFE_USED_WARNING_THRESHOLD (1 << 0)
>> +#define CXL_ALERTS_DEVICE_OVER_TEMP_WARNING_THRESHOLD (1 << 1)
>> +#define CXL_ALERTS_DEVICE_UNDER_TEMP_WARNING_THRESHOLD (1 << 2)
>> +#define CXL_ALERTS_CORRECTED_VOLATILE_MEMORY_ERROR_WARNING_THRESHOLD (1 << 3)
>> +#define CXL_ALERTS_CORRECTED_PERSISTENT_MEMORY_ERROR_WARNING_THRESHOLD (1 << 4)
>Let's shorten these as they are very ugly to use when a line long!
>
>#define CXL_ALERTS_OVER_TEMP_WARN_THRESH
>etc. Similar to comments below.
>
Ok, will shorten the macro names as suggested in v3 patch.
>>
>>  /*
>>   * How to add a new command, example. The command set FOO, with cmd BAR.
>> @@ -86,6 +91,9 @@ enum {
>>          #define GET_PARTITION_INFO     0x0
>>          #define GET_LSA       0x2
>>          #define SET_LSA       0x3
>> +    HEALTH_INFO_ALERTS = 0x42,
>> +        #define GET_ALERT_CONFIGURATION 0x1
>> +        #define SET_ALERT_CONFIGURATION 0x2
>CONFIG maybe enough?
>
Okay.
>>      SANITIZE    = 0x44,
>>          #define OVERWRITE     0x0
>>          #define SECURE_ERASE  0x1
>> @@ -1625,6 +1633,110 @@ static CXLRetCode cmd_ccls_set_lsa(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> +/* CXL r3.2 Section 8.2.10.9.3.2 Get Alert Configuration (Opcode 4201h) */
>> +static CXLRetCode cmd_get_alert_config(const struct cxl_cmd *cmd,
>> +                                       uint8_t *payload_in,
>> +                                       size_t len_in,
>> +                                       uint8_t *payload_out,
>> +                                       size_t *len_out,
>> +                                       CXLCCI *cci)
>> +{
>> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>> +    CXLAlertConfig *out = (void *)payload_out;
>
>In this case we can cast to the right type (can't do that if we don't
>name that type which happens in a lot these structures). So prefer
>    CXLAlertConfig *out = (CXLAlertConfig *)payload_out;
>
>
Okay, will fix this in v3 patch
>> +
>> +    memcpy(out, &ct3d->alert_config, sizeof(ct3d->alert_config));
>> +    *len_out = sizeof(ct3d->alert_config);
>> +
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>> +/* CXL r3.2 Section 8.2.10.9.3.3 Set Alert Configuration (Opcode 4202h) */
>> +static CXLRetCode cmd_set_alert_config(const struct cxl_cmd *cmd,
>> +                                       uint8_t *payload_in,
>> +                                       size_t len_in,
>> +                                       uint8_t *payload_out,
>> +                                       size_t *len_out,
>> +                                       CXLCCI *cci)
>> +{
>> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>> +    CXLAlertConfig *alert_config = &ct3d->alert_config;
>> +    struct {
>> +        uint8_t valid_alert_actions;
>> +        uint8_t enable_alert_actions;
>> +        uint8_t life_used_warning_threshold;
>> +        uint8_t rsvd;
>> +        uint16_t device_over_temperature_warning_threshold;
>> +        uint16_t device_under_temperature_warning_threshold;
>> +        uint16_t Corrected_volatile_memory_error_warning_threshold;
>> +        uint16_t Corrected_persistent_memory_error_warning_threshold;
>
>Shorten these as well. Similar to suggestions below.  They are
>just too long to make for nice code!
>
Got it.
>> +    } QEMU_PACKED *in = (void *)payload_in;
>> +
>> +    if (in->valid_alert_actions & CXL_ALERTS_LIFE_USED_WARNING_THRESHOLD) {
>> +        /*
>> +         * CXL 3.2 Table 8-149 The life used warning threshold shall be
>> +         * less than the life used critical alert value.
>> +         */
>> +        if (in->life_used_warning_threshold >=
>> +            alert_config->life_used_critical_alert_threshold) {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +        alert_config->life_used_warning_threshold =
>> +            in->life_used_warning_threshold;
>> +        alert_config->enable_alerts |= CXL_ALERTS_LIFE_USED_WARNING_THRESHOLD;
>> +    }
>> +
>> +    if (in->valid_alert_actions &
>> +        CXL_ALERTS_DEVICE_OVER_TEMP_WARNING_THRESHOLD) {
>> +        /*
>> +         * CXL 3.2 Table 8-149 The Device Over-Temperature Warning Threshold
>> +         * shall be less than the the Device Over-Temperature Critical
>> +         * Alert Threshold.
>> +         */
>> +        if (in->device_over_temperature_warning_threshold >=
>> +            alert_config->device_over_temperature_critical_alert_threshold) {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +        alert_config->device_over_temperature_warning_threshold =
>> +            in->device_over_temperature_warning_threshold;
>> +        alert_config->enable_alerts |=
>> +            CXL_ALERTS_DEVICE_OVER_TEMP_WARNING_THRESHOLD;
>> +    }
>> +
>> +    if (in->valid_alert_actions &
>> +        CXL_ALERTS_DEVICE_UNDER_TEMP_WARNING_THRESHOLD) {
>> +        /*
>> +         * CXL 3.2 Table 8-149 The Device Under-Temperature Warning Threshold
>> +         * shall be higher than the the Device Under-Temperature Critical
>> +         * Alert Threshold.
>> +         */
>> +        if (in->device_under_temperature_warning_threshold <=
>> +            alert_config->device_under_temperature_critical_alert_threshold) {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +        alert_config->device_under_temperature_warning_threshold =
>> +            in->device_under_temperature_warning_threshold;
>> +        alert_config->enable_alerts |=
>> +            CXL_ALERTS_DEVICE_UNDER_TEMP_WARNING_THRESHOLD;
>> +    }
>> +
>> +    if (in->valid_alert_actions &
>> +        CXL_ALERTS_CORRECTED_VOLATILE_MEMORY_ERROR_WARNING_THRESHOLD) {
>> +        alert_config->Corrected_volatile_memory_error_warning_threshold =
>> +            in->Corrected_volatile_memory_error_warning_threshold;
>> +        alert_config->enable_alerts |=
>> +            CXL_ALERTS_CORRECTED_VOLATILE_MEMORY_ERROR_WARNING_THRESHOLD;
>> +    }
>> +
>> +    if (in->valid_alert_actions &
>> +        CXL_ALERTS_CORRECTED_PERSISTENT_MEMORY_ERROR_WARNING_THRESHOLD) {
>> +        alert_config->Corrected_persistent_memory_error_warning_threshold =
>> +            in->Corrected_persistent_memory_error_warning_threshold;
>> +        alert_config->enable_alerts |=
>> +            CXL_ALERTS_CORRECTED_PERSISTENT_MEMORY_ERROR_WARNING_THRESHOLD;
>> +    }
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>>  /* Perform the actual device zeroing */
>>  static void __do_sanitization(CXLType3Dev *ct3d)
>>  {
>> @@ -2859,6 +2971,10 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>>      [CCLS][GET_LSA] = { "CCLS_GET_LSA", cmd_ccls_get_lsa, 8, 0 },
>>      [CCLS][SET_LSA] = { "CCLS_SET_LSA", cmd_ccls_set_lsa,
>>          ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE | CXL_MBOX_IMMEDIATE_DATA_CHANGE },
>> +    [HEALTH_INFO_ALERTS][GET_ALERT_CONFIGURATION] = {"GET_ALERT_CONFIGURATION",
>
>Space after { to match local style.
>
Ok, will address this in v3 patch.
>> +        cmd_get_alert_config, 0, 0 },
>> +    [HEALTH_INFO_ALERTS][SET_ALERT_CONFIGURATION] = {"SET_ALERT_CONFIGURATION",
>> +        cmd_set_alert_config, 12, CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
>>      [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite, 0,
>>          (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
>>           CXL_MBOX_SECURITY_STATE_CHANGE |
>
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index a64739be25..1da23bf553 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -581,6 +581,19 @@ typedef struct CXLSetFeatureInfo {
>>      size_t data_size;
>>  } CXLSetFeatureInfo;
>>
>> +typedef struct CXLAlertConfig {
>> +    uint8_t valid_alerts;
>> +    uint8_t enable_alerts;
>> +    uint8_t life_used_critical_alert_threshold;
>> +    uint8_t life_used_warning_threshold;
>> +    uint16_t device_over_temperature_critical_alert_threshold;
>I think we can shorten these at least a bit without lost of meaning!
>It's on a device so can drop that entirely. Perhaps
>
>    uint8_t life_used_crit_alert_thresh;
>    uint8_t life_used_warn_thresh;
>    uint16_t over_temp_crit_alert_thresh;
>    uint16_t under_temp_crit_alert_thresh;
>    uint16_t over_temp_warn_thresh;
>    uint16_t under_temp_warn_thresh;
>    uint16_t cor_volatile_mem_err_warn_thresh;
>    uint16_t cor_persistent_mem_err_warn_thresh;
>
Ok, will shorten the field names as suggested.
>> +    uint16_t device_under_temperature_critical_alert_threshold;
>> +    uint16_t device_over_temperature_warning_threshold;
>> +    uint16_t device_under_temperature_warning_threshold;
>> +    uint16_t Corrected_volatile_memory_error_warning_threshold;
>Capital in just this one doesn't make much sense.
>> +    uint16_t Corrected_persistent_memory_error_warning_threshold;
>> +} QEMU_PACKED CXLAlertConfig;
>
>

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