diff mbox series

[v2,1/2] PCI/AER: Allow Native AER Host Bridges to use AER

Message ID 1587418630-13562-2-git-send-email-jonathan.derrick@intel.com
State New
Headers show
Series Honoring Native AER/DPC Host Bridges | expand

Commit Message

Jon Derrick April 20, 2020, 9:37 p.m. UTC
Some platforms have a mix of ports whose capabilities can be negotiated
by _OSC, and some ports which are not described by ACPI and instead
managed by Native drivers. The existing Firmware-First HEST model can
incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
ports by advertising the HEST Global Flag and matching the type and
class of the port (aer_hest_parse).

If the port requests Native AER through the Host Bridge's capability
settings, the AER driver should honor those settings and allow the port
to bind. This patch changes the definition of Firmware-First to exclude
ports whose Host Bridges request Native AER.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pcie/aer.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Kuppuswamy Sathyanarayanan April 22, 2020, 10:48 p.m. UTC | #1
On 4/20/20 2:37 PM, Jon Derrick wrote:
> Some platforms have a mix of ports whose capabilities can be negotiated
> by _OSC, and some ports which are not described by ACPI and instead
> managed by Native drivers. The existing Firmware-First HEST model can
> incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
> ports by advertising the HEST Global Flag and matching the type and
> class of the port (aer_hest_parse).
Is there a real use case for mixed mode (one host bridge in FF mode and
another in native)?
> 
> If the port requests Native AER through the Host Bridge's capability
> settings, the AER driver should honor those settings and allow the port
> to bind. This patch changes the definition of Firmware-First to exclude
> ports whose Host Bridges request Native AER.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>   drivers/pci/pcie/aer.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f4274d3..30fbd1f 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>   	if (pcie_ports_native)
>   		return 0;
>   
> +	if (pci_find_host_bridge(dev->bus)->native_aer)
> +		return 0;
> +
>   	if (!dev->__aer_firmware_first_valid)
>   		aer_set_firmware_first(dev);
>   	return dev->__aer_firmware_first;
>
Jon Derrick April 23, 2020, 3:11 p.m. UTC | #2
Hi Sathyanarayanan,

On Wed, 2020-04-22 at 15:48 -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> On 4/20/20 2:37 PM, Jon Derrick wrote:
> > Some platforms have a mix of ports whose capabilities can be negotiated
> > by _OSC, and some ports which are not described by ACPI and instead
> > managed by Native drivers. The existing Firmware-First HEST model can
> > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
> > ports by advertising the HEST Global Flag and matching the type and
> > class of the port (aer_hest_parse).
> Is there a real use case for mixed mode (one host bridge in FF mode and
> another in native)?

Intel's VMD exposes PCIe segments containing Root Ports and Bridges and
other DPC consumers. These extra PCIe domains aren't described by ACPI.
There have been a few versions where DPC won't bind due to platform's
HEST configuration.

> > If the port requests Native AER through the Host Bridge's capability
> > settings, the AER driver should honor those settings and allow the port
> > to bind. This patch changes the definition of Firmware-First to exclude
> > ports whose Host Bridges request Native AER.
> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >   drivers/pci/pcie/aer.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index f4274d3..30fbd1f 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
> >   	if (pcie_ports_native)
> >   		return 0;
> >   
> > +	if (pci_find_host_bridge(dev->bus)->native_aer)
> > +		return 0;
> > +
> >   	if (!dev->__aer_firmware_first_valid)
> >   		aer_set_firmware_first(dev);
> >   	return dev->__aer_firmware_first;
> >
Bjorn Helgaas April 24, 2020, 11:30 p.m. UTC | #3
Hi Jon,

I'm glad you raised this because I think the way we handle
FIRMWARE_FIRST is really screwed up.

On Mon, Apr 20, 2020 at 03:37:09PM -0600, Jon Derrick wrote:
> Some platforms have a mix of ports whose capabilities can be negotiated
> by _OSC, and some ports which are not described by ACPI and instead
> managed by Native drivers. The existing Firmware-First HEST model can
> incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
> ports by advertising the HEST Global Flag and matching the type and
> class of the port (aer_hest_parse).
> 
> If the port requests Native AER through the Host Bridge's capability
> settings, the AER driver should honor those settings and allow the port
> to bind. This patch changes the definition of Firmware-First to exclude
> ports whose Host Bridges request Native AER.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pcie/aer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f4274d3..30fbd1f 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>  	if (pcie_ports_native)
>  		return 0;
>  
> +	if (pci_find_host_bridge(dev->bus)->native_aer)
> +		return 0;

