[PATCH v4 3/3] hw/cxl/cxl-mailbox-utils: Add device DDR5 ECS control feature

shiju.jose--- via posted 3 patches 8 months, 4 weeks ago
Maintainers: Jonathan Cameron <jonathan.cameron@huawei.com>, Fan Ni <fan.ni@samsung.com>
There is a newer version of this series
[PATCH v4 3/3] hw/cxl/cxl-mailbox-utils: Add device DDR5 ECS control feature
Posted by shiju.jose--- via 8 months, 4 weeks ago
From: Shiju Jose <shiju.jose@huawei.com>

CXL spec 3.1 section 8.2.9.9.11.2 describes the DDR5 Error Check Scrub (ECS)
control feature.

The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
Specification (JESD79-5) and allows the DRAM to internally read, correct
single-bit errors, and write back corrected data bits to the DRAM array
while providing transparency to error counts. The ECS control feature
allows the request to configure ECS input configurations during system
boot or at run-time.

The ECS control allows the requester to change the log entry type, the ECS
threshold count provided that the request is within the definition
specified in DDR5 mode registers, change mode between codeword mode and
row count mode, and reset the ECS counter.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 100 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 908ce16642..2277418c07 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -998,6 +998,7 @@ typedef struct CXLSupportedFeatureEntry {
 
 enum CXL_SUPPORTED_FEATURES_LIST {
     CXL_FEATURE_PATROL_SCRUB = 0,
+    CXL_FEATURE_ECS,
     CXL_FEATURE_MAX
 };
 
@@ -1070,6 +1071,43 @@ typedef struct CXLMemPatrolScrubSetFeature {
 } QEMU_PACKED QEMU_ALIGNED(16) CXLMemPatrolScrubSetFeature;
 static CXLMemPatrolScrubReadAttrs cxl_memdev_ps_feat_attrs;
 
+/*
+ * CXL r3.1 section 8.2.9.9.11.2:
+ * DDR5 Error Check Scrub (ECS) Control Feature
+ */
+static const QemuUUID ecs_uuid = {
+    .data = UUID(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba,
+                 0xb9, 0x69, 0x1e, 0x89, 0x33, 0x86)
+};
+
+#define CXL_ECS_GET_FEATURE_VERSION    0x01
+#define CXL_ECS_SET_FEATURE_VERSION    0x01
+#define CXL_ECS_LOG_ENTRY_TYPE_DEFAULT    0x01
+#define CXL_ECS_REALTIME_REPORT_CAP_DEFAULT    1
+#define CXL_ECS_THRESHOLD_COUNT_DEFAULT    3 /* 3: 256, 4: 1024, 5: 4096 */
+#define CXL_ECS_MODE_DEFAULT    0
+
+#define CXL_ECS_NUM_MEDIA_FRUS   3
+
+/* CXL memdev DDR5 ECS control attributes */
+typedef struct CXLMemECSReadAttrs {
+        uint8_t ecs_log_cap;
+        uint8_t ecs_cap;
+        uint16_t ecs_config;
+        uint8_t ecs_flags;
+} QEMU_PACKED CXLMemECSReadAttrs;
+
+typedef struct CXLMemECSWriteAttrs {
+    uint8_t ecs_log_cap;
+    uint16_t ecs_config;
+} QEMU_PACKED CXLMemECSWriteAttrs;
+
+typedef struct CXLMemECSSetFeature {
+        CXLSetFeatureInHeader hdr;
+        CXLMemECSWriteAttrs feat_data[];
+} QEMU_PACKED QEMU_ALIGNED(16) CXLMemECSSetFeature;
+static CXLMemECSReadAttrs cxl_ecs_feat_attrs[CXL_ECS_NUM_MEDIA_FRUS];
+
 /* CXL r3.1 section 8.2.9.6.1: Get Supported Features (Opcode 0500h) */
 static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
                                              uint8_t *payload_in,
@@ -1088,7 +1126,7 @@ static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
         CXLSupportedFeatureHeader hdr;
         CXLSupportedFeatureEntry feat_entries[];
     } QEMU_PACKED QEMU_ALIGNED(16) * get_feats_out = (void *)payload_out;
