[v4,00/11] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features

Message

Shiju Jose Nov. 30, 2023, 7:23 p.m. UTC

From: Shiju Jose <[e-mail]>

1. Add support for CXL feature mailbox commands.
2. Add CXL device scrub driver supporting patrol scrub control and DDR5 ECS
control features.
3. Add scrub driver supports configuring memory scrubs in the system.
4. Add scrub attributes for DDR5 ECS control to the memory scrub driver.
5. Register CXL device patrol scrub and ECS with scrub control driver.
6. Add documentation for common attributes in the scrub configure driver.
7. Add documentation for CXL memory device scrub control attributes.

Changes
v3 -> v4:
1. Fixes for the warnings/errors reported by kernel test robot.
2. Add support for reading the 'enable' attribute of CXL patrol scrub.

Changes
v2 -> v3:
1. Changes for comments from Davidlohr, Thanks.
 - Updated cxl scrub kconfig
 - removed usage of the flag is_support_feature from
   the function cxl_mem_get_supported_feature_entry().
 - corrected spelling error.
 - removed unnecessary debug message.
 - removed export feature commands to the userspace.
2. Possible fix for the warnings/errors reported by kernel
   test robot.
3. Add documentation for the common scrub configure atrributes.

v1 -> v2:
1. Changes for comments from Dave Jiang, Thanks.
 - Split patches.
 - reversed xmas tree declarations.
 - declared flags as enums.
 - removed few unnecessary variable initializations.
 - replaced PTR_ERR_OR_ZERO() with IS_ERR() and PTR_ERR().
 - add auto clean declarations.
 - replaced while loop with for loop.
 - Removed allocation from cxl_get_supported_features() and
   cxl_get_feature() and make change to take allocated memory
   pointer from the caller.
 - replaced if/else with switch case.
 - replaced sprintf() with sysfs_emit() in 2 places.
 - replaced goto label with return in few functions.
2. removed unused code for supported attributes from ecs.
3. Included following common patch for scrub configure driver
   to this series.
   "memory: scrub: Add scrub driver supports configuring memory scrubbers
    in the system"

Shiju Jose (11):
  cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command
  cxl/mbox: Add GET_FEATURE mailbox command
  cxl/mbox: Add SET_FEATURE mailbox command
  cxl/memscrub: Add CXL device patrol scrub control feature
  cxl/memscrub: Add CXL device DDR5 ECS control feature
  memory: scrub: Add scrub driver supports configuring memory scrubs in
    the system
  cxl/memscrub: Register CXL device patrol scrub with scrub configure
    driver
  memory: scrub: Add scrub control attributes for the DDR5 ECS
  cxl/memscrub: Register CXL device DDR5 ECS with scrub configure driver
  memory: scrub: sysfs: Add Documentation for set of common scrub
    attributes
  cxl: scrub: sysfs: Add Documentation for CXL memory device scrub
    control attributes

 .../testing/sysfs-class-cxl-scrub-configure   |   79 ++
 .../ABI/testing/sysfs-class-scrub-configure   |   82 ++
 drivers/cxl/Kconfig                           |   23 +
 drivers/cxl/core/Makefile                     |    1 +
 drivers/cxl/core/mbox.c                       |   59 +
 drivers/cxl/core/memscrub.c                   | 1002 +++++++++++++++++
 drivers/cxl/cxlmem.h                          |  120 ++
 drivers/cxl/pci.c                             |    6 +
 drivers/memory/Kconfig                        |    1 +
 drivers/memory/Makefile                       |    1 +
 drivers/memory/scrub/Kconfig                  |   11 +
 drivers/memory/scrub/Makefile                 |    6 +
 drivers/memory/scrub/memory-scrub.c           |  481 ++++++++
 include/memory/memory-scrub.h                 |   90 ++
 14 files changed, 1962 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-cxl-scrub-configure
 create mode 100644 Documentation/ABI/testing/sysfs-class-scrub-configure
 create mode 100644 drivers/cxl/core/memscrub.c
 create mode 100644 drivers/memory/scrub/Kconfig
 create mode 100644 drivers/memory/scrub/Makefile
 create mode 100755 drivers/memory/scrub/memory-scrub.c
 create mode 100755 include/memory/memory-scrub.h