I hope we don't have to complicate pcie_aer_get_firmware_first() by
adding this "native_aer" check here.  I'm not sure what we actually
*should* do based on FIRMWARE_FIRST, but I don't think the current
uses really make sense.

I think Linux makes too many assumptions based on the FIRMWARE_FIRST
bit.  The ACPI spec really only says (ACPI v6.3, sec 18.3.2.4):

  If set, FIRMWARE_FIRST indicates to the OSPM that system firmware
  will handle errors from this source first.

  If FIRMWARE_FIRST is set in the flags field, the Enabled field [of
  the HEST AER structure] is ignored by the OSPM.

I do not see anything there about who owns the AER Capability, but
Linux assumes that if FIRMWARE_FIRST is set, firmware must own the AER
Capability.  I think that's reading too much into the spec.

We already have _OSC, which *does* explicitly talk about who owns the
AER Capability, and I think we should rely on that.  If firmware
doesn't want the OS to touch the AER Capability, it should decline to
give ownership to the OS via _OSC.

>  	if (!dev->__aer_firmware_first_valid)
>  		aer_set_firmware_first(dev);
>  	return dev->__aer_firmware_first;
> -- 
> 1.8.3.1
>
Jon Derrick April 27, 2020, 4:11 p.m. UTC | #4
Hi Bjorn,

On Fri, 2020-04-24 at 18:30 -0500, Bjorn Helgaas wrote:
> Hi Jon,
> 
> I'm glad you raised this because I think the way we handle
> FIRMWARE_FIRST is really screwed up.
> 
> On Mon, Apr 20, 2020 at 03:37:09PM -0600, Jon Derrick wrote:
> > Some platforms have a mix of ports whose capabilities can be negotiated
> > by _OSC, and some ports which are not described by ACPI and instead
> > managed by Native drivers. The existing Firmware-First HEST model can
> > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
> > ports by advertising the HEST Global Flag and matching the type and
> > class of the port (aer_hest_parse).
> > 
> > If the port requests Native AER through the Host Bridge's capability
> > settings, the AER driver should honor those settings and allow the port
> > to bind. This patch changes the definition of Firmware-First to exclude
> > ports whose Host Bridges request Native AER.
> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  drivers/pci/pcie/aer.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index f4274d3..30fbd1f 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
> >  	if (pcie_ports_native)
> >  		return 0;
> >  
> > +	if (pci_find_host_bridge(dev->bus)->native_aer)
> > +		return 0;
> 
> I hope we don't have to complicate pcie_aer_get_firmware_first() by
> adding this "native_aer" check here.  I'm not sure what we actually
> *should* do based on FIRMWARE_FIRST, but I don't think the current
> uses really make sense.
> 
> I think Linux makes too many assumptions based on the FIRMWARE_FIRST
> bit.  The ACPI spec really only says (ACPI v6.3, sec 18.3.2.4):
> 
>   If set, FIRMWARE_FIRST indicates to the OSPM that system firmware
>   will handle errors from this source first.
> 
>   If FIRMWARE_FIRST is set in the flags field, the Enabled field [of
>   the HEST AER structure] is ignored by the OSPM.
> 
> I do not see anything there about who owns the AER Capability, but
> Linux assumes that if FIRMWARE_FIRST is set, firmware must own the AER
> Capability.  I think that's reading too much into the spec.
> 
> We already have _OSC, which *does* explicitly talk about who owns the
> AER Capability, and I think we should rely on that.  If firmware
> doesn't want the OS to touch the AER Capability, it should decline to
> give ownership to the OS via _OSC.
> 
> >  	if (!dev->__aer_firmware_first_valid)
> >  		aer_set_firmware_first(dev);
> >  	return dev->__aer_firmware_first;
> > -- 
> > 1.8.3.1
> > 

Just a little bit of reading and my interpretation, as it seems like
some of this is just layers upon layers of possibly conflicting yet
intentionally vague descriptions.

_OSC seems to describe that OSPM can handle AER (6.2.11.3):
PCI Express Advanced Error Reporting (AER) control
   The OS sets this bit to 1 to request control over PCI Express AER.
   If the OS successfully receives control of this feature, it must
   handle error reporting through the AER Capability as described in
   the PCI Express Base Specification.


