[PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
determine the PCIe AER Capability ownership between OS and
firmware. This support is added based on following spec
reference.

Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
flags field BIT-0 and BIT-1 can be used to expose the
ownership of error source between firmware and OSPM.

Bit [0] - FIRMWARE_FIRST: If set, indicates that system
          firmware will handle errors from this source
          first.
Bit [1] – GLOBAL: If set, indicates that the settings
          contained in this structure apply globally to all
          PCI Express Bridges.

Although above spec reference states that setting
FIRMWARE_FIRST bit means firmware will handle the error source
first, it does not explicitly state anything about AER
ownership. So using HEST to determine AER ownership is
incorrect.

Also, as per following specification references, _OSC can be
used to negotiate the AER ownership between firmware and OS.
Details are,

Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
of _OSC Control Field” and as per PCI firmware specification r3.2,
sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
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.

Since above spec references clearly states _OSC can be used to
determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/acpi/pci_root.c    |   9 +--
 drivers/pci/pcie/aer.c     | 122 ++-----------------------------------
 drivers/pci/pcie/portdrv.h |   9 ---
 include/linux/pci-acpi.h   |   6 --
 include/linux/pci.h        |   2 -
 5 files changed, 6 insertions(+), 142 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ac8ad6cb82aa..9e235c1a75ff 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available()) {
-		if (aer_acpi_firmware_first())
-			dev_info(&device->dev,
-				 "PCIe AER handled by firmware\n");
-		else
-			control |= OSC_PCI_EXPRESS_AER_CONTROL;
-	}
+	if (pci_aer_available())
+		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
 	/*
 	 * Per the Downstream Port Containment Related Enhancements ECN to
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f4274d301235..85d73d28ec26 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -217,133 +217,19 @@ void pcie_ecrc_get_policy(char *str)
 }
 #endif	/* CONFIG_PCIE_ECRC */
 
-#ifdef CONFIG_ACPI_APEI
-static inline int hest_match_pci(struct acpi_hest_aer_common *p,
-				 struct pci_dev *pci)
-{
-	return   ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(pci->bus) &&
-		 ACPI_HEST_BUS(p->bus)     == pci->bus->number &&
-		 p->device                 == PCI_SLOT(pci->devfn) &&
-		 p->function               == PCI_FUNC(pci->devfn);
-}
-
-static inline bool hest_match_type(struct acpi_hest_header *hest_hdr,
-				struct pci_dev *dev)
-{
-	u16 hest_type = hest_hdr->type;
-	u8 pcie_type = pci_pcie_type(dev);
-
-	if ((hest_type == ACPI_HEST_TYPE_AER_ROOT_PORT &&
-		pcie_type == PCI_EXP_TYPE_ROOT_PORT) ||
-	    (hest_type == ACPI_HEST_TYPE_AER_ENDPOINT &&
-		pcie_type == PCI_EXP_TYPE_ENDPOINT) ||
-	    (hest_type == ACPI_HEST_TYPE_AER_BRIDGE &&
-		(dev->class >> 16) == PCI_BASE_CLASS_BRIDGE))
-		return true;
-	return false;
-}
-
-struct aer_hest_parse_info {
-	struct pci_dev *pci_dev;
-	int firmware_first;
-};
-
-static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr)
-{
-	if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT ||
-	    hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT ||
-	    hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE)
-		return 1;
-	return 0;
-}
-
-static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
-{
-	struct aer_hest_parse_info *info = data;
-	struct acpi_hest_aer_common *p;
-	int ff;
-
-	if (!hest_source_is_pcie_aer(hest_hdr))
-		return 0;
-
-	p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
-	ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
-
-	/*
-	 * If no specific device is supplied, determine whether
-	 * FIRMWARE_FIRST is set for *any* PCIe device.
-	 */
-	if (!info->pci_dev) {
-		info->firmware_first |= ff;
-		return 0;
-	}
-
-	/* Otherwise, check the specific device */
-	if (p->flags & ACPI_HEST_GLOBAL) {
-		if (hest_match_type(hest_hdr, info->pci_dev))
-			info->firmware_first = ff;
-	} else
-		if (hest_match_pci(p, info->pci_dev))
-			info->firmware_first = ff;
-
-	return 0;
-}
-
-static void aer_set_firmware_first(struct pci_dev *pci_dev)
-{
-	int rc;
-	struct aer_hest_parse_info info = {
-		.pci_dev	= pci_dev,
-		.firmware_first	= 0,
-	};
-
-	rc = apei_hest_parse(aer_hest_parse, &info);
-
-	if (rc)
-		pci_dev->__aer_firmware_first = 0;
-	else
-		pci_dev->__aer_firmware_first = info.firmware_first;
-	pci_dev->__aer_firmware_first_valid = 1;
-}
-
 int pcie_aer_get_firmware_first(struct pci_dev *dev)
 {
+	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
+
 	if (!pci_is_pcie(dev))
 		return 0;
 
 	if (pcie_ports_native)
 		return 0;
 
-	if (!dev->__aer_firmware_first_valid)
-		aer_set_firmware_first(dev);
-	return dev->__aer_firmware_first;
+	return !host->native_aer;
 }
 
-static bool aer_firmware_first;
-
-/**
- * aer_acpi_firmware_first - Check if APEI should control AER.
- */
-bool aer_acpi_firmware_first(void)
-{
-	static bool parsed = false;
-	struct aer_hest_parse_info info = {
-		.pci_dev	= NULL,	/* Check all PCIe devices */
-		.firmware_first	= 0,
-	};
-
-	if (pcie_ports_native)
-		return false;
-
-	if (!parsed) {
-		apei_hest_parse(aer_hest_parse, &info);
-		aer_firmware_first = info.firmware_first;
-		parsed = true;
-	}
-	return aer_firmware_first;
-}
-#endif
-
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
@@ -1523,7 +1409,7 @@ static struct pcie_port_service_driver aerdriver = {
  */
 int __init pcie_aer_init(void)
 {
-	if (!pci_aer_available() || aer_acpi_firmware_first())
+	if (!pci_aer_available())
 		return -ENXIO;
 	return pcie_port_service_register(&aerdriver);
 }
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 64b5e081cdb2..c05b49d740f4 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -147,16 +147,7 @@ static inline bool pcie_pme_no_msi(void) { return false; }
 static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
 #endif /* !CONFIG_PCIE_PME */
 
-#ifdef CONFIG_ACPI_APEI
 int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
-#else
-static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
-{
-	if (pci_dev->__aer_firmware_first_valid)
-		return pci_dev->__aer_firmware_first;
-	return 0;
-}
-#endif
 
 struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
 #endif /* _PORTDRV_H_ */
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 2d155bfb8fbf..11c98875538a 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
 #endif	/* CONFIG_ACPI */
 
-#ifdef CONFIG_ACPI_APEI
-extern bool aer_acpi_firmware_first(void);
-#else
-static inline bool aer_acpi_firmware_first(void) { return false; }
-#endif
-
 #endif	/* _PCI_ACPI_H_ */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 83ce1cdf5676..43f265830eca 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -420,8 +420,6 @@ struct pci_dev {
 	 * mappings to make sure they cannot access arbitrary memory.
 	 */
 	unsigned int	untrusted:1;
-	unsigned int	__aer_firmware_first_valid:1;
-	unsigned int	__aer_firmware_first:1;
 	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
 	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
-- 
2.17.1

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

[+cc Jon, Alexandru]

On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> determine the PCIe AER Capability ownership between OS and
> firmware. This support is added based on following spec
> reference.
> 
> Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> flags field BIT-0 and BIT-1 can be used to expose the
> ownership of error source between firmware and OSPM.
> 
> Bit [0] - FIRMWARE_FIRST: If set, indicates that system
>           firmware will handle errors from this source
>           first.
> Bit [1] – GLOBAL: If set, indicates that the settings
>           contained in this structure apply globally to all
>           PCI Express Bridges.
> 
> Although above spec reference states that setting
> FIRMWARE_FIRST bit means firmware will handle the error source
> first, it does not explicitly state anything about AER
> ownership. So using HEST to determine AER ownership is
> incorrect.
> 
> Also, as per following specification references, _OSC can be
> used to negotiate the AER ownership between firmware and OS.
> Details are,
> 
> Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> of _OSC Control Field” and as per PCI firmware specification r3.2,
> sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> 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.
> 
> Since above spec references clearly states _OSC can be used to
> determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.

I split this up a bit and applied the first part to pci/error to get
it into -next so we can start seeing what breaks.  I won't be too
surprised if we trip over something.

Here's the first part (entire original patch is at
https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com).

commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Date:   Mon Apr 27 18:25:13 2020 -0500

    PCI/AER: Use only _OSC to determine AER ownership
    
    Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of
    AER via bit 3 of the _OSC Control Field.  In the returned value of the
    Control Field:
    
      The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced
      Error Reporting.  ...  after control is transferred to the operating
      system, firmware must not modify the Advanced Error Reporting Capability.
      If control of this feature was requested and denied or was not requested,
      firmware returns this bit set to 0.
    
    Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to
    determine whether to request ownership of the AER Capability.  This was
    based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say
    things like:
    
      Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will
                handle errors from this source first.
    
      Bit [1] - GLOBAL: If set, indicates that the settings contained in this
                structure apply globally to all PCI Express Devices.
    
    These ACPI references don't say anything about ownership of the AER
    Capability.
    
    Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to
    determine whether we have control of the AER Capability.
    
    Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com
    [bhelgaas: commit log, split patches]
    Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ac8ad6cb82aa..9e235c1a75ff 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available()) {
-		if (aer_acpi_firmware_first())
-			dev_info(&device->dev,
-				 "PCIe AER handled by firmware\n");
-		else
-			control |= OSC_PCI_EXPRESS_AER_CONTROL;
-	}
+	if (pci_aer_available())
+		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
 	/*
 	 * Per the Downstream Port Containment Related Enhancements ECN to
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f4274d301235..efc26773cc6d 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
 		aer_set_firmware_first(dev);
 	return dev->__aer_firmware_first;
 }
-
-static bool aer_firmware_first;
-
-/**
- * aer_acpi_firmware_first - Check if APEI should control AER.
- */
-bool aer_acpi_firmware_first(void)
-{
-	static bool parsed = false;
-	struct aer_hest_parse_info info = {
-		.pci_dev	= NULL,	/* Check all PCIe devices */
-		.firmware_first	= 0,
-	};
-
-	if (pcie_ports_native)
-		return false;
-
-	if (!parsed) {
-		apei_hest_parse(aer_hest_parse, &info);
-		aer_firmware_first = info.firmware_first;
-		parsed = true;
-	}
-	return aer_firmware_first;
-}
 #endif
 
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
@@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = {
  */
 int __init pcie_aer_init(void)
 {
-	if (!pci_aer_available() || aer_acpi_firmware_first())
+	if (!pci_aer_available())
 		return -ENXIO;
 	return pcie_port_service_register(&aerdriver);
 }
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 2d155bfb8fbf..11c98875538a 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
 #endif	/* CONFIG_ACPI */
 
-#ifdef CONFIG_ACPI_APEI
-extern bool aer_acpi_firmware_first(void);
-#else
-static inline bool aer_acpi_firmware_first(void) { return false; }
-#endif
-
 #endif	/* _PCI_ACPI_H_ */

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Kuppuswamy, Sathyanarayanan]

Hi Bjorn,

On 4/27/20 5:02 PM, Bjorn Helgaas wrote:
> I split this up a bit and applied the first part to pci/error to get
> it into -next so we can start seeing what breaks.  I won't be too
> surprised if we trip over something.
Any reason for the split? I don't see reason for still keeping HEST
parser.

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

On Mon, Apr 27, 2020 at 08:20:07PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On 4/27/20 5:02 PM, Bjorn Helgaas wrote:
> > I split this up a bit and applied the first part to pci/error to get
> > it into -next so we can start seeing what breaks.  I won't be too
> > surprised if we trip over something.
>
> Any reason for the split? I don't see reason for still keeping HEST
> parser.

I don't either.  My hope is that splitting makes them easier to
understand and review.  I didn't have time last night to finish the
second and explain it.  The second is actually quite different and has
implications for EDR and DPC that need to be considered.

Bjorn

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Derrick, Jonathan]

Sorry I didn't see this before my comments yesterday

For either individual or split set,
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>

Thanks Kuppuswamy

