On 14.04.23 16:32:54, Bjorn Helgaas wrote:

On Thu, Apr 13, 2023 at 01:40:52PM +0200, Robert Richter wrote:

quoted

On 12.04.23 17:02:33, Bjorn Helgaas wrote:

quoted

On Tue, Apr 11, 2023 at 01:03:01PM -0500, Terry Bowman wrote:

quoted

From: Robert Richter <redacted>

quoted

...
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).

quoted

quoted

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.

quoted

...

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

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