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.
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;
> }
