lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <35faf0e5-9f54-44e8-ae65-ce1dc91b9cbd@intel.com>
Date: Wed, 30 Oct 2024 09:46:16 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Shiju Jose <shiju.jose@...wei.com>,
 "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
 "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
 "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
 "linux-mm@...ck.org" <linux-mm@...ck.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "bp@...en8.de" <bp@...en8.de>, "tony.luck@...el.com" <tony.luck@...el.com>,
 "rafael@...nel.org" <rafael@...nel.org>, "lenb@...nel.org"
 <lenb@...nel.org>, "mchehab@...nel.org" <mchehab@...nel.org>,
 "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
 "dave@...olabs.net" <dave@...olabs.net>,
 "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
 "sudeep.holla@....com" <sudeep.holla@....com>,
 "jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
 "alison.schofield@...el.com" <alison.schofield@...el.com>,
 "vishal.l.verma@...el.com" <vishal.l.verma@...el.com>,
 "ira.weiny@...el.com" <ira.weiny@...el.com>,
 "david@...hat.com" <david@...hat.com>,
 "Vilas.Sridharan@....com" <Vilas.Sridharan@....com>,
 "leo.duran@....com" <leo.duran@....com>,
 "Yazen.Ghannam@....com" <Yazen.Ghannam@....com>,
 "rientjes@...gle.com" <rientjes@...gle.com>,
 "jiaqiyan@...gle.com" <jiaqiyan@...gle.com>,
 "Jon.Grimm@....com" <Jon.Grimm@....com>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "naoya.horiguchi@....com" <naoya.horiguchi@....com>,
 "james.morse@....com" <james.morse@....com>,
 "jthoughton@...gle.com" <jthoughton@...gle.com>,
 "somasundaram.a@....com" <somasundaram.a@....com>,
 "erdemaktas@...gle.com" <erdemaktas@...gle.com>,
 "pgonda@...gle.com" <pgonda@...gle.com>,
 "duenwen@...gle.com" <duenwen@...gle.com>,
 "gthelen@...gle.com" <gthelen@...gle.com>,
 "wschwartz@...erecomputing.com" <wschwartz@...erecomputing.com>,
 "dferguson@...erecomputing.com" <dferguson@...erecomputing.com>,
 "wbs@...amperecomputing.com" <wbs@...amperecomputing.com>,
 "nifan.cxl@...il.com" <nifan.cxl@...il.com>,
 tanxiaofei <tanxiaofei@...wei.com>, "Zengtao (B)"
 <prime.zeng@...ilicon.com>, Roberto Sassu <roberto.sassu@...wei.com>,
 "kangkang.shen@...urewei.com" <kangkang.shen@...urewei.com>,
 wanghuiqiang <wanghuiqiang@...wei.com>, Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH v14 07/14] cxl/memfeature: Add CXL memory device patrol
 scrub control feature