On Mon, 2020-04-27 at 19:02 -0500, Bjorn Helgaas wrote:
> [+cc Jon, Alexandru]
> 
> On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> > determine the PCIe AER Capability ownership between OS and
> > firmware. This support is added based on following spec
> > reference.
> > 
> > Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> > flags field BIT-0 and BIT-1 can be used to expose the
> > ownership of error source between firmware and OSPM.
> > 
> > Bit [0] - FIRMWARE_FIRST: If set, indicates that system
> >           firmware will handle errors from this source
> >           first.
> > Bit [1] – GLOBAL: If set, indicates that the settings
> >           contained in this structure apply globally to all
> >           PCI Express Bridges.
> > 
> > Although above spec reference states that setting
> > FIRMWARE_FIRST bit means firmware will handle the error source
> > first, it does not explicitly state anything about AER
> > ownership. So using HEST to determine AER ownership is
> > incorrect.
> > 
> > Also, as per following specification references, _OSC can be
> > used to negotiate the AER ownership between firmware and OS.
> > Details are,
> > 
> > Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> > of _OSC Control Field” and as per PCI firmware specification r3.2,
> > sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> > 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.
> > 
> > Since above spec references clearly states _OSC can be used to
> > determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
> 
> I split this up a bit and applied the first part to pci/error to get
> it into -next so we can start seeing what breaks.  I won't be too
> surprised if we trip over something.
> 
> Here's the first part (entire original patch is at
> https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com).
> 
> commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
> Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Date:   Mon Apr 27 18:25:13 2020 -0500
> 
>     PCI/AER: Use only _OSC to determine AER ownership
>     
>     Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of
>     AER via bit 3 of the _OSC Control Field.  In the returned value of the
>     Control Field:
>     
>       The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced
>       Error Reporting.  ...  after control is transferred to the operating
>       system, firmware must not modify the Advanced Error Reporting Capability.
>       If control of this feature was requested and denied or was not requested,
>       firmware returns this bit set to 0.
>     
>     Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to
>     determine whether to request ownership of the AER Capability.  This was
>     based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say
>     things like:
>     
>       Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will
>                 handle errors from this source first.
>     
>       Bit [1] - GLOBAL: If set, indicates that the settings contained in this
>                 structure apply globally to all PCI Express Devices.
>     
>     These ACPI references don't say anything about ownership of the AER
>     Capability.
>     
>     Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to
>     determine whether we have control of the AER Capability.
>     
>     Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com
>     [bhelgaas: commit log, split patches]
>     Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ac8ad6cb82aa..9e235c1a75ff 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>  
> -	if (pci_aer_available()) {
> -		if (aer_acpi_firmware_first())
> -			dev_info(&device->dev,
> -				 "PCIe AER handled by firmware\n");
> -		else
> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
> -	}
> +	if (pci_aer_available())
> +		control |= OSC_PCI_EXPRESS_AER_CONTROL;
>  
>  	/*
>  	 * Per the Downstream Port Containment Related Enhancements ECN to
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f4274d301235..efc26773cc6d 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>  		aer_set_firmware_first(dev);
>  	return dev->__aer_firmware_first;
>  }
> -
> -static bool aer_firmware_first;
> -
> -/**
> - * aer_acpi_firmware_first - Check if APEI should control AER.
> - */
> -bool aer_acpi_firmware_first(void)
> -{
> -	static bool parsed = false;
> -	struct aer_hest_parse_info info = {
> -		.pci_dev	= NULL,	/* Check all PCIe devices */
> -		.firmware_first	= 0,
> -	};
> -
> -	if (pcie_ports_native)
> -		return false;
> -
> -	if (!parsed) {
> -		apei_hest_parse(aer_hest_parse, &info);
> -		aer_firmware_first = info.firmware_first;
> -		parsed = true;
> -	}
> -	return aer_firmware_first;
> -}
>  #endif
>  
>  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
> @@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = {
>   */
>  int __init pcie_aer_init(void)
>  {
> -	if (!pci_aer_available() || aer_acpi_firmware_first())
> +	if (!pci_aer_available())
>  		return -ENXIO;
>  	return pcie_port_service_register(&aerdriver);
>  }
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 2d155bfb8fbf..11c98875538a 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>  #endif	/* CONFIG_ACPI */
>  
> -#ifdef CONFIG_ACPI_APEI
> -extern bool aer_acpi_firmware_first(void);
> -#else
> -static inline bool aer_acpi_firmware_first(void) { return false; }
> -#endif
> -
>  #endif	/* _PCI_ACPI_H_ */

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

[+to Mario, Austin, Rafael; Dell folks, I suspect this commit will
break Dell servers but I'd like your opinion]

On Mon, Apr 27, 2020 at 07:02:13PM -0500, Bjorn Helgaas wrote:
> On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> > determine the PCIe AER Capability ownership between OS and
> > firmware. This support is added based on following spec
> > reference.
> > 
> > Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> > flags field BIT-0 and BIT-1 can be used to expose the
> > ownership of error source between firmware and OSPM.
> > 
> > Bit [0] - FIRMWARE_FIRST: If set, indicates that system
> >           firmware will handle errors from this source
> >           first.
> > Bit [1] – GLOBAL: If set, indicates that the settings
> >           contained in this structure apply globally to all
> >           PCI Express Bridges.
> > 
> > Although above spec reference states that setting
> > FIRMWARE_FIRST bit means firmware will handle the error source
> > first, it does not explicitly state anything about AER
> > ownership. So using HEST to determine AER ownership is
> > incorrect.
> > 
> > Also, as per following specification references, _OSC can be
> > used to negotiate the AER ownership between firmware and OS.
> > Details are,
> > 
> > Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> > of _OSC Control Field” and as per PCI firmware specification r3.2,
> > sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> > 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.
> > 
> > Since above spec references clearly states _OSC can be used to
> > determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
> 
> I split this up a bit and applied the first part to pci/error to get
> it into -next so we can start seeing what breaks.  I won't be too
> surprised if we trip over something.
> 
> Here's the first part (entire original patch is at
> https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com).

I *really* want the patch because the current mix of using both _OSC
and FIRMWARE_FIRST to determine AER capability ownership is a mess and
getting worse, but I'm more and more doubtful.

My contention is that if firmware doesn't want the OS to use the AER
capability it should simply decline to grant control via _OSC.

But things like 0584396157ad ("PCI: PCIe AER: honor ACPI HEST FIRMWARE
FIRST mode") [1] suggest that some machines grant AER control to the
OS via _OSC, but still expect the OS to leave AER alone for certain
devices.

I think the FIRMWARE_FIRST language in the ACPI spec is really too
vague to tell the OS not to use the AER Capability, but it seems like
that's what commits like [1] rely on.

The current _OSC definition (PCI Firmware r3.2) applies only to
PNP0A03/PNP0A08 devices, but it's conceivable that it could be
extended to other devices if we need per-device AER Capability
ownership.

[1] https://git.kernel.org/linus/0584396157ad

> commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
> Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Date:   Mon Apr 27 18:25:13 2020 -0500
> 
>     PCI/AER: Use only _OSC to determine AER ownership
>     
>     Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of
>     AER via bit 3 of the _OSC Control Field.  In the returned value of the
>     Control Field:
>     
>       The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced
>       Error Reporting.  ...  after control is transferred to the operating
>       system, firmware must not modify the Advanced Error Reporting Capability.
>       If control of this feature was requested and denied or was not requested,
>       firmware returns this bit set to 0.
>     
>     Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to
>     determine whether to request ownership of the AER Capability.  This was
>     based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say
>     things like:
>     
>       Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will
>                 handle errors from this source first.
>     
>       Bit [1] - GLOBAL: If set, indicates that the settings contained in this
>                 structure apply globally to all PCI Express Devices.
>     
>     These ACPI references don't say anything about ownership of the AER
>     Capability.
>     
>     Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to
>     determine whether we have control of the AER Capability.
>     
>     Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com
>     [bhelgaas: commit log, split patches]
>     Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ac8ad6cb82aa..9e235c1a75ff 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>  
> -	if (pci_aer_available()) {
> -		if (aer_acpi_firmware_first())
> -			dev_info(&device->dev,
> -				 "PCIe AER handled by firmware\n");
> -		else
> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
> -	}
> +	if (pci_aer_available())
> +		control |= OSC_PCI_EXPRESS_AER_CONTROL;
>  
>  	/*
>  	 * Per the Downstream Port Containment Related Enhancements ECN to
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f4274d301235..efc26773cc6d 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>  		aer_set_firmware_first(dev);
>  	return dev->__aer_firmware_first;
>  }
> -
> -static bool aer_firmware_first;
> -
> -/**
> - * aer_acpi_firmware_first - Check if APEI should control AER.
> - */
> -bool aer_acpi_firmware_first(void)
> -{
> -	static bool parsed = false;
> -	struct aer_hest_parse_info info = {
> -		.pci_dev	= NULL,	/* Check all PCIe devices */
> -		.firmware_first	= 0,
> -	};
> -
> -	if (pcie_ports_native)
> -		return false;
> -
> -	if (!parsed) {
> -		apei_hest_parse(aer_hest_parse, &info);
> -		aer_firmware_first = info.firmware_first;
> -		parsed = true;
> -	}
> -	return aer_firmware_first;
> -}
>  #endif
>  
>  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
> @@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = {
>   */
>  int __init pcie_aer_init(void)
>  {
> -	if (!pci_aer_available() || aer_acpi_firmware_first())
> +	if (!pci_aer_available())
>  		return -ENXIO;
>  	return pcie_port_service_register(&aerdriver);
>  }
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 2d155bfb8fbf..11c98875538a 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>  #endif	/* CONFIG_ACPI */
>  
> -#ifdef CONFIG_ACPI_APEI
> -extern bool aer_acpi_firmware_first(void);
> -#else
> -static inline bool aer_acpi_firmware_first(void) { return false; }
> -#endif
> -
>  #endif	/* _PCI_ACPI_H_ */

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Austin.Bolen]

On 4/28/2020 3:37 PM, Bjorn Helgaas wrote:
> [EXTERNAL EMAIL] 
>
> [+to Mario, Austin, Rafael; Dell folks, I suspect this commit will
> break Dell servers but I'd like your opinion]
>
> <snip>
Thanks Bjorn, for the heads up. I checked with our server BIOS team and
they say that only checking _OSC for AER should work on our servers.  We
always configure_OSC and the HEST FIRMWARE_FIRST flag to retain firmware
control of AER so either could be checked.

> I *really* want the patch because the current mix of using both _OSC
> and FIRMWARE_FIRST to determine AER capability ownership is a mess and
> getting worse, but I'm more and more doubtful.
>
> My contention is that if firmware doesn't want the OS to use the AER
> capability it should simply decline to grant control via _OSC.
I agree per spec that _OSC should be used and this was confirmed by the
ACPI working group. Alex had submitted a patch for us [2] to switch to
using _OSC to determine AER ownership following the decision in the ACPI
working group.

[2] https://lkml.org/lkml/2018/11/16/202

>
> But things like 0584396157ad ("PCI: PCIe AER: honor ACPI HEST FIRMWARE
> FIRST mode") [1] suggest that some machines grant AER control to the
> OS via _OSC, but still expect the OS to leave AER alone for certain
> devices.
AFAIK, no Dell server, including the 11G servers mentioned in that
patch, have granted control of AER via _OSC and set HEST FIRMWARE_FIRST
for some devices. I don't think this model is even support by the
ACPI/PCIe standards.  Yes, you can set the bits that way, but there is
no text I've found that says how the OS/firmware should behave in that
scenario. In order to be interoperable, I think someone would need to
standardized how the OS/firmware would could co-ordinate in such a model.
>
> I think the FIRMWARE_FIRST language in the ACPI spec is really too
> vague to tell the OS not to use the AER Capability, but it seems like
> that's what commits like [1] rely on.
>
> The current _OSC definition (PCI Firmware r3.2) applies only to
> PNP0A03/PNP0A08 devices, but it's conceivable that it could be
> extended to other devices if we need per-device AER Capability
> ownership.
>
> [1] https://git.kernel.org/linus/0584396157ad
>
>> commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
>> Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Date:   Mon Apr 27 18:25:13 2020 -0500
>>
>>     PCI/AER: Use only _OSC to determine AER ownership
>>     
>>     Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of
>>     AER via bit 3 of the _OSC Control Field.  In the returned value of the
>>     Control Field:
>>     
>>       The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced
>>       Error Reporting.  ...  after control is transferred to the operating
>>       system, firmware must not modify the Advanced Error Reporting Capability.
>>       If control of this feature was requested and denied or was not requested,
>>       firmware returns this bit set to 0.
>>     
>>     Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to
>>     determine whether to request ownership of the AER Capability.  This was
>>     based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say
>>     things like:
>>     
>>       Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will
>>                 handle errors from this source first.
>>     
>>       Bit [1] - GLOBAL: If set, indicates that the settings contained in this
>>                 structure apply globally to all PCI Express Devices.
>>     
>>     These ACPI references don't say anything about ownership of the AER
>>     Capability.
>>     
>>     Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to
>>     determine whether we have control of the AER Capability.
>>     
>>     Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com
>>     [bhelgaas: commit log, split patches]
>>     Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index ac8ad6cb82aa..9e235c1a75ff 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>>  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>>  
>> -	if (pci_aer_available()) {
>> -		if (aer_acpi_firmware_first())
>> -			dev_info(&device->dev,
>> -				 "PCIe AER handled by firmware\n");
>> -		else
>> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
>> -	}
>> +	if (pci_aer_available())
>> +		control |= OSC_PCI_EXPRESS_AER_CONTROL;
>>  
>>  	/*
>>  	 * Per the Downstream Port Containment Related Enhancements ECN to
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index f4274d301235..efc26773cc6d 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>>  		aer_set_firmware_first(dev);
>>  	return dev->__aer_firmware_first;
>>  }
>> -
>> -static bool aer_firmware_first;
>> -
>> -/**
>> - * aer_acpi_firmware_first - Check if APEI should control AER.
>> - */
>> -bool aer_acpi_firmware_first(void)
>> -{
>> -	static bool parsed = false;
>> -	struct aer_hest_parse_info info = {
>> -		.pci_dev	= NULL,	/* Check all PCIe devices */
>> -		.firmware_first	= 0,
>> -	};
>> -
>> -	if (pcie_ports_native)
>> -		return false;
>> -
>> -	if (!parsed) {
>> -		apei_hest_parse(aer_hest_parse, &info);
>> -		aer_firmware_first = info.firmware_first;
>> -		parsed = true;
>> -	}
>> -	return aer_firmware_first;
>> -}
>>  #endif
>>  
>>  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
>> @@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = {
>>   */
>>  int __init pcie_aer_init(void)
>>  {
>> -	if (!pci_aer_available() || aer_acpi_firmware_first())
>> +	if (!pci_aer_available())
>>  		return -ENXIO;
>>  	return pcie_port_service_register(&aerdriver);
>>  }
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 2d155bfb8fbf..11c98875538a 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>>  #endif	/* CONFIG_ACPI */
>>  
>> -#ifdef CONFIG_ACPI_APEI
>> -extern bool aer_acpi_firmware_first(void);
>> -#else
>> -static inline bool aer_acpi_firmware_first(void) { return false; }
>> -#endif
>> -
>>  #endif	/* _PCI_ACPI_H_ */

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

On Wed, Apr 29, 2020 at 03:24:41PM +0000, Austin.Bolen@dell.com wrote:
> On 4/28/2020 3:37 PM, Bjorn Helgaas wrote:
> > [EXTERNAL EMAIL] 
> >
> > [+to Mario, Austin, Rafael; Dell folks, I suspect this commit will
> > break Dell servers but I'd like your opinion]
> >
> > <snip>
> Thanks Bjorn, for the heads up. I checked with our server BIOS team and
> they say that only checking _OSC for AER should work on our servers.  We
> always configure_OSC and the HEST FIRMWARE_FIRST flag to retain firmware
> control of AER so either could be checked.
> 
> > I *really* want the patch because the current mix of using both _OSC
> > and FIRMWARE_FIRST to determine AER capability ownership is a mess and
> > getting worse, but I'm more and more doubtful.
> >
> > My contention is that if firmware doesn't want the OS to use the AER
> > capability it should simply decline to grant control via _OSC.
>
> I agree per spec that _OSC should be used and this was confirmed by the
> ACPI working group. Alex had submitted a patch for us [2] to switch to
> using _OSC to determine AER ownership following the decision in the ACPI
> working group.

Perfect, thank you!  I had forgotten that Alex posted that.  We should
add credit to him and a link to that discussion.  Thanks again!

> [2] https://lkml.org/lkml/2018/11/16/202
> 
> > But things like 0584396157ad ("PCI: PCIe AER: honor ACPI HEST FIRMWARE
> > FIRST mode") [1] suggest that some machines grant AER control to the
> > OS via _OSC, but still expect the OS to leave AER alone for certain
> > devices.
>
> AFAIK, no Dell server, including the 11G servers mentioned in that
> patch, have granted control of AER via _OSC and set HEST FIRMWARE_FIRST
> for some devices. I don't think this model is even support by the
> ACPI/PCIe standards.  Yes, you can set the bits that way, but there is
> no text I've found that says how the OS/firmware should behave in that
> scenario. In order to be interoperable, I think someone would need to
> standardized how the OS/firmware would could co-ordinate in such a model.

I agree and I want to get Linux out of the current muddle where we
try to make sense out of it.

> > I think the FIRMWARE_FIRST language in the ACPI spec is really too
> > vague to tell the OS not to use the AER Capability, but it seems like
> > that's what commits like [1] rely on.
> >
> > The current _OSC definition (PCI Firmware r3.2) applies only to
> > PNP0A03/PNP0A08 devices, but it's conceivable that it could be
> > extended to other devices if we need per-device AER Capability
> > ownership.
> >
> > [1] https://git.kernel.org/linus/0584396157ad
<snip>

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

[+cc Keith, Sinan, Tyler]

On Wed, Apr 29, 2020 at 03:24:41PM +0000, Austin.Bolen@dell.com wrote:
> On 4/28/2020 3:37 PM, Bjorn Helgaas wrote:
> > [+to Mario, Austin, Rafael; Dell folks, I suspect this commit will
> > break Dell servers but I'd like your opinion]
> >
> > <snip>
> Thanks Bjorn, for the heads up. I checked with our server BIOS team and
> they say that only checking _OSC for AER should work on our servers.  We
> always configure_OSC and the HEST FIRMWARE_FIRST flag to retain firmware
> control of AER so either could be checked.
> 
> > I *really* want the patch because the current mix of using both _OSC
> > and FIRMWARE_FIRST to determine AER capability ownership is a mess and
> > getting worse, but I'm more and more doubtful.
> >
> > My contention is that if firmware doesn't want the OS to use the AER
> > capability it should simply decline to grant control via _OSC.
>
> I agree per spec that _OSC should be used and this was confirmed by the
> ACPI working group. Alex had submitted a patch for us [2] to switch to
> using _OSC to determine AER ownership following the decision in the ACPI
> working group.
> 
> [2] https://lkml.org/lkml/2018/11/16/202

Wow, egg on my face :(  How embarrassing.  Alex posted the *identical*
patch 18 months ago.  There was a lot of discussion the first time,
but I certainly should have applied this the second time around when
that had been resolved.

Anyway, I did apply it now on pci/error for v5.8, and I added links to
Alex's posting as well as his sign-off, since he actually did all the
work earlier.

I apologize for missing this.

This still leaves patch [2/2] from Alex and the second half of Sathy's
patch.  They're similar but not identical, so I'll try to sort that
out next.

> > But things like 0584396157ad ("PCI: PCIe AER: honor ACPI HEST FIRMWARE
> > FIRST mode") [1] suggest that some machines grant AER control to the
> > OS via _OSC, but still expect the OS to leave AER alone for certain
> > devices.
>
> AFAIK, no Dell server, including the 11G servers mentioned in that
> patch, have granted control of AER via _OSC and set HEST FIRMWARE_FIRST
> for some devices. I don't think this model is even support by the
> ACPI/PCIe standards.  Yes, you can set the bits that way, but there is
> no text I've found that says how the OS/firmware should behave in that
> scenario. In order to be interoperable, I think someone would need to
> standardized how the OS/firmware would could co-ordinate in such a model.
> >
> > I think the FIRMWARE_FIRST language in the ACPI spec is really too
> > vague to tell the OS not to use the AER Capability, but it seems like
> > that's what commits like [1] rely on.
> >
> > The current _OSC definition (PCI Firmware r3.2) applies only to
> > PNP0A03/PNP0A08 devices, but it's conceivable that it could be
> > extended to other devices if we need per-device AER Capability
> > ownership.
> >
> > [1] https://git.kernel.org/linus/0584396157ad
> >
> >> commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
> >> Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >> Date:   Mon Apr 27 18:25:13 2020 -0500
> >>
> >>     PCI/AER: Use only _OSC to determine AER ownership
> >>     
> >>     Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of
> >>     AER via bit 3 of the _OSC Control Field.  In the returned value of the
> >>     Control Field:
> >>     
> >>       The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced
> >>       Error Reporting.  ...  after control is transferred to the operating
> >>       system, firmware must not modify the Advanced Error Reporting Capability.
> >>       If control of this feature was requested and denied or was not requested,
> >>       firmware returns this bit set to 0.
> >>     
> >>     Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to
> >>     determine whether to request ownership of the AER Capability.  This was
> >>     based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say
> >>     things like:
> >>     
> >>       Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will
> >>                 handle errors from this source first.
> >>     
> >>       Bit [1] - GLOBAL: If set, indicates that the settings contained in this
> >>                 structure apply globally to all PCI Express Devices.
> >>     
> >>     These ACPI references don't say anything about ownership of the AER
> >>     Capability.
> >>     
> >>     Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to
> >>     determine whether we have control of the AER Capability.
> >>     
> >>     Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com
> >>     [bhelgaas: commit log, split patches]
> >>     Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>
> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> >> index ac8ad6cb82aa..9e235c1a75ff 100644
> >> --- a/drivers/acpi/pci_root.c
> >> +++ b/drivers/acpi/pci_root.c
> >> @@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> >>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> >>  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> >>  
> >> -	if (pci_aer_available()) {
> >> -		if (aer_acpi_firmware_first())
> >> -			dev_info(&device->dev,
> >> -				 "PCIe AER handled by firmware\n");
> >> -		else
> >> -			control |= OSC_PCI_EXPRESS_AER_CONTROL;
> >> -	}
> >> +	if (pci_aer_available())
> >> +		control |= OSC_PCI_EXPRESS_AER_CONTROL;
> >>  
> >>  	/*
> >>  	 * Per the Downstream Port Containment Related Enhancements ECN to
> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >> index f4274d301235..efc26773cc6d 100644
> >> --- a/drivers/pci/pcie/aer.c
> >> +++ b/drivers/pci/pcie/aer.c
> >> @@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
> >>  		aer_set_firmware_first(dev);
> >>  	return dev->__aer_firmware_first;
> >>  }
> >> -
> >> -static bool aer_firmware_first;
> >> -
> >> -/**
> >> - * aer_acpi_firmware_first - Check if APEI should control AER.
> >> - */
> >> -bool aer_acpi_firmware_first(void)
> >> -{
> >> -	static bool parsed = false;
> >> -	struct aer_hest_parse_info info = {
> >> -		.pci_dev	= NULL,	/* Check all PCIe devices */
> >> -		.firmware_first	= 0,
> >> -	};
> >> -
> >> -	if (pcie_ports_native)
> >> -		return false;
> >> -
> >> -	if (!parsed) {
> >> -		apei_hest_parse(aer_hest_parse, &info);
> >> -		aer_firmware_first = info.firmware_first;
> >> -		parsed = true;
> >> -	}
> >> -	return aer_firmware_first;
> >> -}
> >>  #endif
> >>  
> >>  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
> >> @@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = {
> >>   */
> >>  int __init pcie_aer_init(void)
> >>  {
> >> -	if (!pci_aer_available() || aer_acpi_firmware_first())
> >> +	if (!pci_aer_available())
> >>  		return -ENXIO;
> >>  	return pcie_port_service_register(&aerdriver);
> >>  }
> >> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> >> index 2d155bfb8fbf..11c98875538a 100644
> >> --- a/include/linux/pci-acpi.h
> >> +++ b/include/linux/pci-acpi.h
> >> @@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> >>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> >>  #endif	/* CONFIG_ACPI */
> >>  
> >> -#ifdef CONFIG_ACPI_APEI
> >> -extern bool aer_acpi_firmware_first(void);
> >> -#else
> >> -static inline bool aer_acpi_firmware_first(void) { return false; }
> >> -#endif
> >> -
> >>  #endif	/* _PCI_ACPI_H_ */
> 
> 

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

[+cc Austin, Mario, Jon, Alex, Rafael, Keith, Sinan, Tyler]

On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> determine the PCIe AER Capability ownership between OS and
> firmware. This support is added based on following spec
> reference.
> 
> Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> flags field BIT-0 and BIT-1 can be used to expose the
> ownership of error source between firmware and OSPM.
> 
> Bit [0] - FIRMWARE_FIRST: If set, indicates that system
>           firmware will handle errors from this source
>           first.
> Bit [1] – GLOBAL: If set, indicates that the settings
>           contained in this structure apply globally to all
>           PCI Express Bridges.
> 
> Although above spec reference states that setting
> FIRMWARE_FIRST bit means firmware will handle the error source
> first, it does not explicitly state anything about AER
> ownership. So using HEST to determine AER ownership is
> incorrect.
> 
> Also, as per following specification references, _OSC can be
> used to negotiate the AER ownership between firmware and OS.
> Details are,
> 
> Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> of _OSC Control Field” and as per PCI firmware specification r3.2,
> sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> 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.
> 
> Since above spec references clearly states _OSC can be used to
> determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.

I pulled out the _OSC part of this to a separate patch.  What's left
is below, and is essentially equivalent to Alex's patch:

  https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/

I like what this does, but what I don't like is the fact that we now
have this thing called pcie_aer_get_firmware_first() that is not
connected with the ACPI FIRMWARE_FIRST bit at all.

I think the end result will be more readable if we get rid of the
"firmware_first" completely.  I don't know if we need a wrapper for it
at all, or if we should just open-code:

  int pci_enable_pcie_error_reporting(struct pci_dev *dev)
  {
    struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);

    if (!host->native_aer)
      return -EIO;

It's of nice to minimize the number of things you have look at.
We already have native_aer, aer_cap, pcie_ports_native,
pci_aer_available(), ..., so maybe we can live with seeing
pci_find_host_bridge() a few times.

> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f4274d301235..85d73d28ec26 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -217,133 +217,19 @@ void pcie_ecrc_get_policy(char *str)
>  }
>  #endif	/* CONFIG_PCIE_ECRC */
>  
> -#ifdef CONFIG_ACPI_APEI
> -static inline int hest_match_pci(struct acpi_hest_aer_common *p,
> -				 struct pci_dev *pci)
> -{
> -	return   ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(pci->bus) &&
> -		 ACPI_HEST_BUS(p->bus)     == pci->bus->number &&
> -		 p->device                 == PCI_SLOT(pci->devfn) &&
> -		 p->function               == PCI_FUNC(pci->devfn);
> -}
> -
> -static inline bool hest_match_type(struct acpi_hest_header *hest_hdr,
> -				struct pci_dev *dev)
> -{
> -	u16 hest_type = hest_hdr->type;
> -	u8 pcie_type = pci_pcie_type(dev);
> -
> -	if ((hest_type == ACPI_HEST_TYPE_AER_ROOT_PORT &&
> -		pcie_type == PCI_EXP_TYPE_ROOT_PORT) ||
> -	    (hest_type == ACPI_HEST_TYPE_AER_ENDPOINT &&
> -		pcie_type == PCI_EXP_TYPE_ENDPOINT) ||
> -	    (hest_type == ACPI_HEST_TYPE_AER_BRIDGE &&
> -		(dev->class >> 16) == PCI_BASE_CLASS_BRIDGE))
> -		return true;
> -	return false;
> -}
> -
> -struct aer_hest_parse_info {
> -	struct pci_dev *pci_dev;
> -	int firmware_first;
> -};
> -
> -static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr)
> -{
> -	if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT ||
> -	    hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT ||
> -	    hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE)
> -		return 1;
> -	return 0;
> -}
> -
> -static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
> -{
> -	struct aer_hest_parse_info *info = data;
> -	struct acpi_hest_aer_common *p;
> -	int ff;
> -
> -	if (!hest_source_is_pcie_aer(hest_hdr))
> -		return 0;
> -
> -	p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
> -	ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
> -
> -	/*
> -	 * If no specific device is supplied, determine whether
> -	 * FIRMWARE_FIRST is set for *any* PCIe device.
> -	 */
> -	if (!info->pci_dev) {
> -		info->firmware_first |= ff;
> -		return 0;
> -	}
> -
> -	/* Otherwise, check the specific device */
> -	if (p->flags & ACPI_HEST_GLOBAL) {
> -		if (hest_match_type(hest_hdr, info->pci_dev))
> -			info->firmware_first = ff;
> -	} else
> -		if (hest_match_pci(p, info->pci_dev))
> -			info->firmware_first = ff;
> -
> -	return 0;
> -}
> -
> -static void aer_set_firmware_first(struct pci_dev *pci_dev)
> -{
> -	int rc;
> -	struct aer_hest_parse_info info = {
> -		.pci_dev	= pci_dev,
> -		.firmware_first	= 0,
> -	};
> -
> -	rc = apei_hest_parse(aer_hest_parse, &info);
> -
> -	if (rc)
> -		pci_dev->__aer_firmware_first = 0;
> -	else
> -		pci_dev->__aer_firmware_first = info.firmware_first;
> -	pci_dev->__aer_firmware_first_valid = 1;
> -}
> -
>  int pcie_aer_get_firmware_first(struct pci_dev *dev)
>  {
> +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> +
>  	if (!pci_is_pcie(dev))
>  		return 0;
>  
>  	if (pcie_ports_native)
>  		return 0;
>  
> -	if (!dev->__aer_firmware_first_valid)
> -		aer_set_firmware_first(dev);
> -	return dev->__aer_firmware_first;
> +	return !host->native_aer;
>  }

> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index 64b5e081cdb2..c05b49d740f4 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -147,16 +147,7 @@ static inline bool pcie_pme_no_msi(void) { return false; }
>  static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
>  #endif /* !CONFIG_PCIE_PME */
>  
> -#ifdef CONFIG_ACPI_APEI
>  int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
> -#else
> -static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
> -{
> -	if (pci_dev->__aer_firmware_first_valid)
> -		return pci_dev->__aer_firmware_first;
> -	return 0;
> -}
> -#endif
>  
>  struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
>  #endif /* _PORTDRV_H_ */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 83ce1cdf5676..43f265830eca 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -420,8 +420,6 @@ struct pci_dev {
>  	 * mappings to make sure they cannot access arbitrary memory.
>  	 */
>  	unsigned int	untrusted:1;
> -	unsigned int	__aer_firmware_first_valid:1;
> -	unsigned int	__aer_firmware_first:1;
>  	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
>  	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
>  	unsigned int	irq_managed:1;
> -- 
> 2.17.1
> 

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

[Austin, help us understand the FIRMWARE_FIRST bit! :)]

On Thu, Apr 30, 2020 at 05:40:22PM -0500, Bjorn Helgaas wrote:
> On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> > determine the PCIe AER Capability ownership between OS and
> > firmware. This support is added based on following spec
> > reference.
> > 
> > Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> > flags field BIT-0 and BIT-1 can be used to expose the
> > ownership of error source between firmware and OSPM.
> > 
> > Bit [0] - FIRMWARE_FIRST: If set, indicates that system
> >           firmware will handle errors from this source
> >           first.
> > Bit [1] – GLOBAL: If set, indicates that the settings
> >           contained in this structure apply globally to all
> >           PCI Express Bridges.
> > 
> > Although above spec reference states that setting
> > FIRMWARE_FIRST bit means firmware will handle the error source
> > first, it does not explicitly state anything about AER
> > ownership. So using HEST to determine AER ownership is
> > incorrect.
> > 
> > Also, as per following specification references, _OSC can be
> > used to negotiate the AER ownership between firmware and OS.
> > Details are,
> > 
> > Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> > of _OSC Control Field” and as per PCI firmware specification r3.2,
> > sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> > 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.
> > 
> > Since above spec references clearly states _OSC can be used to
> > determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
> 
> I pulled out the _OSC part of this to a separate patch.  What's left
> is below, and is essentially equivalent to Alex's patch:
> 
>   https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/
> 
> I like what this does, but what I don't like is the fact that we now
> have this thing called pcie_aer_get_firmware_first() that is not
> connected with the ACPI FIRMWARE_FIRST bit at all.

Austin, if we remove this, we'll have no PCIe-related code that looks
at the HEST FIRMWARE_FIRST bit at all.  Presumably it's there for some
reason, but I'm not sure what the reason is.

Alex's mail [1] has a nice table of _OSC AER/HEST FFS bits that looks
useful, but the only actionable thing I can see is that in the (1,1)
case, OSPM should do some initialization with masks/enables.

But I have no clue what that means or how to connect that with the
spec.  What are the masks/enables?  Is that something connected with
ERST?

[1] https://lore.kernel.org/r/20190326172343.28946-1-mr.nuke.me@gmail.com/

> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index f4274d301235..85d73d28ec26 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -217,133 +217,19 @@ void pcie_ecrc_get_policy(char *str)
> >  }
> >  #endif	/* CONFIG_PCIE_ECRC */
> >  
> > -#ifdef CONFIG_ACPI_APEI
> > -static inline int hest_match_pci(struct acpi_hest_aer_common *p,
> > -				 struct pci_dev *pci)
> > -{
> > -	return   ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(pci->bus) &&
> > -		 ACPI_HEST_BUS(p->bus)     == pci->bus->number &&
> > -		 p->device                 == PCI_SLOT(pci->devfn) &&
> > -		 p->function               == PCI_FUNC(pci->devfn);
> > -}
> > -
> > -static inline bool hest_match_type(struct acpi_hest_header *hest_hdr,
> > -				struct pci_dev *dev)
> > -{
> > -	u16 hest_type = hest_hdr->type;
> > -	u8 pcie_type = pci_pcie_type(dev);
> > -
> > -	if ((hest_type == ACPI_HEST_TYPE_AER_ROOT_PORT &&
> > -		pcie_type == PCI_EXP_TYPE_ROOT_PORT) ||
> > -	    (hest_type == ACPI_HEST_TYPE_AER_ENDPOINT &&
> > -		pcie_type == PCI_EXP_TYPE_ENDPOINT) ||
> > -	    (hest_type == ACPI_HEST_TYPE_AER_BRIDGE &&
> > -		(dev->class >> 16) == PCI_BASE_CLASS_BRIDGE))
> > -		return true;
> > -	return false;
> > -}
> > -
> > -struct aer_hest_parse_info {
> > -	struct pci_dev *pci_dev;
> > -	int firmware_first;
> > -};
> > -
> > -static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr)
> > -{
> > -	if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT ||
> > -	    hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT ||
> > -	    hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE)
> > -		return 1;
> > -	return 0;
> > -}
> > -
> > -static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
> > -{
> > -	struct aer_hest_parse_info *info = data;
> > -	struct acpi_hest_aer_common *p;
> > -	int ff;
> > -
> > -	if (!hest_source_is_pcie_aer(hest_hdr))
> > -		return 0;
> > -
> > -	p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
> > -	ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
> > -
> > -	/*
> > -	 * If no specific device is supplied, determine whether
> > -	 * FIRMWARE_FIRST is set for *any* PCIe device.
> > -	 */
> > -	if (!info->pci_dev) {
> > -		info->firmware_first |= ff;
> > -		return 0;
> > -	}
> > -
> > -	/* Otherwise, check the specific device */
> > -	if (p->flags & ACPI_HEST_GLOBAL) {
> > -		if (hest_match_type(hest_hdr, info->pci_dev))
> > -			info->firmware_first = ff;
> > -	} else
> > -		if (hest_match_pci(p, info->pci_dev))
> > -			info->firmware_first = ff;
> > -
> > -	return 0;
> > -}
> > -
> > -static void aer_set_firmware_first(struct pci_dev *pci_dev)
> > -{
> > -	int rc;
> > -	struct aer_hest_parse_info info = {
> > -		.pci_dev	= pci_dev,
> > -		.firmware_first	= 0,
> > -	};
> > -
> > -	rc = apei_hest_parse(aer_hest_parse, &info);
> > -
> > -	if (rc)
> > -		pci_dev->__aer_firmware_first = 0;
> > -	else
> > -		pci_dev->__aer_firmware_first = info.firmware_first;
> > -	pci_dev->__aer_firmware_first_valid = 1;
> > -}
> > -
> >  int pcie_aer_get_firmware_first(struct pci_dev *dev)
> >  {
> > +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> > +
> >  	if (!pci_is_pcie(dev))
> >  		return 0;
> >  
> >  	if (pcie_ports_native)
> >  		return 0;
> >  
> > -	if (!dev->__aer_firmware_first_valid)
> > -		aer_set_firmware_first(dev);
> > -	return dev->__aer_firmware_first;
> > +	return !host->native_aer;
> >  }
> 
> > diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> > index 64b5e081cdb2..c05b49d740f4 100644
> > --- a/drivers/pci/pcie/portdrv.h
> > +++ b/drivers/pci/pcie/portdrv.h
> > @@ -147,16 +147,7 @@ static inline bool pcie_pme_no_msi(void) { return false; }
> >  static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
> >  #endif /* !CONFIG_PCIE_PME */
> >  
> > -#ifdef CONFIG_ACPI_APEI
> >  int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
> > -#else
> > -static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
> > -{
> > -	if (pci_dev->__aer_firmware_first_valid)
> > -		return pci_dev->__aer_firmware_first;
> > -	return 0;
> > -}
> > -#endif
> >  
> >  struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
> >  #endif /* _PORTDRV_H_ */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 83ce1cdf5676..43f265830eca 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -420,8 +420,6 @@ struct pci_dev {
> >  	 * mappings to make sure they cannot access arbitrary memory.
> >  	 */
> >  	unsigned int	untrusted:1;
> > -	unsigned int	__aer_firmware_first_valid:1;
> > -	unsigned int	__aer_firmware_first:1;
> >  	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
> >  	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
> >  	unsigned int	irq_managed:1;
> > -- 
> > 2.17.1
> > 

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Austin.Bolen]

On 4/30/2020 6:02 PM, Bjorn Helgaas wrote:
> [EXTERNAL EMAIL] 
>
> [Austin, help us understand the FIRMWARE_FIRST bit! :)]
>
> On Thu, Apr 30, 2020 at 05:40:22PM -0500, Bjorn Helgaas wrote:
>> On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>
>>> Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
>>> determine the PCIe AER Capability ownership between OS and
>>> firmware. This support is added based on following spec
>>> reference.
>>>
>>> Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
>>> flags field BIT-0 and BIT-1 can be used to expose the
>>> ownership of error source between firmware and OSPM.
>>>
>>> Bit [0] - FIRMWARE_FIRST: If set, indicates that system
>>>           firmware will handle errors from this source
>>>           first.
>>> Bit [1] – GLOBAL: If set, indicates that the settings
>>>           contained in this structure apply globally to all
>>>           PCI Express Bridges.
>>>
>>> Although above spec reference states that setting
>>> FIRMWARE_FIRST bit means firmware will handle the error source
>>> first, it does not explicitly state anything about AER
>>> ownership. So using HEST to determine AER ownership is
>>> incorrect.
>>>
>>> Also, as per following specification references, _OSC can be
>>> used to negotiate the AER ownership between firmware and OS.
>>> Details are,
>>>
>>> Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
>>> of _OSC Control Field” and as per PCI firmware specification r3.2,
>>> sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
>>> 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.
>>>
>>> Since above spec references clearly states _OSC can be used to
>>> determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
>> I pulled out the _OSC part of this to a separate patch.  What's left
>> is below, and is essentially equivalent to Alex's patch:
>>
>>   https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/
>>
>> I like what this does, but what I don't like is the fact that we now
>> have this thing called pcie_aer_get_firmware_first() that is not
>> connected with the ACPI FIRMWARE_FIRST bit at all.
> Austin, if we remove this, we'll have no PCIe-related code that looks
> at the HEST FIRMWARE_FIRST bit at all.  Presumably it's there for some
> reason, but I'm not sure what the reason is.
>
> Alex's mail [1] has a nice table of _OSC AER/HEST FFS bits that looks
> useful, but the only actionable thing I can see is that in the (1,1)
> case, OSPM should do some initialization with masks/enables.
>
> But I have no clue what that means or how to connect that with the
> spec.  What are the masks/enables?  Is that something connected with
> ERST?
>
> [1] https://lore.kernel.org/r/20190326172343.28946-1-mr.nuke.me@gmail.com/

The only values that make sense to me are (1, 0) for full native OS
init/handling of AER and (0, 1) for the firmware first model where
firmware does the init and handles errors first then passes control to
the OS. For these cases the FIRMWARE_FIRST flag in HEST is redundant and
not needed.

We did discuss the (1, 1) case in the ACPI working group and there were
a potential use case (which Alex documented in the link you provided)
but there is nothing specified in the standard about how that model
would actually work AFAICT. And no x86 system has the hardware support
needed for what was proposed that I'm aware of (not sure about other
architectures).

So unless and until someone documents how the firmware and OS are
supposed to behave in the (1, 1) or (0, 0) scenario and expresses a need
for those models, I would not bother adding support for them.  Just my 2
cents.

>
>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>> index f4274d301235..85d73d28ec26 100644
>>> --- a/drivers/pci/pcie/aer.c
>>> +++ b/drivers/pci/pcie/aer.c
>>> @@ -217,133 +217,19 @@ void pcie_ecrc_get_policy(char *str)
>>>  }
>>>  #endif	/* CONFIG_PCIE_ECRC */
>>>  
>>> -#ifdef CONFIG_ACPI_APEI
>>> -static inline int hest_match_pci(struct acpi_hest_aer_common *p,
>>> -				 struct pci_dev *pci)
>>> -{
>>> -	return   ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(pci->bus) &&
>>> -		 ACPI_HEST_BUS(p->bus)     == pci->bus->number &&
>>> -		 p->device                 == PCI_SLOT(pci->devfn) &&
>>> -		 p->function               == PCI_FUNC(pci->devfn);
>>> -}
>>> -
>>> -static inline bool hest_match_type(struct acpi_hest_header *hest_hdr,
>>> -				struct pci_dev *dev)
>>> -{
>>> -	u16 hest_type = hest_hdr->type;
>>> -	u8 pcie_type = pci_pcie_type(dev);
>>> -
>>> -	if ((hest_type == ACPI_HEST_TYPE_AER_ROOT_PORT &&
>>> -		pcie_type == PCI_EXP_TYPE_ROOT_PORT) ||
>>> -	    (hest_type == ACPI_HEST_TYPE_AER_ENDPOINT &&
>>> -		pcie_type == PCI_EXP_TYPE_ENDPOINT) ||
>>> -	    (hest_type == ACPI_HEST_TYPE_AER_BRIDGE &&
>>> -		(dev->class >> 16) == PCI_BASE_CLASS_BRIDGE))
>>> -		return true;
>>> -	return false;
>>> -}
>>> -
>>> -struct aer_hest_parse_info {
>>> -	struct pci_dev *pci_dev;
>>> -	int firmware_first;
>>> -};
>>> -
>>> -static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr)
>>> -{
>>> -	if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT ||
>>> -	    hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT ||
>>> -	    hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE)
>>> -		return 1;
>>> -	return 0;
>>> -}
>>> -
>>> -static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
>>> -{
>>> -	struct aer_hest_parse_info *info = data;
>>> -	struct acpi_hest_aer_common *p;
>>> -	int ff;
>>> -
>>> -	if (!hest_source_is_pcie_aer(hest_hdr))
>>> -		return 0;
>>> -
>>> -	p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
>>> -	ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
>>> -
>>> -	/*
>>> -	 * If no specific device is supplied, determine whether
>>> -	 * FIRMWARE_FIRST is set for *any* PCIe device.
>>> -	 */
>>> -	if (!info->pci_dev) {
>>> -		info->firmware_first |= ff;
>>> -		return 0;
>>> -	}
>>> -
>>> -	/* Otherwise, check the specific device */
>>> -	if (p->flags & ACPI_HEST_GLOBAL) {
>>> -		if (hest_match_type(hest_hdr, info->pci_dev))
>>> -			info->firmware_first = ff;
>>> -	} else
>>> -		if (hest_match_pci(p, info->pci_dev))
>>> -			info->firmware_first = ff;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static void aer_set_firmware_first(struct pci_dev *pci_dev)
>>> -{
>>> -	int rc;
>>> -	struct aer_hest_parse_info info = {
>>> -		.pci_dev	= pci_dev,
>>> -		.firmware_first	= 0,
>>> -	};
>>> -
>>> -	rc = apei_hest_parse(aer_hest_parse, &info);
>>> -
>>> -	if (rc)
>>> -		pci_dev->__aer_firmware_first = 0;
>>> -	else
>>> -		pci_dev->__aer_firmware_first = info.firmware_first;
>>> -	pci_dev->__aer_firmware_first_valid = 1;
>>> -}
>>> -
>>>  int pcie_aer_get_firmware_first(struct pci_dev *dev)
>>>  {
>>> +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>>> +
>>>  	if (!pci_is_pcie(dev))
>>>  		return 0;
>>>  
>>>  	if (pcie_ports_native)
>>>  		return 0;
>>>  
>>> -	if (!dev->__aer_firmware_first_valid)
>>> -		aer_set_firmware_first(dev);
>>> -	return dev->__aer_firmware_first;
>>> +	return !host->native_aer;
>>>  }
>>> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
>>> index 64b5e081cdb2..c05b49d740f4 100644
>>> --- a/drivers/pci/pcie/portdrv.h
>>> +++ b/drivers/pci/pcie/portdrv.h
>>> @@ -147,16 +147,7 @@ static inline bool pcie_pme_no_msi(void) { return false; }
>>>  static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
>>>  #endif /* !CONFIG_PCIE_PME */
>>>  
>>> -#ifdef CONFIG_ACPI_APEI
>>>  int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
>>> -#else
>>> -static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
>>> -{
>>> -	if (pci_dev->__aer_firmware_first_valid)
>>> -		return pci_dev->__aer_firmware_first;
>>> -	return 0;
>>> -}
>>> -#endif
>>>  
>>>  struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
>>>  #endif /* _PORTDRV_H_ */
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 83ce1cdf5676..43f265830eca 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -420,8 +420,6 @@ struct pci_dev {
>>>  	 * mappings to make sure they cannot access arbitrary memory.
>>>  	 */
>>>  	unsigned int	untrusted:1;
>>> -	unsigned int	__aer_firmware_first_valid:1;
>>> -	unsigned int	__aer_firmware_first:1;
>>>  	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
>>>  	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
>>>  	unsigned int	irq_managed:1;
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

On Fri, May 01, 2020 at 02:40:23PM +0000, Austin.Bolen@dell.com wrote:
> On 4/30/2020 6:02 PM, Bjorn Helgaas wrote:
> > [Austin, help us understand the FIRMWARE_FIRST bit! :)]
> >
> > On Thu, Apr 30, 2020 at 05:40:22PM -0500, Bjorn Helgaas wrote:
> >> On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> >>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>>
> >>> Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> >>> determine the PCIe AER Capability ownership between OS and
> >>> firmware. This support is added based on following spec
> >>> reference.
> >>>
> >>> Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> >>> flags field BIT-0 and BIT-1 can be used to expose the
> >>> ownership of error source between firmware and OSPM.
> >>>
> >>> Bit [0] - FIRMWARE_FIRST: If set, indicates that system
> >>>           firmware will handle errors from this source
> >>>           first.
> >>> Bit [1] – GLOBAL: If set, indicates that the settings
> >>>           contained in this structure apply globally to all
> >>>           PCI Express Bridges.
> >>>
> >>> Although above spec reference states that setting
> >>> FIRMWARE_FIRST bit means firmware will handle the error source
> >>> first, it does not explicitly state anything about AER
> >>> ownership. So using HEST to determine AER ownership is
> >>> incorrect.
> >>>
> >>> Also, as per following specification references, _OSC can be
> >>> used to negotiate the AER ownership between firmware and OS.
> >>> Details are,
> >>>
> >>> Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> >>> of _OSC Control Field” and as per PCI firmware specification r3.2,
> >>> sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> >>> 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.
> >>>
> >>> Since above spec references clearly states _OSC can be used to
> >>> determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
> >> I pulled out the _OSC part of this to a separate patch.  What's left
> >> is below, and is essentially equivalent to Alex's patch:
> >>
> >>   https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/
> >>
> >> I like what this does, but what I don't like is the fact that we now
> >> have this thing called pcie_aer_get_firmware_first() that is not
> >> connected with the ACPI FIRMWARE_FIRST bit at all.
> >
> > Austin, if we remove this, we'll have no PCIe-related code that looks
> > at the HEST FIRMWARE_FIRST bit at all.  Presumably it's there for some
> > reason, but I'm not sure what the reason is.
> >
> > Alex's mail [1] has a nice table of _OSC AER/HEST FFS bits that looks
> > useful, but the only actionable thing I can see is that in the (1,1)
> > case, OSPM should do some initialization with masks/enables.
> >
> > But I have no clue what that means or how to connect that with the
> > spec.  What are the masks/enables?  Is that something connected with
> > ERST?
> >
> > [1] https://lore.kernel.org/r/20190326172343.28946-1-mr.nuke.me@gmail.com/
> 
> The only values that make sense to me are (1, 0) for full native OS
> init/handling of AER and (0, 1) for the firmware first model where
> firmware does the init and handles errors first then passes control to
> the OS. For these cases the FIRMWARE_FIRST flag in HEST is redundant and
> not needed.
> 
> We did discuss the (1, 1) case in the ACPI working group and there were
> a potential use case (which Alex documented in the link you provided)
> but there is nothing specified in the standard about how that model
> would actually work AFAICT. And no x86 system has the hardware support
> needed for what was proposed that I'm aware of (not sure about other
> architectures).
> 
> So unless and until someone documents how the firmware and OS are
> supposed to behave in the (1, 1) or (0, 0) scenario and expresses a need
> for those models, I would not bother adding support for them.  Just my 2
> cents.

