Re: [PATCH v3 3/6] cxl/memdev: Add trigger_poison_list sysfs attribute

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Nov 16, 2022 at 12:48:55PM +0000, Jonathan Cameron wrote:
> On Thu, 10 Nov 2022 19:12:41 -0800
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > When a boolean 'true' is written to this attribute the memdev driver
> > retrieves the poison list from the device. The list includes addresses
> > that are poisoned, or would result in poison if accessed, and the
> > source of the poison. This attribute is only visible for devices
> > supporting the capability. The retrieved errors are logged as kernel
> > trace events with the label 'cxl_poison'.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Hi Alison,
> 
> A few comments inline.
> 
> Jonathan
> 
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl | 14 +++++++++
> >  drivers/cxl/core/memdev.c               | 41 +++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 8494ef27e8d2..1c5f4a853ba2 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -388,3 +388,17 @@ Description:
> >  		1), and checks that the hardware accepts the commit request.
> >  		Reading this value indicates whether the region is committed or
> >  		not.
> > +
> > +
> > +What:		/sys/bus/cxl/devices/memX/trigger_poison_list
> > +Date:		November, 2022
> > +KernelVersion:	v6.2
> > +Contact:	linux-cxl@vger.kernel.org
> > +Description:
> > +		(WO) When a boolean 'true' is written to this attribute the
> > +		memdev driver retrieves the poison list from the device. The
> > +		list includes addresses that are poisoned or would result in
> > +		poison if accessed, and the source of the poison. This
> 
> Trivial, but 'includes' kind of implies it might have other things as well.
> 
> The list consists of addresses that are... perhaps?
> 

Changed that to consists. Now, I will be on the lookout for other
usages of includes with unintended implications ;)


> > +		attribute is only visible for devices supporting the
> > +		capability. The retrieved errors are logged as kernel
> > +		trace events with the label 'cxl_poison'.
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index 20ce488a7754..06d265db5127 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -106,12 +106,45 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(numa_node);
> >  
> > +static ssize_t trigger_poison_list_store(struct device *dev,
> > +					 struct device_attribute *attr,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +	u64 offset, length;
> > +	bool tmp;
> > +	int rc;
> > +
> > +	if (kstrtobool(buf, &tmp))
> > +		return -EINVAL;
> > +
> > +	/* Per CXL Spec, separate the pmem and ram poison list reads */

/* Per CXL Spec 8.2.9.8.4.1 separate the pmem and ram requests */

Added this ^

> 
> Reference?  I assume this is the bit about what happens if the
> device doesn't support poison list retrieval for volatile memory?
> I can't find anything more specific but maybe I'm looking in wrong place.
> 
Section 8.2.9.8.4.1
Says this: 
The device may reject this command if the requested range spans the
device’s volatile and persistent partitions.

And this -
If the device does not support poison list for volatile ranges and
any location in the requested list maps to volatile, the device
shall return Invalid Physical Address.

And, the device only reports that it supports the Get Poison 
command, in general, not specifically for pmem or volatile.

So, we split the requests along ram and pmem boundaries.

> > +	if (resource_size(&cxlds->pmem_res)) {
> > +		offset = cxlds->pmem_res.start;
> > +		length = resource_size(&cxlds->pmem_res);
> > +		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +	if (resource_size(&cxlds->ram_res)) {
> > +		offset = cxlds->ram_res.start;
> > +		length = resource_size(&cxlds->ram_res);
> > +		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> > +		if (rc)
> > +			return rc;
> 
> Hmm.  So should we handle the return of Invalid Physical Address as an
> error or not?  I think not given it's optional to support the poison
> list for volatile memory.
>
Double hmmm. It wasn't obvious what to do here. Users can look for the
events to see if anything was traced, regardless of the rc. But, if an
'rc' stopped a user from looking for results, that would be bad.

I'll go with no error return on ram failure.

> > +	}
> > +	return len;
> > +}
> > +static DEVICE_ATTR_WO(trigger_poison_list);
> > +
> >  static struct attribute *cxl_memdev_attributes[] = {
> >  	&dev_attr_serial.attr,
> >  	&dev_attr_firmware_version.attr,
> >  	&dev_attr_payload_max.attr,
> >  	&dev_attr_label_storage_size.attr,
> >  	&dev_attr_numa_node.attr,
> > +	&dev_attr_trigger_poison_list.attr,
> >  	NULL,
> >  };
> >  
> > @@ -130,6 +163,14 @@ static umode_t cxl_memdev_visible(struct kobject *kobj, struct attribute *a,
> >  {
> >  	if (!IS_ENABLED(CONFIG_NUMA) && a == &dev_attr_numa_node.attr)
> >  		return 0;
> > +
> > +	if (a == &dev_attr_trigger_poison_list.attr) {
> > +		struct device *dev = kobj_to_dev(kobj);
> > +
> > +		if (!test_bit(CXL_MEM_COMMAND_ID_GET_POISON,
> > +			      to_cxl_memdev(dev)->cxlds->enabled_cmds))
> > +			return 0;
> > +	}
> >  	return a->mode;
> >  }
> >  
>