Comments

Dan Williams Dec. 6, 2023, 7:38 p.m. UTC | #1

Hi Shiju,

I have some general feedback at this point before digging too deep into
the details:

shiju.jose@ wrote:
> From: Shiju Jose <[e-mail]>
> 
> 1. Add support for CXL feature mailbox commands.
> 2. Add CXL device scrub driver supporting patrol scrub control and DDR5 ECS
> control features.
> 3. Add scrub driver supports configuring memory scrubs in the system.
> 4. Add scrub attributes for DDR5 ECS control to the memory scrub driver.

For a new a subsystem that is meant to generically abstract a "memory
scrub" facility the "DDR5 ECS" naming has too much precision. How much
of this interface is DDR5 ECS specific and how much of it is applicable
to a theoretical DDR6 scrub implementation?

My primary reaction is to boil down this interface so that only generic
scrub details are visible in the ABI, and DDR5 specifics are invisible
in the sysfs ABI.

For example the Linux NVDIMM subsystem has an address-range-scrub
facility that is independent of the specific memory technology scrub
mechanism. That one is based on ACPI NFIT, but I realize you also looked
at enabling the ACPI RASF scrub interface. It would be useful if this
patchset could plausibly enable one non-CXL scrubber along with the CXL
one.

> 5. Register CXL device patrol scrub and ECS with scrub control driver.
> 6. Add documentation for common attributes in the scrub configure driver.

Going forward, please include the Documentation in the patch that adds
the new ABI, it improves the readability / story-telling of the patches.
It also makes it easier to analyze which code is needed for which ABI,
and whether a given ABI is justified.

The regionY nomenclature in the sysfs ABI looks like a potential
opportunity to align with the "memregion" id scheme. See all the callers
of memregion_alloc() where those are tagging device-backed physical
address ranges with a common id namespace. It would be great if the
memory-scrub ABI reported failures in terms of region-ids that correlate
with CXL, DAX, or NVDIMM regions.

> 7. Add documentation for CXL memory device scrub control attributes.

Do the CXL specifics need to be in the ABI? One thing I missed was how
the series of log entries are conveyed. For CXL in contrast to what
NVDIMM did for address range scrub is that CXL makes use of trace-events
to emit log records. That allows the sysfs ABI to remain relatively
simple, but the various trace-events can get into more protocol specific
details. For example, see cxl_trigger_poison_list() and
trace_cxl_poison() as a way to genericly trigger the listing of a flow
of device-specific details. In other words put the DDR5 ECS specifics in
the trace-event, not the sysfs ABI if possible.

Lastly, dynamically defined sysfs groups are less palatable than
statically defined. See cxl_region_target_visible() for a scheme for
statically defining a runtime variable number of attributes.
Specifically I would like to see a way to define this new subsystem
without scrub_create_attrs() and all the runtime attribute definition.

Overall, I like the general approach to define a common subsystem for
this, and get off the treadmill of reinventing custom scrub interfaces
per bus, but that also requires that it be generic enough to subsume a
number of those per-bus-scrub-types.

Shiju Jose Dec. 8, 2023, 1:48 p.m. UTC | #2

Hi Dan,

Thanks  for the feedbacks.

