Messages in this thread | | | Date | Fri, 17 Jun 2022 10:29:14 -0700 | From | Davidlohr Bueso <> | Subject | Re: [PATCH 2/3] cxl/mbox: Add GET_POISON_LIST mailbox command support |
| |
On Fri, 17 Jun 2022, 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@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> >> >> 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:
I don't think we should prevent multiple list reads. I would expect the scenario Jonathan describes to be the uncommon case.
Thanks, Davidlohr
> >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); > >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; >>
| |