Message-ID: <20250307091137.00006a0a@huawei.com>
Date: Fri, 7 Mar 2025 09:11:37 +0800
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <shiju.jose@...wei.com>
CC: <linux-cxl@...r.kernel.org>, <dan.j.williams@...el.com>,
	<dave@...olabs.net>, <dave.jiang@...el.com>, <alison.schofield@...el.com>,
	<vishal.l.verma@...el.com>, <ira.weiny@...el.com>, <david@...hat.com>,
	<Vilas.Sridharan@....com>, <linux-edac@...r.kernel.org>,
	<linux-acpi@...r.kernel.org>, <linux-mm@...ck.org>,
	<linux-kernel@...r.kernel.org>, <bp@...en8.de>, <tony.luck@...el.com>,
	<rafael@...nel.org>, <lenb@...nel.org>, <mchehab@...nel.org>,
	<leo.duran@....com>, <Yazen.Ghannam@....com>, <rientjes@...gle.com>,
	<jiaqiyan@...gle.com>, <Jon.Grimm@....com>, <dave.hansen@...ux.intel.com>,
	<naoya.horiguchi@....com>, <james.morse@....com>, <jthoughton@...gle.com>,
	<somasundaram.a@....com>, <erdemaktas@...gle.com>, <pgonda@...gle.com>,
	<duenwen@...gle.com>, <gthelen@...gle.com>, <wschwartz@...erecomputing.com>,
	<dferguson@...erecomputing.com>, <wbs@...amperecomputing.com>,
	<nifan.cxl@...il.com>, <tanxiaofei@...wei.com>, <prime.zeng@...ilicon.com>,
	<roberto.sassu@...wei.com>, <kangkang.shen@...urewei.com>,
	<wanghuiqiang@...wei.com>, <linuxarm@...wei.com>
Subject: Re: [PATCH 8/8] cxl/memfeature: Add CXL memory device memory
 sparing control feature

On Thu, 27 Feb 2025 22:38:15 +0000
<shiju.jose@...wei.com> wrote:

> From: Shiju Jose <shiju.jose@...wei.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.
>  - If the previous case is not enough, may be when the number of corrected
>    error reported on a CXL.mem media to the user space exceeds an error
>    threshold set in 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.
> 
> Tested for memory sparing control feature with
>    "hw/cxl: Add memory sparing control feature"
>    Repository: "https://gitlab.com/shiju.jose/qemu.git"
>    Branch: cxl-ras-features-2024-10-24
> 
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
Similar comment to earlier on maybe using single line comments
in more places rather than multiline.  Perhaps worth doing
that if you are respinning for other reasons.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

> +static int cxl_mem_do_sparing_op(struct device *dev,
> +				 struct cxl_mem_sparing_context *cxl_sparing_ctx,
> +				 struct cxl_memdev_sparing_params *rd_params)
> +{
> +	struct cxl_memdev *cxlmd = cxl_sparing_ctx->cxlmd;
> +	struct cxl_memdev_sparing_in_payload sparing_pi;
> +	struct cxl_event_dram *rec = NULL;
> +	u16 validity_flags = 0;
> +
> +	if (!rd_params->cap_safe_when_in_use) {
> +		/*
> +		 * Memory to repair must be offline
> +		 */
> +		if (cxl_are_decoders_committed(cxlmd))
> +			return -EBUSY;
> +		/*
> +		 * offline, so good for repair
> +		 */
More places as below where a single line comment would be fine
and make a reader scroll a bit less.
> +static int cxl_memdev_sparing_init(struct cxl_memdev *cxlmd,
> +				   struct edac_dev_feature *ras_feature,
> +				   const struct cxl_mem_sparing_desc *desc,
> +				   u8 repair_inst)
> +{
> +	struct cxl_mem_sparing_context *cxl_sparing_ctx;
> +	struct cxl_memdev_sparing_params rd_params;
> +	struct cxl_feat_entry *feat_entry;
> +	int ret;
> +
> +	feat_entry = cxl_get_feature_entry(cxlmd->cxlds, &desc->repair_uuid);
> +	if (IS_ERR(feat_entry))
> +		return -EOPNOTSUPP;
> +
> +	if (!(le32_to_cpu(feat_entry->flags) & CXL_FEATURE_F_CHANGEABLE))
> +		return -EOPNOTSUPP;
> +
> +	cxl_sparing_ctx = devm_kzalloc(&cxlmd->dev, sizeof(*cxl_sparing_ctx),
> +				       GFP_KERNEL);
> +	if (!cxl_sparing_ctx)
> +		return -ENOMEM;
> +
> +	*cxl_sparing_ctx = (struct cxl_mem_sparing_context) {
> +		.get_feat_size = le16_to_cpu(feat_entry->get_feat_size),
> +		.set_feat_size = le16_to_cpu(feat_entry->set_feat_size),
> +		.get_version = feat_entry->get_feat_ver,
> +		.set_version = feat_entry->set_feat_ver,
> +		.effects = le16_to_cpu(feat_entry->effects),
> +		.cxlmd = cxlmd,
> +		.repair_type = desc->repair_type,
> +		.granularity = desc->granularity,
> +		.instance = repair_inst++,
> +	};
> +	uuid_copy(&cxl_sparing_ctx->repair_uuid, &desc->repair_uuid);
> +
> +	/*
> +	 * Read CXL device's sparing capabilities.
a below.
> +	 */
> +	ret = cxl_mem_sparing_get_attrs(cxl_sparing_ctx, &rd_params);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Set default value for persist_mode.
> +	 */

If respining some of these comments don't need to be multiline.

Powered by blists - more mailing lists