Re: [PATCH v6] cxl: add RAS status unmasking for CXL

[Dan Williams <dan.j.williams@intel.com>]

Bjorn Helgaas wrote:
> On Fri, Feb 10, 2023 at 10:04:03AM -0700, Dave Jiang wrote:
> > By default the CXL RAS mask registers bits are defaulted to 1's and
> > suppress all error reporting. If the kernel has negotiated ownership
> > of error handling for CXL then unmask the mask registers by writing 0s.
> > 
> > PCI_EXP_AER_FLAGS moved to linux/pci.h header to expose to driver. It
> > allows exposure of system enabled PCI error flags for the driver to decide
> > which error bits to toggle. Bjorn suggested that the error enabling should
> > be controlled from the system policy rather than a driver level choice[1].
> > 
> > CXL RAS CE and UE masks are checked against PCI_EXP_AER_FLAGS before
> > unmasking.
> > 
> > [1]: https://lore.kernel.org/linux-cxl/20230210122952.00006999@Huawei.com/T/#me8c7f39d43029c64ccff5c950b78a2cee8e885af
> 
> > +static int cxl_pci_ras_unmask(struct pci_dev *pdev)
> > +{
> > +	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
> > +	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> > +	void __iomem *addr;
> > +	u32 orig_val, val, mask;
> > +
> > +	if (!cxlds->regs.ras)
> > +		return -ENODEV;
> > +
> > +	/* BIOS has CXL error control */
> > +	if (!host_bridge->native_cxl_error)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (PCI_EXP_AER_FLAGS & PCI_EXP_DEVCTL_URRE) {
> 
> 1) I don't really want to expose PCI_EXP_AER_FLAGS in linux/pci.h.
> It's basically a convenience part of the AER implementation.
> 
> 2) I think your intent here is to configure the CXL RAS masking based
> on what PCIe error reporting is enabled, but doing it by looking at
> PCI_EXP_AER_FLAGS doesn't seem right.  This expression is a
> compile-time constant that is always true, but we can't rely on
> devices always being configured that way.

Oh, I had asked Dave to do that to try to satisfy your request for a
system wide policy. So that if someone wanted to modify what errors get
unmasked globally just look at that value rather than re-read the
register, but it seems I over-intepreted what you and Jonathan were
talking about here when you mention "system policy":

https://lore.kernel.org/linux-cxl/20221229172731.GA611562@bhelgaas/

> We call pci_aer_init() for every device during enumeration, but we
> only write PCI_EXP_AER_FLAGS if pci_aer_available() and if
> pcie_aer_is_native().  And there are a bunch of drivers that call
> pci_disable_pcie_error_reporting(), which *clears* those flags.  I'm
> not sure those drivers *should* be doing that, but they do today.
> 
> I'm not sure why this needs to be conditional at all, but if it does,
> maybe you want to read PCI_EXP_DEVCTL and base it on that?

Re-reading the register works too.

Some of this may want to move to a more core location once/if CXL devices
beyond the generic Memory Expander Class Code start arriving, but that
can be saved for later.