>-----Original Message-----
>From: Dan Williams <[e-mail]>
>Sent: 06 December 2023 19:39
>To: Shiju Jose <[e-mail]>; [e-mail]; linux-
>[e-mail]; [e-mail]; Jonathan Cameron
><[e-mail]>; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]
>Cc: [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail];
>[e-mail]; [e-mail];
>tanxiaofei <[e-mail]>; Zengtao (B) <[e-mail]>;
>[e-mail]; wanghuiqiang <[e-mail]>;
>Linuxarm <[e-mail]>; Shiju Jose <[e-mail]>
>Subject: RE: [PATCH v4 00/11] cxl: Add support for CXL feature commands, CXL
>device patrol scrub control and DDR5 ECS control features
>
>Hi Shiju,
>
>I have some general feedback at this point before digging too deep into the
>details:
>
>shiju.jose@ wrote:
>> From: Shiju Jose <[e-mail]>
>>
>> 1. Add support for CXL feature mailbox commands.
>> 2. Add CXL device scrub driver supporting patrol scrub control and
>> DDR5 ECS control features.
>> 3. Add scrub driver supports configuring memory scrubs in the system.
>> 4. Add scrub attributes for DDR5 ECS control to the memory scrub driver.
>
>For a new a subsystem that is meant to generically abstract a "memory scrub"
>facility the "DDR5 ECS" naming has too much precision. How much of this
>interface is DDR5 ECS specific and how much of it is applicable to a theoretical
>DDR6 scrub implementation?
>
>My primary reaction is to boil down this interface so that only generic scrub
>details are visible in the ABI, and DDR5 specifics are invisible in the sysfs ABI.
Sure. I will check this.

>
>For example the Linux NVDIMM subsystem has an address-range-scrub facility
>that is independent of the specific memory technology scrub mechanism. That
>one is based on ACPI NFIT, but I realize you also looked at enabling the ACPI
>RASF scrub interface. It would be useful if this patchset could plausibly enable
>one non-CXL scrubber along with the CXL one.
Sure. I will do this. I will add ACPI RASF scrub patches to this patch set.

>
>> 5. Register CXL device patrol scrub and ECS with scrub control driver.
>> 6. Add documentation for common attributes in the scrub configure driver.
>
>Going forward, please include the Documentation in the patch that adds the new
>ABI, it improves the readability / story-telling of the patches.
>It also makes it easier to analyze which code is needed for which ABI, and
>whether a given ABI is justified.
I will do.

>
>The regionY nomenclature in the sysfs ABI looks like a potential opportunity to
>align with the "memregion" id scheme. See all the callers of memregion_alloc()
>where those are tagging device-backed physical address ranges with a common
>id namespace. It would be great if the memory-scrub ABI reported failures in
>terms of region-ids that correlate with CXL, DAX, or NVDIMM regions.
For the CXL DDR5 ECS feature, presently the regionY  corresponds to the IDs of the
memory media FRUs (1 to N),  defined in the DDR5 ECS Control Feature tables Table 8-210 and  Table 8-211. 
 
>
>> 7. Add documentation for CXL memory device scrub control attributes.
>
>Do the CXL specifics need to be in the ABI? One thing I missed was how the
Ok. I will remove. Should these DDR5 ECS specifics consider as generic and 
be part of the memory scrub ABI?      
>series of log entries are conveyed. For CXL in contrast to what NVDIMM did for
>address range scrub is that CXL makes use of trace-events to emit log records.
>That allows the sysfs ABI to remain relatively simple, but the various trace-
>events can get into more protocol specific details. For example, see
>cxl_trigger_poison_list() and
>trace_cxl_poison() as a way to genericly trigger the listing of a flow of device-
>specific details. In other words put the DDR5 ECS specifics in the trace-event, not
>the sysfs ABI if possible.
Did you mean remove the readable attributes for DDR5 ECS from the sysfs?
For example the "ECS Threshold Count per Gb of Memory Cells" and "Codeword/Row Count Mode"
in the Table 8-78 DDR5 ECS log  of  section 8.2.9.5.2.4 DDR5 Error Check Scrub (ECS) Log.
  
>
>Lastly, dynamically defined sysfs groups are less palatable than statically
>defined. See cxl_region_target_visible() for a scheme for statically defining a
>runtime variable number of attributes.
>Specifically I would like to see a way to define this new subsystem without
>scrub_create_attrs() and all the runtime attribute definition.
>
Sure. I will check this.

>Overall, I like the general approach to define a common subsystem for this, and
>get off the treadmill of reinventing custom scrub interfaces per bus, but that
>also requires that it be generic enough to subsume a number of those per-bus-
>scrub-types.
This is the challenging part to make the scrub interface generic because the scrub control
varies between the scrub types, for example as seen in the CXL ECS.

Thanks,
Shiju