-    uint16_t index;
+    uint16_t count, index;
     uint16_t entry, req_entries;
     uint16_t feat_entries = 0;
 
@@ -1130,6 +1168,35 @@ static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
             cxl_memdev_ps_feat_attrs.scrub_flags =
                                 CXL_MEMDEV_PS_ENABLE_DEFAULT;
             break;
+        case  CXL_FEATURE_ECS:
+            /* Fill supported feature entry for device DDR5 ECS control */
+            get_feats_out->feat_entries[entry] =
+                         (struct CXLSupportedFeatureEntry) {
+                .uuid = ecs_uuid,
+                .feat_index = index,
+                .get_feat_size = CXL_ECS_NUM_MEDIA_FRUS *
+                                    sizeof(CXLMemECSReadAttrs),
+                .set_feat_size = CXL_ECS_NUM_MEDIA_FRUS *
+                                    sizeof(CXLMemECSWriteAttrs),
+                .attr_flags = 0x1,
+                .get_feat_version = CXL_ECS_GET_FEATURE_VERSION,
+                .set_feat_version = CXL_ECS_SET_FEATURE_VERSION,
+                .set_feat_effects = 0,
+            };
+            feat_entries++;
+            /* Set default value for DDR5 ECS read attributes */
+            for (count = 0; count < CXL_ECS_NUM_MEDIA_FRUS; count++) {
+                cxl_ecs_feat_attrs[count].ecs_log_cap =
+                                    CXL_ECS_LOG_ENTRY_TYPE_DEFAULT;
+                cxl_ecs_feat_attrs[count].ecs_cap =
+                                    CXL_ECS_REALTIME_REPORT_CAP_DEFAULT;
+                cxl_ecs_feat_attrs[count].ecs_config =
+                                    CXL_ECS_THRESHOLD_COUNT_DEFAULT |
+                                    (CXL_ECS_MODE_DEFAULT << 3);
+                /* Reserved */
+                cxl_ecs_feat_attrs[count].ecs_flags = 0;
+            }
+            break;
         default:
             break;
         }
@@ -1180,6 +1247,18 @@ static CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
         memcpy(payload_out,
                &cxl_memdev_ps_feat_attrs + get_feature->offset,
                bytes_to_copy);
+    } else if (qemu_uuid_is_equal(&get_feature->uuid, &ecs_uuid)) {
+        if (get_feature->offset >=  CXL_ECS_NUM_MEDIA_FRUS *
+                                sizeof(CXLMemECSReadAttrs)) {
+            return CXL_MBOX_INVALID_INPUT;
+        }
+        bytes_to_copy = CXL_ECS_NUM_MEDIA_FRUS *
+                        sizeof(CXLMemECSReadAttrs) -
+                            get_feature->offset;
+        bytes_to_copy = MIN(bytes_to_copy, get_feature->count);
+        memcpy(payload_out,
+               &cxl_ecs_feat_attrs + get_feature->offset,
+               bytes_to_copy);
     } else {
         return CXL_MBOX_UNSUPPORTED;
     }
