[v5,3/5] cxl/memdev: Add trigger_poison_list sysfs attribute

Commit Message

Alison Schofield Jan. 18, 2023, 8:02 p.m. UTC

From: Alison Schofield <[e-mail]>

When a boolean 'true' is written to this attribute the memdev driver
retrieves the poison list from the device. The list consists of
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'.

Reviewed-by: Jonathan Cameron <[e-mail]>
Signed-off-by: Alison Schofield <[e-mail]>
---
 Documentation/ABI/testing/sysfs-bus-cxl | 14 ++++++++
 drivers/cxl/core/memdev.c               | 45 +++++++++++++++++++++++++
 drivers/cxl/cxlmem.h                    |  2 +-
 3 files changed, 60 insertions(+), 1 deletion(-)

Comments

Dan Williams Jan. 27, 2023, 7:42 p.m. UTC | #1

alison.schofield@ wrote:
> From: Alison Schofield <[e-mail]>
> 
> When a boolean 'true' is written to this attribute the memdev driver
> retrieves the poison list from the device. The list consists of
> 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'.
> 
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Alison Schofield <[e-mail]>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 14 ++++++++
>  drivers/cxl/core/memdev.c               | 45 +++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h                    |  2 +-
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 8494ef27e8d2..df40ed09ea67 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:	[e-mail]
> +Description:
> +		(WO) When a boolean 'true' is written to this attribute the
> +		memdev driver retrieves the poison list from the device. The
> +		list consists of 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'.
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index a74a93310d26..e0af7e9c9989 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -106,12 +106,49 @@ 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;

Wait, where's the check for "if (tmp)"? Otherwise I can do "echo N >
trigger_poison_list" and it will still run. Should probably
s/tmp/trigger/.

> +
> +	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
> +	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);

Ah now I see why the region information is not provided in the memdev
triggers. I think while the scan needs to be done on partition boundary
basis, like you have here, I think the region lookup needs to happen on
a per-record basis.

However, that's a significant architectural change, so I am going to
think out loud about the implications.

A site operator wants to know "is FRU (field-replaceable-unit) X bad,
and if so what's the damage?". In that report I expect they want to know
if poison impacts media that is allocated to a region (an imminent
danger) or is in free space (a problem for a later date). If the memdev
trigger does all the region lookup per record it provides that
information. Further if the memdev trigger reliably provides region
information it allows the region trigger ABI to be deleted. The region
trigger can be replaced in userspace with logic that does:

   set tracepoint filter to look for region_id == $region
   for endpoint decoder in $region
       trigger associated memdev

...so I do think the arch change is worth it, but I am willing to hear
counter arguments.

Alison Schofield Jan. 27, 2023, 9:52 p.m. UTC | #2

On Fri, Jan 27, 2023 at 11:42:59AM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <[e-mail]>
> > 
> > When a boolean 'true' is written to this attribute the memdev driver
> > retrieves the poison list from the device. The list consists of
> > 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'.
> > 
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Signed-off-by: Alison Schofield <[e-mail]>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl | 14 ++++++++
> >  drivers/cxl/core/memdev.c               | 45 +++++++++++++++++++++++++
> >  drivers/cxl/cxlmem.h                    |  2 +-
> >  3 files changed, 60 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 8494ef27e8d2..df40ed09ea67 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:	[e-mail]
> > +Description:
> > +		(WO) When a boolean 'true' is written to this attribute the
> > +		memdev driver retrieves the poison list from the device. The
> > +		list consists of 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'.
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index a74a93310d26..e0af7e9c9989 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -106,12 +106,49 @@ 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;
> 
> Wait, where's the check for "if (tmp)"? Otherwise I can do "echo N >
> trigger_poison_list" and it will still run. Should probably
> s/tmp/trigger/.

Got it.
(I guess I was too loose w the trigger, thinking you write anything to
it, I'm triggering.)
> 
> > +
> > +	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
> > +	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);
> 
> Ah now I see why the region information is not provided in the memdev
> triggers. I think while the scan needs to be done on partition boundary
> basis, like you have here, I think the region lookup needs to happen on
> a per-record basis.
> 
> However, that's a significant architectural change, so I am going to
> think out loud about the implications.

I'm thinking that adding region info to the trace event isn't starting
here. That will be looked up when we log the event. Basically assuming
assuming find regions memdev participates in, and look for which one
maps this DPA.

> 
> A site operator wants to know "is FRU (field-replaceable-unit) X bad,
> and if so what's the damage?". In that report I expect they want to know
> if poison impacts media that is allocated to a region (an imminent
> danger) or is in free space (a problem for a later date). If the memdev
> trigger does all the region lookup per record it provides that
> information. Further if the memdev trigger reliably provides region
> information it allows the region trigger ABI to be deleted. The region
> trigger can be replaced in userspace with logic that does:
> 
>    set tracepoint filter to look for region_id == $region
>    for endpoint decoder in $region
>        trigger associated memdev
> 
> ...so I do think the arch change is worth it, but I am willing to hear
> counter arguments.

