Message-ID: <ZD/4Dimu19lsJLyO@rric.localdomain>
Date:   Wed, 19 Apr 2023 16:17:50 +0200
From:   Robert Richter <rrichter@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     <alison.schofield@...el.com>, <dave.jiang@...el.com>,
        Terry Bowman <terry.bowman@....com>,
        <vishal.l.verma@...el.com>, <linuxppc-dev@...ts.ozlabs.org>,
        <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-cxl@...r.kernel.org>,
        "Mahesh J Salgaonkar" <mahesh@...ux.ibm.com>,
        <bhelgaas@...gle.com>, Oliver O'Halloran <oohall@...il.com>,
        <Jonathan.Cameron@...wei.com>, <bwidawsk@...nel.org>,
        <dan.j.williams@...el.com>, <ira.weiny@...el.com>
Subject: Re: [PATCH v3 5/6] PCI/AER: Forward RCH downstream port-detected
 errors to the CXL.mem dev handler

Bjorn,

On 18.04.23 00:00:58, Robert Richter wrote:
> On 14.04.23 16:32:54, Bjorn Helgaas wrote:
> > On Thu, Apr 13, 2023 at 01:40:52PM +0200, Robert Richter wrote:
> > > On 12.04.23 17:02:33, Bjorn Helgaas wrote:
> > > > On Tue, Apr 11, 2023 at 01:03:01PM -0500, Terry Bowman wrote:

