lkml.org 
[lkml]   [2022]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/3] cxl/mbox: Add GET_POISON_LIST mailbox command support
    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:

    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;
    >

    \
     
     \ /
      Last update: 2022-06-17 18:51    [W:3.323 / U:23.588 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site