Hi Jonathan,
Thanks for the feedbacks.
>-----Original Message-----
>From: Jonathan Cameron <jonathan.came...@huawei.com>
>Sent: 19 February 2024 16:59
>To: Shiju Jose <shiju.j...@huawei.com>
>Cc: qemu-devel@nongnu.org; linux-...@vger.kernel.org; tanxiaofei
><tanxiao...@huawei.com>; Zengtao (B) <prime.z...@hisilicon.com>; Linuxarm
><linux...@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.j...@huawei.com> wrote:
>
>> From: Shiju Jose <shiju.j...@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 <d...@stgolabs.net>
>> Reviewed-by: Fan Ni <fan...@samsung.com>
>> Signed-off-by: Shiju Jose <shiju.j...@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
