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, 5 Jan 2023 10:54:06 -0600	[thread overview]
Message-ID: <20230105165406.GA1150163@bhelgaas> (raw)
In-Reply-To: <20230105163127.00005ae2@huawei.com>

On Thu, Jan 05, 2023 at 04:31:27PM +0000, Jonathan Cameron wrote:
> On Thu, 29 Dec 2022 11:27:31 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sat, Dec 17, 2022 at 05:52:04PM +0000, Jonathan Cameron wrote:

> > > 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.
> 
> Spot on.  I keep confusing the correctable / corrected stuff in PCIe.
> Made more confusing by the CXL stuff layered on top.

Great, it wasn't confusing enough already, so CXL rectified that
problem :)

> > 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.
> 
> Sounds good.  Any thoughts on where to get the policy from?
> Feels like an administrator thing rather than a kernel config one
> to me, so maybe pci_aer_init() is too early or we'd benefit from
> a nice easy per device interface to tweak a default?

If we get a solid system-level policy in place and still end up
needing some kind of administrative control, that might be OK.  But we
don't have that solid system policy yet, so I'd like to push on that
before adding admin interfaces.

Bjorn