Re: [PATCH v3 5/6] PCI/AER: Forward RCH downstream port-detected errors to the CXL.mem dev handler

[Robert Richter]

Hi Jonathan,

On 17.04.23 17:54:31, Jonathan Cameron wrote:
> On Fri, 14 Apr 2023 16:35:05 +0200
> Robert Richter <rrichter@xxxxxxx> wrote:
> 
> > On 14.04.23 13:19:50, Jonathan Cameron wrote:
> > > On Tue, 11 Apr 2023 13:03:01 -0500
> > > Terry Bowman <terry.bowman@xxxxxxx> wrote:
> > >   
> > > > From: Robert Richter <rrichter@xxxxxxx>
> > > > 
> > > > In Restricted CXL Device (RCD) mode a CXL device is exposed as an
> > > > RCiEP, but CXL downstream and upstream ports are not enumerated and
> > > > not visible in the PCIe hierarchy. Protocol and link errors are sent
> > > > to an RCEC.
> > > > 
> > > > Restricted CXL host (RCH) downstream port-detected errors are signaled
> > > > as internal AER errors, either Uncorrectable Internal Error (UIE) or
> > > > Corrected Internal Errors (CIE). The error source is the id of the
> > > > RCEC. A CXL handler must then inspect the error status in various CXL
> > > > registers residing in the dport's component register space (CXL RAS
> > > > cap) or the dport's RCRB (AER ext cap). [1]
> > > > 
> > > > Errors showing up in the RCEC's error handler must be handled and
> > > > connected to the CXL subsystem. Implement this by forwarding the error
> > > > to all CXL devices below the RCEC. Since the entire CXL device is
> > > > controlled only using PCIe Configuration Space of device 0, Function
> > > > 0, only pass it there [2]. These devices have the Memory Device class
> > > > code set (PCI_CLASS_MEMORY_CXL, 502h) and the existing cxl_pci driver
> > > > can implement the handler.  
> > > 
> > > This comment implies only class code compliant drivers.  Sure we don't
> > > have drivers for anything else yet, but we should try to avoid saying
> > > there won't be any (which I think above implies).
> > > 
> > > You have a comment in the code, but maybe relaxing the description above
> > > to "currently support devices have..."  
> > 
> > It is used here to identify CXL memory devices and limit the
> > enablement to those. The spec requires this to be set for CXL mem devs
> > (see cxl 3.0, 8.1.12.2).
> > 
> > There could be other CXL devices (e.g. cache), but other drivers are
> > not yet implemented. That is what I am referring to. The check makes
> > sure there is actually a driver with a handler for it (cxl_pci).
> 
> Understood on intent. My worry is that the above can be read as a
> statement on hardware restrictions, rathe than on what software currently
> implements.  Meh. Minor point so I don't care that much!
> Unlikely anyone will read the patch description after it merges anyway ;)

I have updated the description ...

> > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > > > index 7a25b62d9e01..171a08fd8ebd 100644
> > > > --- a/drivers/pci/pcie/aer.c
> > > > +++ b/drivers/pci/pcie/aer.c
> > > > @@ -946,6 +946,65 @@ static bool find_source_device(struct pci_dev *parent,
> > > >  	return true;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_PCIEAER_CXL
> > > > +
> > > > +static bool is_cxl_mem_dev(struct pci_dev *dev)
> > > > +{
> > > > +	/*
> > > > +	 * A CXL device is controlled only using PCIe Configuration
> > > > +	 * Space of device 0, Function 0.  
> > > 
> > > That's not true in general.   Definitely true that CXL protocol
> > > error reporting is controlled only using this Devfn, but
> > > more generally there could be other stuff in later functions.
> > > So perhaps make the comment more specific.  
> > 
> > I actually mean CXL device in RCD mode here (seen as RCiEP in the PCI
> > hierarchy).
> > 
> > The spec says (cxl 3.0, 8.1.3):
> > 
> > """
> > In either case [(RCD and non-RCD)], the capability, status, and
> > control fields in Device 0, Function 0 DVSEC control the CXL
> > functionality of the entire device.
> 
> > """
> > 
> > So dev 0, func 0 must contain a CXL PCIe DVSEC. Thus it is a CXL
> > device and able to handle CXL AER errors. The limitation to the first
> > device prevents the handler from being run multiple times for the same
> > event.
> 
> Fine with limitation.  Text says "device is controlled only using".
> That is true for what you are controlling here, but other aspects of the
> device are controlled via whatever interface they like.
> 
> Perhaps just quote the specification as you have done in your reply. Then it
> is clear that we mean just these registers.

... and comments.

Thanks,

-Robert