| Message ID | 20240219150025.1531-4-shiju.jose@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/cxl/cxl-mailbox-utils: Add feature commands, device patrol scrub control and DDR5 ECS control features | expand |
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; > }
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
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; }