Perfect, I think we should ignore the FIRMWARE_FIRST bit in the HEST
PCIe entries for now.  Thanks a lot for your help with this!

Bjorn

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Kuppuswamy, Sathyanarayanan]

Hi Bjorn,

On 4/30/2020 3:40 PM, Bjorn Helgaas wrote:
> [+cc Austin, Mario, Jon, Alex, Rafael, Keith, Sinan, Tyler]
> 
> On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
>> determine the PCIe AER Capability ownership between OS and
>> firmware. This support is added based on following spec
>> reference.
>>
>> Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
>> flags field BIT-0 and BIT-1 can be used to expose the
>> ownership of error source between firmware and OSPM.
>>
>> Bit [0] - FIRMWARE_FIRST: If set, indicates that system
>>            firmware will handle errors from this source
>>            first.
>> Bit [1] – GLOBAL: If set, indicates that the settings
>>            contained in this structure apply globally to all
>>            PCI Express Bridges.
>>
>> Although above spec reference states that setting
>> FIRMWARE_FIRST bit means firmware will handle the error source
>> first, it does not explicitly state anything about AER
>> ownership. So using HEST to determine AER ownership is
>> incorrect.
>>
>> Also, as per following specification references, _OSC can be
>> used to negotiate the AER ownership between firmware and OS.
>> Details are,
>>
>> Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
>> of _OSC Control Field” and as per PCI firmware specification r3.2,
>> sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
>> 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.
>>
>> Since above spec references clearly states _OSC can be used to
>> determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
> 
> I pulled out the _OSC part of this to a separate patch.  What's left
> is below, and is essentially equivalent to Alex's patch:
> 
>    https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/
> 
> I like what this does, but what I don't like is the fact that we now
> have this thing called pcie_aer_get_firmware_first() that is not
> connected with the ACPI FIRMWARE_FIRST bit at all.
I agree. Since the current function has nothing to do with firmware
first check, we can rename it. May be pci_aer_is_native() ?
> 
> I think the end result will be more readable if we get rid of the
> "firmware_first" completely.  I don't know if we need a wrapper for it
> at all, or if we should just open-code:
Since pcie_aer_get_firmware_first() is used in following exported 
functions, I think we still need to check for "pcie_ports_native"
and "pci_is_pcie()"