For AER and DPC the ACPI root port enumeration will properly set
native_aer/dpc based on _OSC:

struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
...
	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
		host_bridge->native_aer = 0;
	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
		host_bridge->native_pme = 0;
	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
		host_bridge->native_ltr = 0;
	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
		host_bridge->native_dpc = 0;

As DPC was defined in an ECN [1], I would imagine AER will need to
cover DPC for legacy platforms prior to the ECN.



The complication is that HEST also seems to describe how ports (and
other devices) are managed either individually or globally:

Table 18-387  PCI Express Root Port AER Structure
...
Flags:
   [0] - FIRMWARE_FIRST: If set, this bit indicates to the OSPM that
   system firmware will handle errors from this source
   [1] - GLOBAL: If set, indicates that the settings contained in this
   structure apply globally to all PCI Express Devices. All other bits
   must be set to zero


The _OSC definition seems to contradict/negate the above FIRMWARE_FIRST
definition that says only firmware will handle errors. It's a bit
different than the IA_32 MCE definition which allows for a GHES_ASSIST
condition, which would cause Firmware 'First', however does allow the
error to be received by OSPM AER via GHES:

Table 18-385  IA-32 Architecture Corrected Machine Check Structure
   [0] - FIRMWARE_FIRST: If set, this bit indicates that system
   firmware will handle errors from this source first.
   [2] - GHES_ASSIST: If set, this bit indicates that although OSPM is
   responsible for directly handling the error (as expected when
   FIRMWARE_FIRST is not set), system firmware reports additional
   information in the context of an interrupt generated by the error.
   The additional information is reported in a Generic Hardware Error
   Source structure with a matching Related Source Id.


I think Linux needs to make an assumption that devices either
enumerated in HEST or enumerated globally by HEST should be managed by
FFS. However it seems that Linux should also be correlating that with
_OSC as _OSC seems to directly contradict and possibly supercede the
HEST expectation.



