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@xxxxxxx>
> ...
> 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()
- 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.
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?
> > 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.
Bjorn