pci_enable_pcie_error_reporting()
pci_disable_pcie_error_reporting()
pci_cleanup_aer_uncorrect_error_status()
pci_aer_clear_fatal_status()
pci_cleanup_aer_error_status_regs()
> 
>    int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>    {
>      struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> 
>      if (!host->native_aer)
>        return -EIO;
> 
> It's of nice to minimize the number of things you have look at.
> We already have native_aer, aer_cap, pcie_ports_native,
> pci_aer_available(), ..., so maybe we can live with seeing
> pci_find_host_bridge() a few times.
> 
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index f4274d301235..85d73d28ec26 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -217,133 +217,19 @@ void pcie_ecrc_get_policy(char *str)
>>   }
>>   #endif	/* CONFIG_PCIE_ECRC */
>>   
>> -#ifdef CONFIG_ACPI_APEI
>> -static inline int hest_match_pci(struct acpi_hest_aer_common *p,
>> -				 struct pci_dev *pci)
>> -{
>> -	return   ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(pci->bus) &&
>> -		 ACPI_HEST_BUS(p->bus)     == pci->bus->number &&
>> -		 p->device                 == PCI_SLOT(pci->devfn) &&
>> -		 p->function               == PCI_FUNC(pci->devfn);
>> -}
>> -
>> -static inline bool hest_match_type(struct acpi_hest_header *hest_hdr,
>> -				struct pci_dev *dev)
>> -{
>> -	u16 hest_type = hest_hdr->type;
>> -	u8 pcie_type = pci_pcie_type(dev);
>> -
>> -	if ((hest_type == ACPI_HEST_TYPE_AER_ROOT_PORT &&
>> -		pcie_type == PCI_EXP_TYPE_ROOT_PORT) ||
>> -	    (hest_type == ACPI_HEST_TYPE_AER_ENDPOINT &&
>> -		pcie_type == PCI_EXP_TYPE_ENDPOINT) ||
>> -	    (hest_type == ACPI_HEST_TYPE_AER_BRIDGE &&
>> -		(dev->class >> 16) == PCI_BASE_CLASS_BRIDGE))
>> -		return true;
>> -	return false;
>> -}
>> -
>> -struct aer_hest_parse_info {
>> -	struct pci_dev *pci_dev;
>> -	int firmware_first;
>> -};
>> -
>> -static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr)
>> -{
>> -	if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT ||
>> -	    hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT ||
>> -	    hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE)
>> -		return 1;
>> -	return 0;
>> -}
>> -
>> -static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
>> -{
>> -	struct aer_hest_parse_info *info = data;
>> -	struct acpi_hest_aer_common *p;
>> -	int ff;
>> -
>> -	if (!hest_source_is_pcie_aer(hest_hdr))
>> -		return 0;
>> -
>> -	p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
>> -	ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
>> -
>> -	/*
>> -	 * If no specific device is supplied, determine whether
>> -	 * FIRMWARE_FIRST is set for *any* PCIe device.
>> -	 */
>> -	if (!info->pci_dev) {
>> -		info->firmware_first |= ff;
>> -		return 0;
>> -	}
>> -
>> -	/* Otherwise, check the specific device */
>> -	if (p->flags & ACPI_HEST_GLOBAL) {
>> -		if (hest_match_type(hest_hdr, info->pci_dev))
>> -			info->firmware_first = ff;
>> -	} else
>> -		if (hest_match_pci(p, info->pci_dev))
>> -			info->firmware_first = ff;
>> -
>> -	return 0;
>> -}
>> -
>> -static void aer_set_firmware_first(struct pci_dev *pci_dev)
>> -{
>> -	int rc;
>> -	struct aer_hest_parse_info info = {
>> -		.pci_dev	= pci_dev,
>> -		.firmware_first	= 0,
>> -	};
>> -
>> -	rc = apei_hest_parse(aer_hest_parse, &info);
>> -
>> -	if (rc)
>> -		pci_dev->__aer_firmware_first = 0;
>> -	else
>> -		pci_dev->__aer_firmware_first = info.firmware_first;
>> -	pci_dev->__aer_firmware_first_valid = 1;
>> -}
>> -
>>   int pcie_aer_get_firmware_first(struct pci_dev *dev)
>>   {
>> +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>> +
>>   	if (!pci_is_pcie(dev))
>>   		return 0;
>>   
>>   	if (pcie_ports_native)
>>   		return 0;
>>   
>> -	if (!dev->__aer_firmware_first_valid)
>> -		aer_set_firmware_first(dev);
>> -	return dev->__aer_firmware_first;
>> +	return !host->native_aer;
>>   }
> 
>> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
>> index 64b5e081cdb2..c05b49d740f4 100644
>> --- a/drivers/pci/pcie/portdrv.h
>> +++ b/drivers/pci/pcie/portdrv.h
>> @@ -147,16 +147,7 @@ static inline bool pcie_pme_no_msi(void) { return false; }
>>   static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
>>   #endif /* !CONFIG_PCIE_PME */
>>   
>> -#ifdef CONFIG_ACPI_APEI
>>   int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
>> -#else
>> -static inline int pcie_aer_get_firmware_first(struct pci_dev *pci_dev)
>> -{
>> -	if (pci_dev->__aer_firmware_first_valid)
>> -		return pci_dev->__aer_firmware_first;
>> -	return 0;
>> -}
>> -#endif
>>   
>>   struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
>>   #endif /* _PORTDRV_H_ */
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 83ce1cdf5676..43f265830eca 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -420,8 +420,6 @@ struct pci_dev {
>>   	 * mappings to make sure they cannot access arbitrary memory.
>>   	 */
>>   	unsigned int	untrusted:1;
>> -	unsigned int	__aer_firmware_first_valid:1;
>> -	unsigned int	__aer_firmware_first:1;
>>   	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
>>   	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
>>   	unsigned int	irq_managed:1;
>> -- 
>> 2.17.1
>>

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

[Bjorn Helgaas]

On Thu, Apr 30, 2020 at 05:35:34PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> On 4/30/2020 3:40 PM, Bjorn Helgaas wrote:
> > On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to
> > > determine the PCIe AER Capability ownership between OS and
> > > firmware. This support is added based on following spec
> > > reference.
> > > 
> > > Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table
> > > flags field BIT-0 and BIT-1 can be used to expose the
> > > ownership of error source between firmware and OSPM.
> > > 
> > > Bit [0] - FIRMWARE_FIRST: If set, indicates that system
> > >            firmware will handle errors from this source
> > >            first.
> > > Bit [1] – GLOBAL: If set, indicates that the settings
> > >            contained in this structure apply globally to all
> > >            PCI Express Bridges.
> > > 
> > > Although above spec reference states that setting
> > > FIRMWARE_FIRST bit means firmware will handle the error source
> > > first, it does not explicitly state anything about AER
> > > ownership. So using HEST to determine AER ownership is
> > > incorrect.
> > > 
> > > Also, as per following specification references, _OSC can be
> > > used to negotiate the AER ownership between firmware and OS.
> > > Details are,
> > > 
> > > Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation
> > > of _OSC Control Field” and as per PCI firmware specification r3.2,
> > > sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field
> > > 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.
> > > 
> > > Since above spec references clearly states _OSC can be used to
> > > determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit.
> > 
> > I pulled out the _OSC part of this to a separate patch.  What's left
> > is below, and is essentially equivalent to Alex's patch:
> > 
> >    https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/
> > 
> > I like what this does, but what I don't like is the fact that we now
> > have this thing called pcie_aer_get_firmware_first() that is not
> > connected with the ACPI FIRMWARE_FIRST bit at all.
>
> I agree. Since the current function has nothing to do with firmware
> first check, we can rename it. May be pci_aer_is_native() ?
>
> > I think the end result will be more readable if we get rid of the
> > "firmware_first" completely.  I don't know if we need a wrapper for it
> > at all, or if we should just open-code:
>
> Since pcie_aer_get_firmware_first() is used in following exported functions,
> I think we still need to check for "pcie_ports_native"
> and "pci_is_pcie()"

We *could* check pci_is_pcie(), but I don't think it's strictly
necessary because we really just depend on the AER capability, and we
already check for dev->aer_cap, which will only be set if we find one.

Good point about "pcie_ports_native".  I think I implemented
host->native_aer in a sub-optimal way: instead of sprinkling tests for
pcie_ports_native around, I think I should have done something like
this in acpi_pci_root_create():

  if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL) &&
      !pcie_ports_native)
        host_bridge->native_aer = 0;

That way (1) it's more obvious that the point of pcie_ports_native is
to override _OSC, and (2) we only need to check native_aer elsewhere.

If we refactored like that, the following should be sufficient:

> >    int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> >    {
> >      struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> > 
> >      if (!host->native_aer)
> >        return -EIO;