[1] https://members.pcisig.com/wg/PCI-SIG/document/12888
Bjorn Helgaas April 27, 2020, 10:14 p.m. UTC | #5
On Mon, Apr 27, 2020 at 04:11:07PM +0000, Derrick, Jonathan wrote:
> On Fri, 2020-04-24 at 18:30 -0500, Bjorn Helgaas wrote:
> > I'm glad you raised this because I think the way we handle
> > FIRMWARE_FIRST is really screwed up.
> > 
> > On Mon, Apr 20, 2020 at 03:37:09PM -0600, Jon Derrick wrote:
> > > Some platforms have a mix of ports whose capabilities can be negotiated
> > > by _OSC, and some ports which are not described by ACPI and instead
> > > managed by Native drivers. The existing Firmware-First HEST model can
> > > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
> > > ports by advertising the HEST Global Flag and matching the type and
> > > class of the port (aer_hest_parse).
> > > 
> > > If the port requests Native AER through the Host Bridge's capability
> > > settings, the AER driver should honor those settings and allow the port
> > > to bind. This patch changes the definition of Firmware-First to exclude
> > > ports whose Host Bridges request Native AER.
> > > 
> > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > ---
> > >  drivers/pci/pcie/aer.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > > index f4274d3..30fbd1f 100644
> > > --- a/drivers/pci/pcie/aer.c
> > > +++ b/drivers/pci/pcie/aer.c
> > > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
> > >  	if (pcie_ports_native)
> > >  		return 0;
> > >  
> > > +	if (pci_find_host_bridge(dev->bus)->native_aer)
> > > +		return 0;
> > 
> > I hope we don't have to complicate pcie_aer_get_firmware_first() by
> > adding this "native_aer" check here.  I'm not sure what we actually
> > *should* do based on FIRMWARE_FIRST, but I don't think the current
> > uses really make sense.
> > 
> > I think Linux makes too many assumptions based on the FIRMWARE_FIRST
> > bit.  The ACPI spec really only says (ACPI v6.3, sec 18.3.2.4):
> > 
> >   If set, FIRMWARE_FIRST indicates to the OSPM that system firmware
> >   will handle errors from this source first.
> > 
> >   If FIRMWARE_FIRST is set in the flags field, the Enabled field [of
> >   the HEST AER structure] is ignored by the OSPM.
> > 
> > I do not see anything there about who owns the AER Capability, but
> > Linux assumes that if FIRMWARE_FIRST is set, firmware must own the AER
> > Capability.  I think that's reading too much into the spec.
> > 
> > We already have _OSC, which *does* explicitly talk about who owns the
> > AER Capability, and I think we should rely on that.  If firmware
> > doesn't want the OS to touch the AER Capability, it should decline to
> > give ownership to the OS via _OSC.
> > 
> > >  	if (!dev->__aer_firmware_first_valid)
> > >  		aer_set_firmware_first(dev);
> > >  	return dev->__aer_firmware_first;
> 
> Just a little bit of reading and my interpretation, as it seems like
> some of this is just layers upon layers of possibly conflicting yet
> intentionally vague descriptions.
> 
> _OSC seems to describe that OSPM can handle AER (6.2.11.3):
> PCI Express Advanced Error Reporting (AER) control
>    The OS sets this bit to 1 to request control over PCI Express AER.
>    If the OS successfully receives control of this feature, it must
>    handle error reporting through the AER Capability as described in
>    the PCI Express Base Specification.
> 
> 
> For AER and DPC the ACPI root port enumeration will properly set
> native_aer/dpc based on _OSC:
> 
> struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> ...
> 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> 		host_bridge->native_aer = 0;
> 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> 		host_bridge->native_pme = 0;
> 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
> 		host_bridge->native_ltr = 0;
> 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
> 		host_bridge->native_dpc = 0;
> 
> As DPC was defined in an ECN [1], I would imagine AER will need to
> cover DPC for legacy platforms prior to the ECN.
> 
> 
> 
> The complication is that HEST also seems to describe how ports (and
> other devices) are managed either individually or globally:
> 
> Table 18-387  PCI Express Root Port AER Structure
> ...
> Flags:
>    [0] - FIRMWARE_FIRST: If set, this bit indicates to the OSPM that
>    system firmware will handle errors from this source
>    [1] - GLOBAL: If set, indicates that the settings contained in this
>    structure apply globally to all PCI Express Devices. All other bits
>    must be set to zero
> 
> 
> The _OSC definition seems to contradict/negate the above FIRMWARE_FIRST
> definition that says only firmware will handle errors. It's a bit
> different than the IA_32 MCE definition which allows for a GHES_ASSIST
> condition, which would cause Firmware 'First', however does allow the
> error to be received by OSPM AER via GHES:
> 
> Table 18-385  IA-32 Architecture Corrected Machine Check Structure
>    [0] - FIRMWARE_FIRST: If set, this bit indicates that system
>    firmware will handle errors from this source first.
>    [2] - GHES_ASSIST: If set, this bit indicates that although OSPM is
>    responsible for directly handling the error (as expected when
>    FIRMWARE_FIRST is not set), system firmware reports additional
>    information in the context of an interrupt generated by the error.
>    The additional information is reported in a Generic Hardware Error
>    Source structure with a matching Related Source Id.
> 
> 
> I think Linux needs to make an assumption that devices either
> enumerated in HEST or enumerated globally by HEST should be managed by
> FFS. However it seems that Linux should also be correlating that with
> _OSC as _OSC seems to directly contradict and possibly supercede the
> HEST expectation.

That's basically what Linux been doing -- we've been assuming that if
_OSC declines to grant us control, *or* if FFS is set somewhere, we
shouldn't touch the AER capability.  But this leads to lots of weird
corner cases, and I really doubt that firmware and Linux are
interpreting all these the same way.

What breaks if we change Linux to *only* use _OSC to determine
ownership of the AER capability?  My argument is that firmware doesn't
want the OS to touch the AER capability registers, it should decline
to give the OS control of the AER capability via _OSC.

If _OSC grants control to the OS in a case where firmware doesn't want
the OS to have control, I'd say that's just a firmware defect that
should be worked around with some sort of quirk.

> [1] https://members.pcisig.com/wg/PCI-SIG/document/12888
diff mbox series

Patch

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f4274d3..30fbd1f 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -314,6 +314,9 @@  int pcie_aer_get_firmware_first(struct pci_dev *dev)
 	if (pcie_ports_native)
 		return 0;
 
+	if (pci_find_host_bridge(dev->bus)->native_aer)
+		return 0;
+
 	if (!dev->__aer_firmware_first_valid)
 		aer_set_firmware_first(dev);
 	return dev->__aer_firmware_first;