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

[Shiju Jose <shiju.jose@huawei.com>]

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