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

From: Dan Williams
Date: Fri Jun 17 2022 - 15:32:41 EST


Alison Schofield wrote:
> On Fri, Jun 17, 2022 at 07:05:08AM -0700, Jonathan Cameron wrote:
> > On Tue, 14 Jun 2022 17:10:27 -0700
> > alison.schofield@xxxxxxxxx wrote:
> >
> > > From: Alison Schofield <alison.schofield@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> >
> > A few more things inline.
> >
> > Otherwise, can confirm it works with some hack QEMU code.
> > I'll tidy that up and post soon.
> >
> > > +int cxl_mem_get_poison_list(struct device *dev)
> > > +{
> snip
> > > +
> > > + trace_cxl_poison_list(dev, source, addr, len);
> >
> > Need to mask off the lower 6 bits of addr as they contain the source
> > + a few reserved bits.
> >
> > I was confused how you were geting better than 64 byte precision in your
> > example.
> >
> Ah...got it. Thanks!
>
> > > + }
> > > +
> > > + /* Protect against an uncleared _FLAG_MORE */
> > > + nr_records = nr_records + le16_to_cpu(po->count);
> > > + if (nr_records >= cxlds->poison_max)
> > > + goto out;
> > > +
> > > + } while (po->flags & CXL_POISON_FLAG_MORE);
> > So.. A conundrum here. What happens if:
> >
> > 1. We get an error mid way through a set of multiple reads
> > (something intermittent - maybe a software issue)
> > 2. We will drop out of here fine and report the error.
> > 3. We run this function again.
> >
> > It will (I think) currently pick up where we left off, but we have
> > no way of knowing that as there isn't a 'total records' count or
> > any other form of index in the output payload.
>
> Yes. That is sad. I'm assume it's by design and CXL devices never
> intended to keep any totals.
>
> >
> > So, software solutions I think should work (though may warrant a note
> > to be added to the spec).
> >
> > 1. Read whole thing twice. First time is just to ensure we get
> > to the end and flush out any prior half done reads.
> > 2. Issue a read for a different region (perhaps length 0) first
> > and assume everything starts from scratch when we go back to
> > this region.
>
> Can you tell me more about 2 ?
>
> Also, Since posting this I have added protection to this path to ensure
> only one reader of the poison list for this device. Like this:
>
> if (!completion_done(&cxlds->read_poison_complete);
> return -EBUSY;
> wait_for_completion_interruptible(&cxlds->read_poison_complete);
> ...GET ALL THE POISON...
> complete(&cxlds->read_poison_complete);

Since this runs in the context of the requester a completion feels out
of place. What this probably wants is a mutex() protecting the state
machine of the Media Error Record retrieval and the "more" flag.

>
> And will add the error message on that unexpected _FLAG_MORE too.
>
> Alison
> >
> > Jonathan
> >
>
>
>
> > > +
> > > +out:
> > > + kvfree(po);
> > > + return rc;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison_list, CXL);
> > > +
> > > struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
> > > {
> > > struct cxl_dev_state *cxlds;
> >