On 10/30/24 9:16 AM, Jonathan Cameron wrote:
> On Tue, 29 Oct 2024 11:32:47 -0700
> Dave Jiang <dave.jiang@...el.com> wrote:
> 
>> On 10/29/24 10:00 AM, Shiju Jose wrote:
>>>
>>>   
>>>> -----Original Message-----
>>>> From: Dave Jiang <dave.jiang@...el.com>
>>>> Sent: 29 October 2024 16:32
>>>> To: Shiju Jose <shiju.jose@...wei.com>; linux-edac@...r.kernel.org; linux-
>>>> cxl@...r.kernel.org; linux-acpi@...r.kernel.org; linux-mm@...ck.org; linux-
>>>> kernel@...r.kernel.org
>>>> Cc: bp@...en8.de; tony.luck@...el.com; rafael@...nel.org; lenb@...nel.org;
>>>> mchehab@...nel.org; dan.j.williams@...el.com; dave@...olabs.net; Jonathan
>>>> Cameron <jonathan.cameron@...wei.com>; gregkh@...uxfoundation.org;
>>>> sudeep.holla@....com; jassisinghbrar@...il.com; alison.schofield@...el.com;
>>>> vishal.l.verma@...el.com; ira.weiny@...el.com; david@...hat.com;
>>>> Vilas.Sridharan@....com; 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 <tanxiaofei@...wei.com>; Zengtao (B)
>>>> <prime.zeng@...ilicon.com>; Roberto Sassu <roberto.sassu@...wei.com>;
>>>> kangkang.shen@...urewei.com; wanghuiqiang <wanghuiqiang@...wei.com>;
>>>> Linuxarm <linuxarm@...wei.com>
>>>> Subject: Re: [PATCH v14 07/14] cxl/memfeature: Add CXL memory device patrol
>>>> scrub control feature
>>>>
>>>>
>>>>
>>>> On 10/25/24 10:13 AM, shiju.jose@...wei.com wrote:  
>>>>> From: Shiju Jose <shiju.jose@...wei.com>
>>>>>
>>>>> CXL spec 3.1 section 8.2.9.9.11.1 describes the device patrol scrub
>>>>> control feature. The device patrol scrub proactively locates and makes
>>>>> corrections to errors in regular cycle.
>>>>>
>>>>> Allow specifying the number of hours within which the patrol scrub
>>>>> must be completed, subject to minimum and maximum limits reported by the  
>>>> device.  
>>>>> Also allow disabling scrub allowing trade-off error rates against
>>>>> performance.
>>>>>
>>>>> Add support for patrol scrub control on CXL memory devices.
>>>>> Register with the EDAC device driver, which retrieves the scrub
>>>>> attribute descriptors from EDAC scrub and exposes the sysfs scrub
>>>>> control attributes to userspace. For example, scrub control for the
>>>>> CXL memory device "cxl_mem0" is exposed in  
>>>> /sys/bus/edac/devices/cxl_mem0/scrubX/.  
>>>>>
>>>>> Additionally, add support for region-based CXL memory patrol scrub control.
>>>>> CXL memory regions may be interleaved across one or more CXL memory
>>>>> devices. For example, region-based scrub control for "cxl_region1" is
>>>>> exposed in /sys/bus/edac/devices/cxl_region1/scrubX/.
>>>>>
>>>>> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>>>>> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
>>>>> ---
>>>>>  Documentation/edac/edac-scrub.rst |  74 ++++++
>>>>>  drivers/cxl/Kconfig               |  18 ++
>>>>>  drivers/cxl/core/Makefile         |   1 +
>>>>>  drivers/cxl/core/memfeature.c     | 381 ++++++++++++++++++++++++++++++
>>>>>  drivers/cxl/core/region.c         |   6 +
>>>>>  drivers/cxl/cxlmem.h              |   7 +
>>>>>  drivers/cxl/mem.c                 |   4 +
>>>>>  7 files changed, 491 insertions(+)
>>>>>  create mode 100644 Documentation/edac/edac-scrub.rst  create mode
>>>>> 100644 drivers/cxl/core/memfeature.c
>>>>>
>>>>> diff --git a/Documentation/edac/edac-scrub.rst
>>>>> b/Documentation/edac/edac-scrub.rst
>>>>> new file mode 100644
>>>>> index 000000000000..4aad4974b208
>>>>> --- /dev/null
>>>>> +++ b/Documentation/edac/edac-scrub.rst
>>>>> @@ -0,0 +1,74 @@
>>>>> +.. SPDX-License-Identifier: GPL-2.0
>>>>> +  
>>> [...]
>>>   
>>>>> +static int cxl_mem_ps_get_attrs(struct cxl_memdev_state *mds,
>>>>> +				struct cxl_memdev_ps_params *params) {
>>>>> +	size_t rd_data_size = sizeof(struct cxl_memdev_ps_rd_attrs);
>>>>> +	size_t data_size;
>>>>> +	struct cxl_memdev_ps_rd_attrs *rd_attrs __free(kfree) =
>>>>> +						kmalloc(rd_data_size,  
>>>> GFP_KERNEL);  
>>>>> +	if (!rd_attrs)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	data_size = cxl_get_feature(mds, cxl_patrol_scrub_uuid,
>>>>> +				    CXL_GET_FEAT_SEL_CURRENT_VALUE,
>>>>> +				    rd_attrs, rd_data_size);
>>>>> +	if (!data_size)
>>>>> +		return -EIO;
>>>>> +
>>>>> +	params->scrub_cycle_changeable =  
>>>> FIELD_GET(CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK,  
>>>>> +						   rd_attrs->scrub_cycle_cap);
>>>>> +	params->enable =  
>>>> FIELD_GET(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,  
>>>>> +				   rd_attrs->scrub_flags);
>>>>> +	params->scrub_cycle_hrs =  
>>>> FIELD_GET(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,  
>>>>> +					    rd_attrs->scrub_cycle_hrs);
>>>>> +	params->min_scrub_cycle_hrs =  
>>>> FIELD_GET(CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK,  
>>>>> +						rd_attrs->scrub_cycle_hrs);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int cxl_ps_get_attrs(struct device *dev, void *drv_data,  
>>>>
>>>> Would a union be better than a void *drv_data for all the places this is used as a
>>>> parameter? How many variations of this are there?
>>>>
>>>> DJ  
>>> Hi Dave,
>>>
>>> Can you give more info on this given this is a generic callback for the scrub control and each
>>> implementation will have its own context struct (for eg. struct cxl_patrol_scrub_context here
>>> for CXL scrub control), which in turn will be passed in and out as opaque data.  
>>
>> Mainly I'm just seeing a lot of calls with (void *). Just asking if we want to make it a union that contains 'struct cxl_patrol_scrub_context' and etc.
> 
> You could but then every new driver would need to include
> changes in the edac core to add it's own entry to that union.
> 
> Not sure that's a good way to go for opaque driver specific context.
> 
> This particular function though can use
> a struct cxl_patrol_scrub_context * anyway as it's not part of the
> core interface, but rather one called only indirectly
> by functions that are passed a void * but know it is a
> struct clx_patrol_scrub_context *.

Thanks Jonathan. That's basically what I wanted to know. 

> 
> Jonathan
> 
> 
>>
>>>
>>> Thanks,
>>> Shiju  
>>>>  
>>>>> +			    struct cxl_memdev_ps_params *params) {
>>>>> +	struct cxl_patrol_scrub_context *cxl_ps_ctx = drv_data;
>>>>> +	struct cxl_memdev *cxlmd;
>>>>> +	struct cxl_dev_state *cxlds;
>>>>> +	struct cxl_memdev_state *mds;
>>>>> +	u16 min_scrub_cycle = 0;
>>>>> +	int i, ret;
>>>>> +
>>>>> +	if (cxl_ps_ctx->cxlr) {
>>>>> +		struct cxl_region *cxlr = cxl_ps_ctx->cxlr;
>>>>> +		struct cxl_region_params *p = &cxlr->params;
>>>>> +
>>>>> +		for (i = p->interleave_ways - 1; i >= 0; i--) {
>>>>> +			struct cxl_endpoint_decoder *cxled = p->targets[i];
>>>>> +
>>>>> +			cxlmd = cxled_to_memdev(cxled);
>>>>> +			cxlds = cxlmd->cxlds;
>>>>> +			mds = to_cxl_memdev_state(cxlds);
>>>>> +			ret = cxl_mem_ps_get_attrs(mds, params);
>>>>> +			if (ret)
>>>>> +				return ret;
>>>>> +
>>>>> +			if (params->min_scrub_cycle_hrs > min_scrub_cycle)
>>>>> +				min_scrub_cycle = params-
>>>>> min_scrub_cycle_hrs;
>>>>> +		}
>>>>> +		params->min_scrub_cycle_hrs = min_scrub_cycle;
>>>>> +		return 0;
>>>>> +	}
>>>>> +	cxlmd = cxl_ps_ctx->cxlmd;
>>>>> +	cxlds = cxlmd->cxlds;
>>>>> +	mds = to_cxl_memdev_state(cxlds);
>>>>> +
>>>>> +	return cxl_mem_ps_get_attrs(mds, params); }
>>>>> +  
>>> [...]  
>>>>  
>>>   
>>
>>
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