Re: [PATCH v6 7/8] cxl/edac: Add CXL memory device memory sparing control feature

[Dan Williams <dan.j.williams@intel.com>]

shiju.jose@ wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Memory sparing is defined as a repair function that replaces a portion of
> memory with a portion of functional memory at that same DPA. The subclasses
> for this operation vary in terms of the scope of the sparing being
> performed. The cacheline sparing subclass refers to a sparing action that
> can replace a full cacheline. Row sparing is provided as an alternative to
> PPR sparing functions and its scope is that of a single DDR row.
> As per CXL r3.2 Table 8-125 foot note 1. Memory sparing is preferred over
> PPR when possible.
> Bank sparing allows an entire bank to be replaced. Rank sparing is defined
> as an operation in which an entire DDR rank is replaced.
> 
> Memory sparing maintenance operations may be supported by CXL devices
> that implement CXL.mem protocol. A sparing maintenance operation requests
> the CXL device to perform a repair operation on its media.
> For example, a CXL device with DRAM components that support memory sparing
> features may implement sparing maintenance operations.
> 
> The host may issue a query command by setting query resources flag in the
> input payload (CXL spec 3.2 Table 8-120) to determine availability of
> sparing resources for a given address. In response to a query request,
> the device shall report the resource availability by producing the memory
> sparing event record (CXL spec 3.2 Table 8-60) in which the Channel, Rank,
> Nibble Mask, Bank Group, Bank, Row, Column, Sub-Channel fields are a copy
> of the values specified in the request.
> 
> During the execution of a sparing maintenance operation, a CXL memory
> device:
> - may not retain data
> - may not be able to process CXL.mem requests correctly.
> These CXL memory device capabilities are specified by restriction flags
> in the memory sparing feature readable attributes.
> 
> When a CXL device identifies error on a memory component, the device
> may inform the host about the need for a memory sparing maintenance
> operation by using DRAM event record, where the 'maintenance needed' flag
> may set. The event record contains some of the DPA, Channel, Rank,
> Nibble Mask, Bank Group, Bank, Row, Column, Sub-Channel fields that
> should be repaired. The userspace tool requests for maintenance operation
> if the 'maintenance needed' flag set in the CXL DRAM error record.
> 
> CXL spec 3.2 section 8.2.10.7.1.4 describes the device's memory sparing
> maintenance operation feature.
> 
> CXL spec 3.2 section 8.2.10.7.2.3 describes the memory sparing feature
> discovery and configuration.
> 
> Add support for controlling CXL memory device memory sparing feature.
> Register with EDAC driver, which gets the memory repair attr descriptors
> from the EDAC memory repair driver and exposes sysfs repair control
> attributes for memory sparing to the userspace. For example CXL memory
> sparing control for the CXL mem0 device is exposed in
> /sys/bus/edac/devices/cxl_mem0/mem_repairX/
> 
> Use case
> ========
> 1. CXL device identifies a failure in a memory component, report to
>    userspace in a CXL DRAM trace event with DPA and other attributes of
>    memory to repair such as channel, rank, nibble mask, bank Group,
>    bank, row, column, sub-channel.
> 
> 2. Rasdaemon process the trace event and may issue query request in sysfs
> check resources available for memory sparing if either of the following
> conditions met.
>  - 'maintenance needed' flag set in the event record.
>  - 'threshold event' flag set for CVME threshold feature.
>  - When the number of corrected error reported on a CXL.mem media to the
>    userspace exceeds the threshold value for corrected error count defined
>    by the userspace policy.
> 
> 3. Rasdaemon process the memory sparing trace event and issue repair
>    request for memory sparing.
> 
> Kernel CXL driver shall report memory sparing event record to the userspace
> with the resource availability in order rasdaemon to process the event
> record and issue a repair request in sysfs for the memory sparing operation
> in the CXL device.
> 
> Note: Based on the feedbacks from the community 'query' sysfs attribute is
> removed and reporting memory sparing error record to the userspace are not
> supported. Instead userspace issues sparing operation and kernel does the
> same to the CXL memory device, when 'maintenance needed' flag set in the
> DRAM event record.
> 
> Add checks to ensure the memory to be repaired is offline and if online,
> then originates from a CXL DRAM error record reported in the current boot
> before requesting a memory sparing operation on the device.
> 
> Note: Tested memory sparing feature control with QEMU patch
>       "hw/cxl: Add emulation for memory sparing control feature"
>       https://lore.kernel.org/linux-cxl/20250509172229.726-1-shiju.jose@huawei.com/T/#m5f38512a95670d75739f9dad3ee91b95c7f5c8d6
> 
> [dj: Move cxl_is_memdev_memory_online() before its caller. (Alison)]
> [dj: Check return from cxl_feature_info() with IS_ERR]

I would love for more of this changelog to make it into the
documentation, but that can be a follow-up. For example the policy
described by:

"Add checks to ensure the memory to be repaired is offline and if online,
 then originates from a CXL DRAM error record reported in the current boot
 before requesting a memory sparing operation on the device."

...is important information for the interface, but that can arrive in a
follow-on change.

It should probably also clarify the data consistency and access latency
impacts of the repair. Like it is a hardware bug if data changes over
the repair event, and to consult product documnetation about the latency
of repair.


> +static int cxl_mem_sparing_get_repair_type(struct device *dev, void *drv_data,
> +					   const char **repair_type)

A lot of my unease with this patch arises from the abandonment of
type-safety in all these callbacks... but that ship has sailed
at this point so that unease will need to be addressed as a follow-on,
if ever.

I also think the edac_dev_register() scheme and its usage of drvdata
outside of a driver context looks odd, i.e. the normal expectations about the
device_lock() relative to sysfs attribute visibility can not be applied.
However, nothing looks obviously broken, so:

Acked-by: Dan Williams <dan.j.williams@intel.com>