My only counter argument was that we could let them have it both ways,
but, simplifying and reducing code in kernel is good!

Dan Williams Jan. 27, 2023, 10:46 p.m. UTC | #3

Alison Schofield wrote:
> On Fri, Jan 27, 2023 at 11:42:59AM -0800, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <[e-mail]>
> > > 
> > > When a boolean 'true' is written to this attribute the memdev driver
> > > retrieves the poison list from the device. The list consists of
> > > 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'.
> > > 
> > > Reviewed-by: Jonathan Cameron <[e-mail]>
> > > Signed-off-by: Alison Schofield <[e-mail]>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-cxl | 14 ++++++++
> > >  drivers/cxl/core/memdev.c               | 45 +++++++++++++++++++++++++
> > >  drivers/cxl/cxlmem.h                    |  2 +-
> > >  3 files changed, 60 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > > index 8494ef27e8d2..df40ed09ea67 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:	[e-mail]
> > > +Description:
> > > +		(WO) When a boolean 'true' is written to this attribute the
> > > +		memdev driver retrieves the poison list from the device. The
> > > +		list consists of 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'.
> > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > > index a74a93310d26..e0af7e9c9989 100644
> > > --- a/drivers/cxl/core/memdev.c
> > > +++ b/drivers/cxl/core/memdev.c
> > > @@ -106,12 +106,49 @@ 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;
> > 
> > Wait, where's the check for "if (tmp)"? Otherwise I can do "echo N >
> > trigger_poison_list" and it will still run. Should probably
> > s/tmp/trigger/.
> 
> Got it.
> (I guess I was too loose w the trigger, thinking you write anything to
> it, I'm triggering.)
> > 
> > > +
> > > +	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
> > > +	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);
> > 
> > Ah now I see why the region information is not provided in the memdev
> > triggers. I think while the scan needs to be done on partition boundary
> > basis, like you have here, I think the region lookup needs to happen on
> > a per-record basis.
> > 
> > However, that's a significant architectural change, so I am going to
> > think out loud about the implications.
> 
> I'm thinking that adding region info to the trace event isn't starting
> here. That will be looked up when we log the event. Basically assuming
> assuming find regions memdev participates in, and look for which one
> maps this DPA.

Yup, we're on the same page. Just compare the dpa of the error record to
the dpa ranges of the enabled decoders on the memdev then it is a
straightforward cxld->region de-reference from there.

> 
> > 
> > A site operator wants to know "is FRU (field-replaceable-unit) X bad,
> > and if so what's the damage?". In that report I expect they want to know
> > if poison impacts media that is allocated to a region (an imminent
> > danger) or is in free space (a problem for a later date). If the memdev
> > trigger does all the region lookup per record it provides that
> > information. Further if the memdev trigger reliably provides region
> > information it allows the region trigger ABI to be deleted. The region
> > trigger can be replaced in userspace with logic that does:
> > 
> >    set tracepoint filter to look for region_id == $region
> >    for endpoint decoder in $region
> >        trigger associated memdev
> > 
> > ...so I do think the arch change is worth it, but I am willing to hear
> > counter arguments.
> 
> My only counter argument was that we could let them have it both ways,
> but, simplifying and reducing code in kernel is good!

Yeah, in this case I think the memdev interface is -><- that close to
being a general purpose poison lister with all the relevant info in the
record. The only thing missing after the info is populated is the filter
logic, but event filtering is a first class feature of the trace events
subsystem.

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 8494ef27e8d2..df40ed09ea67 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:	[e-mail]
+Description:
+		(WO) When a boolean 'true' is written to this attribute the
+		memdev driver retrieves the poison list from the device. The
+		list consists of 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'.
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index a74a93310d26..e0af7e9c9989 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -106,12 +106,49 @@  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;
+
+	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
+	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);
+		/*
+		 * Invalid Physical Address is not an error for
+		 * volatile addresses. Device support is optional.
+		 */
+		if (rc && rc != -EFAULT)
+			return rc;
+	}
+	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 +167,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;
 }
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index f53fae20f502..28ba0cd8f2d3 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -139,7 +139,7 @@  struct cxl_mbox_cmd {
 	C(FWROLLBACK, -ENXIO, "rolled back to the previous active FW"),         \
 	C(FWRESET, -ENXIO, "FW failed to activate, needs cold reset"),		\
 	C(HANDLE, -ENXIO, "one or more Event Record Handles were invalid"),     \
-	C(PADDR, -ENXIO, "physical address specified is invalid"),		\
+	C(PADDR, -EFAULT, "physical address specified is invalid"),		\
 	C(POISONLMT, -ENXIO, "poison injection limit has been reached"),        \
 	C(MEDIAFAILURE, -ENXIO, "permanent issue with the media"),		\
 	C(ABORT, -ENXIO, "background cmd was aborted by device"),               \