> > I'm mostly interested in the PCI entities involved because that's all
> > aer.c can deal with.  For the above, I think the PCI core only knows
> > about these:
> > 
> >   00:00.0 RCEC  with AER, RCEC EA includes 00:01.0
> >   00:01.0 RCiEP with AER
> > 
> > aer_irq() would handle AER interrupts from 00:00.0.
> > cxl_handle_error() would be called for 00:00.0 and would call
> > handle_error_source() for everything below it (only 00:01.0 here).
> > 
> > > > The current code uses pcie_walk_rcec() in this path, which basically
> > > > searches below a Root Port or RCEC for devices that have an AER error
> > > > status bit set, add them to the e_info[] list, and call
> > > > handle_error_source() for each one:
> > > 
> > > For reference, this series adds support to handle RCH downstream
> > > port-detected errors as described in CXL 3.0, 12.2.1.1.
> > > 
> > > This flow looks correct to me, see comments inline.
> > 
> > We seem to be on the same page here, so I'll trim it out.
> > 
> > > ...
> > > > So we insert cxl_handle_error() in handle_error_source(), where it
> > > > gets called for the RCEC, and then it uses pcie_walk_rcec() again to
> > > > forcibly call handle_error_source() for *every* device "below" the
> > > > RCEC (even though they don't have AER error status bits set).
> > > 
> > > The CXL device contains the links to the dport's caps. Also, there can
> > > be multiple RCs with CXL devs connected to it. So we must search for
> > > all CXL devices now, determine the corresponding dport and inspect
> > > both, PCIe AER and CXL RAS caps.
> > > 
> > > > Then handle_error_source() ultimately calls the CXL driver err_handler
> > > > entry points (.cor_error_detected(), .error_detected(), etc), which
> > > > can look at the CXL-specific error status in the CXL RAS or RCRB or
> > > > whatever.
> > > 
> > > The AER driver (portdrv) does not have the knowledge of CXL internals.
> > > Thus the approach is to pass dport errors to the cxl_mem driver to
> > > handle it there in addition to cxl mem dev errors.
> > > 
> > > > So this basically looks like a workaround for the fact that the AER
> > > > code only calls handle_error_source() when it finds AER error status,
> > > > and CXL doesn't *set* that AER error status.  There's not that much
> > > > code here, but it seems like a quite a bit of complexity in an area
> > > > that is already pretty complicated.
> > 
> > My main point here (correct me if I got this wrong) is that:
> > 
> >   - A RCEC generates an AER interrupt
> > 
> >   - find_source_device() searches all devices below the RCEC and
> >     builds a list everything for which to call handle_error_source()
> 
> find_source_device() does not walk the RCEC if the error source is the
> RCEC itself (note that find_device_iter() is called for the root/rcec
> device first and exits early then).
> 
> > 
> >   - cxl_handle_error() *again* looks at all devices below the same
> >     RCEC and calls handle_error_source() for each one
> > 
> > So the main difference here is that the existing flow only calls
> > handle_error_source() when it finds an error logged in an AER status
> > register, while the new CXL flow calls handle_error_source() for
> > *every* device below the RCEC.
> 
> That is limited as much as possible:
> 
>  * The RCEC walk to handle CXL dport errors is done only in case of
>    internal errors, for an RCEC only (not a port) (check in
>    cxl_handle_error()).
> 
>  * Internal errors are only enabled for RCECs connected to CXL devices
>    (handles_cxl_errors()).
> 
>  * The handler is only called if it is a CXL memory device (class code
>    set and zero devfn) (check in cxl_handle_error_iter()).
> 
> An optimization I see here is to convert some runtime checks to cached
> values determined during device enumeration (CXL device list, RCEC is
> associated with CXL devices). Some sort of RCEC-to-CXL-dev
> association, similar to rcec->rcec_ea.
> 
> > 
> > I think it's OK to do that, but the almost recursive structure and the
> > unusual reference counting make the overall AER flow much harder to
> > understand.
> > 
> > What if we changed is_error_source() to add every CXL.mem device it
> > finds to the e_info[] list, which I think could nicely encapsulate the
> > idea that "CXL devices have error state we don't know how to interpret
> > here"?  Would the existing loop in aer_process_err_devices() then do
> > what you need?
> 
> I did not want to mix this with devices determined by the Error Source
> Identification Register. CXL device may not be the error source of an
> error which may cause some unwanted side-effects. We must also touch
> AER_MAX_MULTI_ERR_DEVICES then and how the dev list is implemented as
> the max number of devices is unclear.
> 
> > 
> > > > Here's another idea: the ACPI GHES code (ghes_handle_aer()) basically
> > > > receives a packet of error status from firmware and queues it for
> > > > recovery via pcie_do_recovery().  What if you had a CXL module that
> > > > knew how to look for the CXL error status, package it up similarly,
> > > > and queue it via aer_recover_queue()?
> > > 
> > > ...
> > > But first, RCEC error notifications (RCEC AER interrupts) must be sent
> > > to the CXL driver to look into the dport's RCRB.
> > 
> > Right.  I think it could be solvable to have aer_irq() call or wake a
> > CXL interface that has been registered.  But maybe changing
> > is_error_source() would be simpler.
> 
> I am going to see if is_error_source() can be used to also find CXL
> devices. But my main concern here is to mix CXL devices with actual
> devices identified by the Error Source ID.

I have looked into reusing is_error_source() and modifying
find_source_device() to also add CXL devices (the RCiEPs) to the dev
list in e_info. The problem I see is that at AER level it is unknown
whether an error happened or not. The downstream port AER capability
also does not reside in a PCI config space header and thus is not
directly bound to a pci_dev. That means the endpoint's AER capability
in pci_dev is not the one we need, instead a CXL aware driver must
lookup the RCRB which contains the AER. Additional, the CXL RAS cap
must be inspected by that driver.

Assuming we add the RCiEP to the dev list the CXL endpoint will be
processed by aer_get_device_error_info(), aer_print_error() and
handle_error_source(). This is done for the endpoint device even if
the source is the dport. Also we need to check the error status of
both caps registers first. This will cause error reports and status
checks of devices not being the error source.

That said, I think the best option is still to delegate the error down
to a CXL handler and do the error status check, reporting and handling
of the CXL specifics there.

I see your point that esp. the pci_dev's refcount handling needs to be
improved. I will address that along with the other review comments in
a next version of this patch series. Let's then revisit this
discussion here?

Thanks,

-Robert

Powered by blists - more mailing lists