@@ -1197,8 +1276,11 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
                                            size_t *len_out,
                                            CXLCCI *cci)
 {
+    uint16_t count;
     CXLMemPatrolScrubWriteAttrs *ps_write_attrs;
+    CXLMemECSWriteAttrs *ecs_write_attrs;
     CXLMemPatrolScrubSetFeature *ps_set_feature;
+    CXLMemECSSetFeature *ecs_set_feature;
     CXLSetFeatureInHeader *hdr = (void *)payload_in;
 
     if (qemu_uuid_is_equal(&hdr->uuid, &patrol_scrub_uuid)) {
@@ -1216,6 +1298,22 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
         cxl_memdev_ps_feat_attrs.scrub_flags &= ~0x1;
         cxl_memdev_ps_feat_attrs.scrub_flags |=
                           ps_write_attrs->scrub_flags & 0x1;
+    } else if (qemu_uuid_is_equal(&hdr->uuid,
+                                  &ecs_uuid)) {
+        if (hdr->version != CXL_ECS_SET_FEATURE_VERSION ||
+            (hdr->flags & CXL_SET_FEATURE_FLAG_DATA_TRANSFER_MASK) !=
+                               CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER) {
+            return CXL_MBOX_UNSUPPORTED;
+        }
+
+        ecs_set_feature = (void *)payload_in;
+        ecs_write_attrs = ecs_set_feature->feat_data;
+        for (count = 0; count < CXL_ECS_NUM_MEDIA_FRUS; count++) {
+                cxl_ecs_feat_attrs[count].ecs_log_cap =
+                                  ecs_write_attrs[count].ecs_log_cap;
+                cxl_ecs_feat_attrs[count].ecs_config =
+                                  ecs_write_attrs[count].ecs_config & 0x1F;
+        }
     } else {
         return CXL_MBOX_UNSUPPORTED;
     }
-- 
2.34.1
Re: [PATCH v4 3/3] hw/cxl/cxl-mailbox-utils: Add device DDR5 ECS control feature
Posted by Jonathan Cameron via 8 months, 4 weeks ago
On Mon, 19 Feb 2024 23:00:25 +0800
<shiju.jose@huawei.com> wrote:

> From: Shiju Jose <shiju.jose@huawei.com>
> 
> CXL spec 3.1 section 8.2.9.9.11.2 describes the DDR5 Error Check Scrub (ECS)
> control feature.
> 
> The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
> Specification (JESD79-5) and allows the DRAM to internally read, correct
> single-bit errors, and write back corrected data bits to the DRAM array
> while providing transparency to error counts. The ECS control feature
> allows the request to configure ECS input configurations during system
> boot or at run-time.
> 
> The ECS control allows the requester to change the log entry type, the ECS
> threshold count provided that the request is within the definition
> specified in DDR5 mode registers, change mode between codeword mode and
> row count mode, and reset the ECS counter.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>

Same thing about it being per device, not global. 

Otherwise, just a few minor comments inline.

> ---
>  hw/cxl/cxl-mailbox-utils.c | 100 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 908ce16642..2277418c07 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -998,6 +998,7 @@ typedef struct CXLSupportedFeatureEntry {
>  
>  enum CXL_SUPPORTED_FEATURES_LIST {
>      CXL_FEATURE_PATROL_SCRUB = 0,
> +    CXL_FEATURE_ECS,
>      CXL_FEATURE_MAX
>  };
>  
> @@ -1070,6 +1071,43 @@ typedef struct CXLMemPatrolScrubSetFeature {
>  } QEMU_PACKED QEMU_ALIGNED(16) CXLMemPatrolScrubSetFeature;
>  static CXLMemPatrolScrubReadAttrs cxl_memdev_ps_feat_attrs;
>  
> +/*
> + * CXL r3.1 section 8.2.9.9.11.2:
> + * DDR5 Error Check Scrub (ECS) Control Feature
> + */
> +static const QemuUUID ecs_uuid = {
> +    .data = UUID(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba,
> +                 0xb9, 0x69, 0x1e, 0x89, 0x33, 0x86)
> +};
> +
> +#define CXL_ECS_GET_FEATURE_VERSION    0x01
> +#define CXL_ECS_SET_FEATURE_VERSION    0x01
> +#define CXL_ECS_LOG_ENTRY_TYPE_DEFAULT    0x01
> +#define CXL_ECS_REALTIME_REPORT_CAP_DEFAULT    1
> +#define CXL_ECS_THRESHOLD_COUNT_DEFAULT    3 /* 3: 256, 4: 1024, 5: 4096 */
> +#define CXL_ECS_MODE_DEFAULT    0
> +
> +#define CXL_ECS_NUM_MEDIA_FRUS   3
> +
> +/* CXL memdev DDR5 ECS control attributes */
> +typedef struct CXLMemECSReadAttrs {
> +        uint8_t ecs_log_cap;
> +        uint8_t ecs_cap;
> +        uint16_t ecs_config;
> +        uint8_t ecs_flags;
> +} QEMU_PACKED CXLMemECSReadAttrs;
> +
> +typedef struct CXLMemECSWriteAttrs {
> +    uint8_t ecs_log_cap;
> +    uint16_t ecs_config;
> +} QEMU_PACKED CXLMemECSWriteAttrs;
> +
> +typedef struct CXLMemECSSetFeature {
> +        CXLSetFeatureInHeader hdr;
> +        CXLMemECSWriteAttrs feat_data[];
> +} QEMU_PACKED QEMU_ALIGNED(16) CXLMemECSSetFeature;
> +static CXLMemECSReadAttrs cxl_ecs_feat_attrs[CXL_ECS_NUM_MEDIA_FRUS];

This should be device instance specific.

> +
>  /* CXL r3.1 section 8.2.9.6.1: Get Supported Features (Opcode 0500h) */
>  static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
>                                               uint8_t *payload_in,
> @@ -1088,7 +1126,7 @@ static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
>          CXLSupportedFeatureHeader hdr;
>          CXLSupportedFeatureEntry feat_entries[];
>      } QEMU_PACKED QEMU_ALIGNED(16) * get_feats_out = (void *)payload_out;
> -    uint16_t index;
> +    uint16_t count, index;
>      uint16_t entry, req_entries;
>      uint16_t feat_entries = 0;
>  
> @@ -1130,6 +1168,35 @@ static CXLRetCode cmd_features_get_supported(const struct cxl_cmd *cmd,
>              cxl_memdev_ps_feat_attrs.scrub_flags =
>                                  CXL_MEMDEV_PS_ENABLE_DEFAULT;
>              break;
> +        case  CXL_FEATURE_ECS:
> +            /* Fill supported feature entry for device DDR5 ECS control */
> +            get_feats_out->feat_entries[entry] =
> +                         (struct CXLSupportedFeatureEntry) {
> +                .uuid = ecs_uuid,
> +                .feat_index = index,
> +                .get_feat_size = CXL_ECS_NUM_MEDIA_FRUS *
> +                                    sizeof(CXLMemECSReadAttrs),
> +                .set_feat_size = CXL_ECS_NUM_MEDIA_FRUS *
> +                                    sizeof(CXLMemECSWriteAttrs),
> +                .attr_flags = 0x1,
> +                .get_feat_version = CXL_ECS_GET_FEATURE_VERSION,
> +                .set_feat_version = CXL_ECS_SET_FEATURE_VERSION,
> +                .set_feat_effects = 0,
I think spec says Immediate config change for this one.Plus the CEL bit
should be set (bit 9)

Check this for the other features as well.
 
> +            };
> +            feat_entries++;

Why update this mid setting up the record?  I briefly thought this wrote
two different records (which was odd!)

We don't have gaps in the features - we probably won't ever provide that
degree of control of the QEMU model, so feat_entries will be
req_entries - get_feats_in->start_index
No need to keep a count.

> +            /* Set default value for DDR5 ECS read attributes */
> +            for (count = 0; count < CXL_ECS_NUM_MEDIA_FRUS; count++) {
> +                cxl_ecs_feat_attrs[count].ecs_log_cap =
> +                                    CXL_ECS_LOG_ENTRY_TYPE_DEFAULT;
> +                cxl_ecs_feat_attrs[count].ecs_cap =
> +                                    CXL_ECS_REALTIME_REPORT_CAP_DEFAULT;
> +                cxl_ecs_feat_attrs[count].ecs_config =
> +                                    CXL_ECS_THRESHOLD_COUNT_DEFAULT |
> +                                    (CXL_ECS_MODE_DEFAULT << 3);
> +                /* Reserved */
> +                cxl_ecs_feat_attrs[count].ecs_flags = 0;
> +            }
> +            break;
>          default:
>              break;
>          }
RE: [PATCH v4 3/3] hw/cxl/cxl-mailbox-utils: Add device DDR5 ECS control feature
Posted by Shiju Jose via 8 months, 4 weeks ago
Hi Jonathan,

Thanks for the feedbacks.

>-----Original Message-----
>From: Jonathan Cameron <jonathan.cameron@huawei.com>
>Sent: 19 February 2024 16:59
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: qemu-devel@nongnu.org; linux-cxl@vger.kernel.org; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Linuxarm
><linuxarm@huawei.com>
>Subject: Re: [PATCH v4 3/3] hw/cxl/cxl-mailbox-utils: Add device DDR5 ECS
>control feature
>
>On Mon, 19 Feb 2024 23:00:25 +0800
><shiju.jose@huawei.com> wrote:
>
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> CXL spec 3.1 section 8.2.9.9.11.2 describes the DDR5 Error Check Scrub
>> (ECS) control feature.
>>
>> The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
>> Specification (JESD79-5) and allows the DRAM to internally read,
>> correct single-bit errors, and write back corrected data bits to the
>> DRAM array while providing transparency to error counts. The ECS
>> control feature allows the request to configure ECS input
>> configurations during system boot or at run-time.
>>
>> The ECS control allows the requester to change the log entry type, the
>> ECS threshold count provided that the request is within the definition
>> specified in DDR5 mode registers, change mode between codeword mode
>> and row count mode, and reset the ECS counter.
>>
>> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>> Reviewed-by: Fan Ni <fan.ni@samsung.com>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>
>Same thing about it being per device, not global.
Sure.  I will check this.

>
>Otherwise, just a few minor comments inline.
>
>> ---
>>  hw/cxl/cxl-mailbox-utils.c | 100
>> ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 99 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 908ce16642..2277418c07 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -998,6 +998,7 @@ typedef struct CXLSupportedFeatureEntry {
>>
>>  enum CXL_SUPPORTED_FEATURES_LIST {
>>      CXL_FEATURE_PATROL_SCRUB = 0,
>> +    CXL_FEATURE_ECS,
>>      CXL_FEATURE_MAX
>>  };
>>
>> @@ -1070,6 +1071,43 @@ typedef struct CXLMemPatrolScrubSetFeature {  }
>> QEMU_PACKED QEMU_ALIGNED(16) CXLMemPatrolScrubSetFeature;  static
>> CXLMemPatrolScrubReadAttrs cxl_memdev_ps_feat_attrs;
>>
>> +/*
>> + * CXL r3.1 section 8.2.9.9.11.2:
>> + * DDR5 Error Check Scrub (ECS) Control Feature  */ static const
>> +QemuUUID ecs_uuid = {
>> +    .data = UUID(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba,
>> +                 0xb9, 0x69, 0x1e, 0x89, 0x33, 0x86) };
>> +
>> +#define CXL_ECS_GET_FEATURE_VERSION    0x01
>> +#define CXL_ECS_SET_FEATURE_VERSION    0x01
>> +#define CXL_ECS_LOG_ENTRY_TYPE_DEFAULT    0x01
>> +#define CXL_ECS_REALTIME_REPORT_CAP_DEFAULT    1
>> +#define CXL_ECS_THRESHOLD_COUNT_DEFAULT    3 /* 3: 256, 4: 1024, 5:
>4096 */
>> +#define CXL_ECS_MODE_DEFAULT    0
>> +
>> +#define CXL_ECS_NUM_MEDIA_FRUS   3
>> +
>> +/* CXL memdev DDR5 ECS control attributes */ typedef struct
>> +CXLMemECSReadAttrs {
>> +        uint8_t ecs_log_cap;
>> +        uint8_t ecs_cap;
>> +        uint16_t ecs_config;
>> +        uint8_t ecs_flags;
>> +} QEMU_PACKED CXLMemECSReadAttrs;
>> +
>> +typedef struct CXLMemECSWriteAttrs {
>> +    uint8_t ecs_log_cap;
>> +    uint16_t ecs_config;
>> +} QEMU_PACKED CXLMemECSWriteAttrs;
>> +
>> +typedef struct CXLMemECSSetFeature {
>> +        CXLSetFeatureInHeader hdr;
>> +        CXLMemECSWriteAttrs feat_data[]; } QEMU_PACKED
>> +QEMU_ALIGNED(16) CXLMemECSSetFeature; static CXLMemECSReadAttrs
>> +cxl_ecs_feat_attrs[CXL_ECS_NUM_MEDIA_FRUS];
>
>This should be device instance specific.
Ok.

>
>> +
>>  /* CXL r3.1 section 8.2.9.6.1: Get Supported Features (Opcode 0500h)
>> */  static CXLRetCode cmd_features_get_supported(const struct cxl_cmd
>*cmd,
>>                                               uint8_t *payload_in, @@
>> -1088,7 +1126,7 @@ static CXLRetCode cmd_features_get_supported(const
>struct cxl_cmd *cmd,
>>          CXLSupportedFeatureHeader hdr;
>>          CXLSupportedFeatureEntry feat_entries[];
>>      } QEMU_PACKED QEMU_ALIGNED(16) * get_feats_out = (void
>*)payload_out;
>> -    uint16_t index;
>> +    uint16_t count, index;
>>      uint16_t entry, req_entries;
>>      uint16_t feat_entries = 0;
>>
>> @@ -1130,6 +1168,35 @@ static CXLRetCode
>cmd_features_get_supported(const struct cxl_cmd *cmd,
>>              cxl_memdev_ps_feat_attrs.scrub_flags =
>>                                  CXL_MEMDEV_PS_ENABLE_DEFAULT;
>>              break;
>> +        case  CXL_FEATURE_ECS:
>> +            /* Fill supported feature entry for device DDR5 ECS control */
>> +            get_feats_out->feat_entries[entry] =
>> +                         (struct CXLSupportedFeatureEntry) {
>> +                .uuid = ecs_uuid,
>> +                .feat_index = index,
>> +                .get_feat_size = CXL_ECS_NUM_MEDIA_FRUS *
>> +                                    sizeof(CXLMemECSReadAttrs),
>> +                .set_feat_size = CXL_ECS_NUM_MEDIA_FRUS *
>> +                                    sizeof(CXLMemECSWriteAttrs),
>> +                .attr_flags = 0x1,
>> +                .get_feat_version = CXL_ECS_GET_FEATURE_VERSION,
>> +                .set_feat_version = CXL_ECS_SET_FEATURE_VERSION,
>> +                .set_feat_effects = 0,
>I think spec says Immediate config change for this one.Plus the CEL bit should be
>set (bit 9)
Sure. I will change. 
 
>
>Check this for the other features as well.
>
>> +            };
>> +            feat_entries++;
>
>Why update this mid setting up the record?  I briefly thought this wrote two
>different records (which was odd!)
>
>We don't have gaps in the features - we probably won't ever provide that degree
>of control of the QEMU model, so feat_entries will be req_entries -
>get_feats_in->start_index No need to keep a count.
Yes. You are right.

>
>> +            /* Set default value for DDR5 ECS read attributes */
>> +            for (count = 0; count < CXL_ECS_NUM_MEDIA_FRUS; count++) {
>> +                cxl_ecs_feat_attrs[count].ecs_log_cap =
>> +                                    CXL_ECS_LOG_ENTRY_TYPE_DEFAULT;
>> +                cxl_ecs_feat_attrs[count].ecs_cap =
>> +                                    CXL_ECS_REALTIME_REPORT_CAP_DEFAULT;
>> +                cxl_ecs_feat_attrs[count].ecs_config =
>> +                                    CXL_ECS_THRESHOLD_COUNT_DEFAULT |
>> +                                    (CXL_ECS_MODE_DEFAULT << 3);
>> +                /* Reserved */
>> +                cxl_ecs_feat_attrs[count].ecs_flags = 0;
>> +            }
>> +            break;
>>          default:
>>              break;
>>          }

Thanks,
Shiju