Message-ID: <ZD3BlNYeiXQR2h6+@rric.localdomain>
Date:   Tue, 18 Apr 2023 00:00:52 +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

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:
> > > > From: Robert Richter <rrichter@....com>
> 
> > ...
> > Let's assume just a simple CXL RCH topology:
> > 
> > PCI hierarchy:
> > 
> >               -----------------
> >               | ACPI0016      |--------------       Host bridge (CXL host)
> >               | - CEDT        |             |
> >    -----------|   - RCRB base |             |
> >    |          -----------------             :
> >    |               |
> >    |               |
> >    |          -------------------     ---------
> >    |          | RCiEP           |.....| RCEC  |     Endpoint (CXL dev)
> >    |  --------| - BDF           |     | - BDF |
> >    |  |       | - PCIe AER      |     ---------
> >    |  |       | - CXL dvsec     |
> >    |  |       |   (v2: reg loc) |
> >    |  |       |   - Comp regs   |
> >    |  |       |     - CXL RAS   |
> >    |  |       -------------------
> >    :  :
> >       
> > CXL hierarchy:
> > 
> >    :                                        :
> >    :          ------------------            |
> >    |          | CXL root port  |<------------
> >    |          |                |        
> >    |--------->| - dport RCRB   |<------------
> >    |          |   - PCIe AER   |            |
> >    |          |   - Comp regs  |            |
> >    |          |     - CXL RAS  |            |
> >    |          ------------------            |
> >    |  :                                     |
> >    |  |       ------------------            |
> >    |  ------->| CXL endpoint   |-------------
> >    |          | (v1: RCRB)     |
> >    ---------->| - uport RCRB   |
> >               |   - Comp regs  |
> >               |     - CXL RAS  |
> >               ------------------
> > 
> > Dport detected errors are reported using PCIe AER and CXL RAS caps in
> > the dports RCRB.
> > 
> > Uport detected errors are reported using RCiEP's PCIe AER cap and
> > either the uport's RCRB RAS cap or the RAS cap of the comp regs
> > located using CXL DVSEC register locator.
> > 
> > In all cases the RCEC is used with either the RCEC (dport errors) or
> > the RCiEP (uport errors) error source id (BDF: bus, dev, func).
> 
> 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.

Thanks,

-Robert

Powered by blists - more mailing lists