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

On 28.03.23 12:21:04, Bjorn Helgaas wrote:
> [+cc linux-pci, more error handling folks; beginning of thread at
> https://lore.kernel.org/all/20230323213808.398039-1-terry.bowman@amd.com/]
> 
> On Mon, Mar 27, 2023 at 11:51:39PM +0200, Robert Richter wrote:
> > On 24.03.23 17:36:56, Bjorn Helgaas wrote:
> 
> > > > The CXL device driver is then responsible to
> > > > enable error reporting in the RCEC's AER cap
> > > 
> > > I don't know exactly what you mean by "error reporting in the RCEC's
> > > AER cap", but IIUC, for non-Root Port devices, generation of ERR_COR/
> > > ERR_NONFATAL/ERR_FATAL messages is controlled by the Device Control
> > > register and should already be enabled by pci_aer_init().
> > > 
> > > Maybe you mean setting AER mask/severity specifically for Internal
> > > Errors?  I'm hoping to get as much of AER management as we can in the
> > 
> > Richt, this is implemented in patch #5 in function
> > rcec_enable_aer_ints().
> 
> I think we should add a PCI core interface for this so we can enforce
> the AER ownership question (all the crud like pcie_aer_is_native()) in
> one place.

Do you mean, code around functions rcec_enable_aer_ints() should be
moved to aer.c and the cxl handler then just assumes it is enabled
already? That looks feasible.

> 
> > > PCI core and out of drivers, so maybe we need a new PCI interface to
> > > do that.
> > > 
> > > In any event, I assume this sort of configuration would be an
> > > enumeration-time thing, while *this* patch is a run-time thing, so
> > > maybe this information belongs with a different patch?
> > 
> > Do you mean once a Restricted CXL host (RCH) is detected, the internal
> > errors should be enabled in the device mask, all this done during
> > device enumeration? But wouldn't interrupts being enabled then before
> > the CXL device is ready?
> 
> I'm not sure what you mean by "before the CXL device is ready."  What
> makes a CXL device ready, and how do we know when it is ready?

The cxl_pci driver must be bound to a device which then further
creates a CXL mem dev. With that binding we can determine the
connected CXL dports from the cxl endpoints (which are seen as PCIe
endpoints) to inspect the CXL RAS caps (in the CXL component reg
space) and the PCIe AER caps (in the RCRB of the dport).

> 
> pci_aer_init() turns on PCI_EXP_DEVCTL_CERE, PCI_EXP_DEVCTL_FERE, etc
> as soon as we enumerate the device, before any driver claims the
> device.  I'm wondering whether we can do this PCI_ERR_COR_INTERNAL and
> PCI_ERR_UNC_INTN fiddling around the same time?

Yes, if the CXL device is not yet bound, there is no handler attached
and AER errors are only handled on a PCI level. Though, we need to
make sure the status is cleared.

> 
> > > I haven't worked all the way through this, but I thought Sean Kelley's
> > > and Qiuxu Zhuo's work was along the same line and might cover this,
> > > e.g.,
> > > 
> > >   a175102b0a82 ("PCI/ERR: Recover from RCEC AER errors")
> > >   579086225502 ("PCI/ERR: Recover from RCiEP AER errors")
> > >   af113553d961 ("PCI/AER: Add pcie_walk_rcec() to RCEC AER handling")
> > > 
> > > But I guess maybe it's not quite the same case?
> > 
> > Actually, we use this code to handle errors that are reported to the
> > RCEC and only implement here the CXL specifics. That is, checking if
> > the RCEC receives something from a CXL downstream port and forwarding
> > that to a CXL handler (this patch). The handler then checks the AER
> > err cap in the RCRB of all CXL downstream ports associated to the RCEC
> > (not visible in the PCI hierarchy), but discovered through the :00.0
> > RCiEP (patch #5).
> 
> There are two calls to pcie_walk_rcec():
> 
>   1) The existing one in find_source_device()
>   2) The one you add in handle_cxl_error()
> 
> Does the call in handle_cxl_error() look at devices that the existing
> call in find_source_device() does not?  I'm trying to understand why
> we need both calls.

In case of a dport error, e_info will only contain the RCEC's id after
running find_source_device(). Thus, only the RCEC's handler would be
called. The portdrv is already bound to the device and currently
doesn't have a handler attached.

As described, due to cross dependencies between cxl and the portdrv,
instead of implementing a handler in the portdrv, we decided to
forward errors to the CXL endpoint driver and handle it there. So now,
in handle_cxl_error(), we check if the error source is an RCEC
attached to a CXL bus and we forward everything directly to the CXL
endpoint handler. pcie_walk_rcec() is used for that.

> 
> > > > +static bool is_internal_error(struct aer_err_info *info)
> > > > +{
> > > > +	if (info->severity == AER_CORRECTABLE)
> > > > +		return info->status & PCI_ERR_COR_INTERNAL;
> > > > +
> > > > +	return info->status & PCI_ERR_UNC_INTN;
> > > > +}
> > > > +
> > > > +static void handle_cxl_error(struct pci_dev *dev, struct aer_err_info *info)
> > > > +{
> > > > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC &&
> > > > +	    is_internal_error(info))
> > > 
> > > What's unique about Internal Errors?  I'm trying to figure out why you
> > > wouldn't do this for *all* CXL errors.
> > 
> > Per CXL specification downstream port errors are signaled using
> > internal errors. 
> 
> Maybe a spec reference here to explain is_internal_error()?  Is the
> point of the check to *exclude* non-internal errors?  Or is basically
> documentation that there shouldn't ever *be* any non-internal errors?
> I guess the latter wouldn't make sense because at this point we don't
> know whether this is a CXL hierarchy.

It is described in CXL 3.0 spec, 12.2.1.1 RCH Downstream Port-detected
Errors.

We do not handle errors other than internal ones, this is what the
check is for. In theory, an RCEC could also throw other kind of
errors. But, as per spec, once internal error are received from the
RCEC, the CXL dports need to be inspected.

> 
> > All other errors would be device specific, we cannot
> > handle that in a generic CXL driver.
> 
> I'm missing the point here.  We don't have any device-specific error
> handling in aer.c; it only connects the generic *reporting* mechanism
> (AER log registers and Root Port interrupts) to the drivers that do
> the device-specific things via err_handler hooks.  I assume we want a
> similar model for CXL.

With device specific I mean implementation defined and not described
in a specification. The CXL handler is sort of generic as it is
(solely) implementing the CXL spec. Hope that makes sense.

Thanks,

-Robert

Powered by blists - more mailing lists