linux-kernel - Re: [PATCH v4 13/28] cxl/mem: Expose DCD partition capabilities in sysfs

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <6710316875819_2cee294ec@iweiny-mobl.notmuch>
Date: Wed, 16 Oct 2024 16:34:32 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, <ira.weiny@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Navneet
 Singh" <navneet.singh@...el.com>, Jonathan Corbet <corbet@....net>, "Andrew
 Morton" <akpm@...ux-foundation.org>, Dan Williams <dan.j.williams@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>,
	<linux-btrfs@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 13/28] cxl/mem: Expose DCD partition capabilities in
 sysfs

Jonathan Cameron wrote:
> On Mon, 07 Oct 2024 18:16:19 -0500
> ira.weiny@...el.com wrote:
> 
> > From: Navneet Singh <navneet.singh@...el.com>
> > 
> > To properly configure CXL regions on Dynamic Capacity Devices (DCD),
> > user space will need to know the details of the DC partitions available.
> > 
> > Expose dynamic capacity capabilities through sysfs.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@...el.com>
> > Co-developed-by: Ira Weiny <ira.weiny@...el.com>
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> Some trivial stuff inline that I'm not that bothered about either way.
> 
> Subject to answering Fan's query
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 

[snip]

> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 3f5627a1210a..b865eefdb74c 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -54,6 +54,51 @@ Description:
> >  		identically named field in the Identify Memory Device Output
> >  		Payload in the CXL-2.0 specification.
> >  
> > +What:		/sys/bus/cxl/devices/memX/dcY/size
> > +Date:		December, 2024
> > +KernelVersion:	v6.13
> > +Contact:	linux-cxl@...r.kernel.org
> > +Description:
> > +		(RO) Dynamic Capacity (DC) region information.  Devices only
> > +		export dcY if DCD partition Y is supported.
> > +		dcY/size is the size of each of those partitions.
> > +
> > +What:		/sys/bus/cxl/devices/memX/dcY/read_only
> > +Date:		December, 2024
> > +KernelVersion:	v6.13
> > +Contact:	linux-cxl@...r.kernel.org
> > +Description:
> > +		(RO) Dynamic Capacity (DC) region information.  Devices only
> > +		export dcY if DCD partition Y is supported.
> > +		dcY/read_only indicates true if the region is exported
> > +		read_only from the device.
> > +
> > +What:		/sys/bus/cxl/devices/memX/dcY/shareable
> > +Date:		December, 2024
> > +KernelVersion:	v6.13
> > +Contact:	linux-cxl@...r.kernel.org
> > +Description:
> > +		(RO) Dynamic Capacity (DC) region information.  Devices only
> > +		export dcY if DCD partition Y is supported.
> > +		dcY/shareable indicates true if the region is exported
> > +		shareable from the device.
> > +
> > +What:		/sys/bus/cxl/devices/memX/dcY/qos_class
> > +Date:		December, 2024
> > +KernelVersion:	v6.13
> > +Contact:	linux-cxl@...r.kernel.org
> > +Description:
> > +		(RO) Dynamic Capacity (DC) region information.  Devices only
> > +		export dcY if DCD partition Y is supported.  
> 
> You can document sysfs directories I think, e.g.
> https://elixir.bootlin.com/linux/v6.12-rc2/source/Documentation/ABI/stable/sysfs-devices-node#L32
> so maybe
> 
> What:			/sys/bus/cxl/device/memX/dcY
> Date:		December, 2024
> KernelVersion:	v6.13
> Contact:	linux-cxl@...r.kernel.org
> Description: 
> 		Directory containing Dynamic Capacity (DC) region information.
>                 Devices only export dcY if DCD partition Y is supported.
> 
> What:		/sys/bus/cxl/devices/memX/dcY/qos_class
> Date:		December, 2024
> KernelVersion:	v6.13
> Contact:	linux-cxl@...r.kernel.org
> Description:
> 		For CXL host...
> 
> To avoid the repetition of first bit of docs?

The other docs don't do this.  For example:

	/sys/bus/cxl/devices/memX
	/sys/bus/cxl/devices/memX/ram
	/sys/bus/cxl/devices/memX/pmem

Are not documented like that.  I'm inclined to leave it.

> 
> > +		platforms that support "QoS Telemmetry" this attribute conveys
> > +		a comma delimited list of platform specific cookies that
> > +		identifies a QoS performance class for the persistent partition
> > +		of the CXL mem device. These class-ids can be compared against
> > +		a similar "qos_class" published for a root decoder. While it is
> > +		not required that the endpoints map their local memory-class to
> > +		a matching platform class, mismatches are not recommended and
> > +		there are platform specific performance related side-effects
> > +		that may result. First class-id is displayed.
> >  
> >  What:		/sys/bus/cxl/devices/memX/pmem/qos_class
> >  Date:		May, 2023
> 
> 
> > +static ssize_t show_shareable_dcN(struct cxl_memdev *cxlmd, char *buf, int pos)
> > +{
> > +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> > +
> > +	return sysfs_emit(buf, "%s\n",
> > +			  str_false_true(mds->dc_region[pos].shareable));
> 
> Fan has already raised that these seem backwards.

Yep fixed.


[snip]

> > +static struct attribute *cxl_memdev_dc##n##_attributes[] = {			\
> > +	&dc##n##_size.attr,							\
> > +	&dc##n##_read_only.attr,						\
> > +	&dc##n##_shareable.attr,						\
> > +	&dc##n##_qos_class.attr,						\
> > +	NULL,									\
> 
> No comma needed on terminator.

Fixed.

> 
> > +};										\
> > +static umode_t cxl_memdev_dc##n##_attr_visible(struct kobject *kobj,		\
> > +					       struct attribute *a,		\
> > +					       int pos)				\
> > +{										\
> > +	struct device *dev = kobj_to_dev(kobj);					\
> > +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);				\
> > +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);	\
> > +										\
> > +	/* Not a memory device */						\
> > +	if (!mds)								\
> 	if (!to_cxl_memdev_state(cxlmd->cxlds))
> 		return 0;
> 
> I dislike long macros so if we can shave them down that is always good!

Agreed but this was the most straight forward way to deal with this.  I
could perhaps break it up by having a 'master macro' which is made of
smaller macros...  But this works.

> 
> We do have precedence in hdm.c

Not in hdm.c directly but all the 'to_XXX()' calls have a type check.  So
it is modeled that way and is called from other places.

>
> for just checking the type directly so maybe
> 	if (cxlmd->cxlds->type != CXL_DEVTYPE_CLASSMEM)
> 
> but the above is also fine as compiler should be able to figure out it
> doesn't need to do the second half of the inline.

I'm going to leave it.

> 
> 
> > +		return 0;							\
> > +	return a->mode;								\
> > +}										\
> > +static umode_t cxl_memdev_dc##n##_group_visible(struct kobject *kobj)		\
> > +{										\
> > +	struct device *dev = kobj_to_dev(kobj);					\
> > +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);				\
> > +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);	\
> > +										\
> > +	/* Not a memory device or partition not supported */			\
> > +	if (!mds || n >= mds->nr_dc_region)					\
> > +		return false;							\
> > +	return true;								\
> 
> 	/* Memory device and partition is supported */
> 	return mds && n < mds->nr_dc_region;

Done.
Ira