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

[Jonathan Cameron]

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