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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>,
	linux-cxl@vger.kernel.org, dan.j.williams@intel.com,
	ira.weiny@intel.com, vishal.l.verma@intel.com,
	alison.schofield@intel.com, Bjorn Helgaas <bhelgaas@google.com>,
	Stefan Roese <sr@denx.de>,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>
Subject: Re: [PATCH v5] cxl: add RAS status unmasking for CXL
Date: Thu, 29 Dec 2022 11:27:31 -0600	[thread overview]
Message-ID: <20221229172731.GA611562@bhelgaas> (raw)
In-Reply-To: <20221217175204.0000170a@huawei.com>

[+cc Stefan, Kuppuswamy]

On Sat, Dec 17, 2022 at 05:52:04PM +0000, Jonathan Cameron wrote:
> On Fri, 16 Dec 2022 09:10:02 -0700
> Dave Jiang <dave.jiang@intel.com> 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.

> +CC Bjorn for a question on the AER control element of this.

Timely question (unlike my response, sorry, I've been on vacation).

> I realized that adding this patch still only enables error because I
> didn't check the PCIe spec when writing the QEMU emulation. I had
> changed the value of  "Correctable Internal Error Mask" to default
> to unmasked.  PCIe 6.0 says it defaults to masked.  For some reason
> I thought these masks were impdef (should have checked ;)

I assume you refer to the AER "Corrected Internal Error Mask" bit
(PCIe r6.0, sec 7.8.4.6), which indeed defaults to 1b (masked) if the
bit is implemented.

> I'll drop that change in QEMU, but upshot is that if we want the
> correctable ones to be delivered, we'll need to clear that bit as
> well as the ones you do in this patch.

I don't think Corrected Internal Error Mask affects reporting of other
Correctable Errors, e.g., Receiver Error, Bad TLP, Bad DLLP, etc.  As
I read sec 6.2.10, those would not be classified as "internal" errors.

> This got me thinking about why I never saw the same issue for UNC
> errors.  Upshot, QEMU never sets the UNCOR mask so defaults to everything on
> (it also doesn't allow anyone to write the register). Curiously it has
> the code that uses the mask even though there was no means to set any
> bits in it. 
> I'll fix that (draft patch below) + we should update lspci to cover more of these AER
> flags as it's a PITA debugging by reading the hex dumps!
> 
> Bjorn, is there any convention on drivers enabling these 'default' masked
> AER errors?  Or is expectation that this is policy and userspace should
> be dealing with it?

I think AER configuration should generally be a system policy
decision, not a driver-level choice (unless we need quirks to work
around device defects, of course).

We now have f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is
native"), which turns on error reporting in Device Control for all
devices at enumeration-time when the OS has control of AER.  But this
is only the generic device-level control; it doesn't configure any
*AER* registers.

I'm surprised to learn that the only writes to PCI_ERR_UNCOR_MASK are
some mips and powerpc arch-specific code and a few individual drivers.
It seems like maybe pci_aer_init() should do some more configuration
of the AER mask and severity registers.

Bjorn