Re: [PATCH 2/3] cxl/mbox: Add GET_POISON_LIST mailbox command support

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

On Thu, Jun 16, 2022 at 12:43:34PM -0700, Davidlohr Bueso wrote:
> On Tue, 14 Jun 2022, alison.schofield@intel.com wrote:
> 
> >From: Alison Schofield <alison.schofield@intel.com>
> >
> >CXL devices that support persistent memory maintain a list of locations
> >that are poisoned or result in poison if the addresses are accessed by
> >the host.
> >
> >Per the spec (CXL 2.0 8.2.8.5.4.1), the device returns this Poison
> >list as a set of  Media Error Records that include the source of the
> >error, the starting device physical address and length. The length is
> >the number of adjacent DPAs in the record and is in units of 64 bytes.
> >
> >Retrieve the list and log each Media Error Record as a trace event of
> >type cxl_poison_list.
> >
> >Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> >---
> > drivers/cxl/cxlmem.h    | 43 +++++++++++++++++++++++
> > drivers/cxl/core/mbox.c | 75 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 118 insertions(+)
> >
snip

> >+int cxl_mem_get_poison_list(struct device *dev)
> >+{
> >+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> >+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >+	struct cxl_mbox_poison_payload_out *po;
> >+	struct cxl_mbox_poison_payload_in pi;
> >+	int nr_records = 0;
> >+	int rc, i;
> >+
> >+	if (range_len(&cxlds->pmem_range)) {
> >+		pi.offset = cpu_to_le64(cxlds->pmem_range.start);
> >+		pi.length = cpu_to_le64(range_len(&cxlds->pmem_range));

First off - you stopped at a bug here - that pi.length needs to be
in units of 64 bytes.
> 
> Do you ever see this changing to not always use the full pmem DPA range
> but allow arbitrary ones? I also assume this is the reason why you don't
> check the range vs cxlds->ram_range to prevent any overlaps, no?
> 
> Thanks,
> Davidlohr

David - Great question!

I'm headed in this direction -

cxl list --media-errors -m mem1
	lists media errors for requested memdev

cxl list --media-errors -r region#
	lists region errors with HPA addresses
	(So here cxl tool will collect the poison for all the regions
	 memdevs and do the DPA to HPA translation)

To answer your question, I wasn't thinking of limiting
the range within the memdev, but certainly could. And if we were
taking in ranges, those ranges would need to be checked.

$cxl list --media-errors -m mem1 --range-start=  --range-end|len=

Now, if I left the sysfs inteface as is, the driver will read the 
entire poison list for the memdev and then cxl tool will filter it
for the range requested. 

Or, maybe we should implement in libcxl (not sysfs), with memdev and
range options and only collect from the device the range requested.

Either one looks the same to the cxl tool user, but limiting the
range we send to the device would certainly cut down on unwanted
records being logged, retrieved, and examined.

I'd like to hear more from you and other community members.

Alison

> > snip