For review and comment.
Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
to every PCIe root port for which BIOS reports it should, via ACPI
_OSC.
However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
4.0 is the new Platform Environment Control Interface (PECI), which is
a way for OS and BIOS to handshake over which errors for which
components each will handle. One table in ACPI 4.0 is the Hardware
Error Source Table (HEST), where BIOS can define that errors for
certain PCIe devices (or all devices), should be handled by BIOS
("Firmware First mode"), rather than be handled by the OS.
Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
that it may manage such errors, log them to the System Event Log, and
possibly take other actions. The aer driver should honor this, and
not attach itself to devices noted as such.
Signed-off-by: Matt Domsch <[email protected]>
--
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
---
drivers/pci/pcie/aer/aerdrv.h | 4 +-
drivers/pci/pcie/aer/aerdrv_acpi.c | 106 +++++++++++++++++++++++++++++++++++-
drivers/pci/pcie/aer/aerdrv_core.c | 2 +-
include/acpi/actbl1.h | 8 ++-
4 files changed, 112 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
index bbd7428..2e00a22 100644
--- a/drivers/pci/pcie/aer/aerdrv.h
+++ b/drivers/pci/pcie/aer/aerdrv.h
@@ -128,9 +128,9 @@ extern void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
extern irqreturn_t aer_irq(int irq, void *context);
#ifdef CONFIG_ACPI
-extern int aer_osc_setup(struct pcie_device *pciedev);
+extern int aer_osc_setup(struct pcie_device *pciedev, int forceload);
#else
-static inline int aer_osc_setup(struct pcie_device *pciedev)
+static inline int aer_osc_setup(struct pcie_device *pciedev, int forceload)
{
return 0;
}
diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c
index 8edb2f3..10bd83c 100644
--- a/drivers/pci/pcie/aer/aerdrv_acpi.c
+++ b/drivers/pci/pcie/aer/aerdrv_acpi.c
@@ -18,20 +18,112 @@
#include <linux/delay.h>
#include "aerdrv.h"
+static unsigned long parse_aer_hest_xpf_machine_check(struct acpi_hest_xpf_machine_check *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_xpf_error_bank) * p->num_hardware_banks);
+}
+
+static unsigned long parse_aer_hest_xpf_corrected_machine_check(struct acpi_table_hest_xpf_corrected *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_xpf_error_bank) * p->num_hardware_banks);
+}
+
+static unsigned long parse_aer_hest_xpf_nmi(struct acpi_hest_xpf_nmi *p)
+{
+ return sizeof(*p);
+}
+
+static unsigned long parse_hest_generic(struct acpi_hest_generic *p)
+{
+ return sizeof(*p);
+}
+
+static unsigned long parse_hest_aer(void *hdr, int type, struct pcie_device *pciedev, int *firmware_first)
+{
+ struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header);
+ unsigned long rc=0;
+ switch (type) {
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ rc = sizeof(struct acpi_hest_aer_root);
+ break;
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ rc = sizeof(struct acpi_hest_aer);
+ break;
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ rc = sizeof(struct acpi_hest_aer_bridge);
+ break;
+ }
+
+ if (p->flags & ACPI_HEST_AER_FIRMWARE_FIRST &&
+ (p->flags & ACPI_HEST_AER_GLOBAL ||
+ (p->bus == pciedev->port->bus->number &&
+ p->device == PCI_SLOT(pciedev->port->devfn) &&
+ p->function == PCI_FUNC(pciedev->port->devfn))))
+ *firmware_first = 1;
+ return rc;
+}
+
+static int aer_hest_firmware_first(struct acpi_table_header *stdheader, struct pcie_device *pciedev)
+{
+ struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader;
+ void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */
+ struct acpi_hest_header *hdr = p;
+
+ int i;
+ int firmware_first = 0;
+
+ for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) {
+ switch (hdr->type) {
+ case ACPI_HEST_TYPE_XPF_MACHINE_CHECK:
+ p += parse_aer_hest_xpf_machine_check(p);
+ break;
+ case ACPI_HEST_TYPE_XPF_CORRECTED_MACHINE_CHECK:
+ p += parse_aer_hest_xpf_corrected_machine_check(p);
+ break;
+ case ACPI_HEST_TYPE_XPF_NON_MASKABLE_INTERRUPT:
+ p += parse_aer_hest_xpf_nmi(p);
+ break;
+ /* These three should never appear */
+ case ACPI_HEST_TYPE_XPF_UNUSED:
+ case ACPI_HEST_TYPE_IPF_CORRECTED_MACHINE_CHECK:
+ case ACPI_HEST_TYPE_IPF_CORRECTED_PLATFORM_ERROR:
+ break;
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ p += parse_hest_aer(p, hdr->type, pciedev, &firmware_first);
+ break;
+ case ACPI_HEST_TYPE_GENERIC_HARDWARE_ERROR_SOURCE:
+ p += parse_hest_generic(p);
+ break;
+ /* These should never appear either */
+ case ACPI_HEST_TYPE_RESERVED:
+ default:
+ break;
+ }
+ }
+ return firmware_first;
+}
+
/**
* aer_osc_setup - run ACPI _OSC method
* @pciedev: pcie_device which AER is being enabled on
*
* @return: Zero on success. Nonzero otherwise.
*
- * Invoked when PCIE bus loads AER service driver. To avoid conflict with
- * BIOS AER support requires BIOS to yield AER control to OS native driver.
+ * Invoked when PCIE bus loads AER service driver. To avoid conflict
+ * with BIOS AER support requires BIOS to yield AER control to OS
+ * native driver. If HEST is found, and BIOS requires FIRMWARE FIRST
+ * mode, expect the BIOS to continue managing AER.
**/
-int aer_osc_setup(struct pcie_device *pciedev)
+int aer_osc_setup(struct pcie_device *pciedev, int forceload)
{
acpi_status status = AE_NOT_FOUND;
struct pci_dev *pdev = pciedev->port;
acpi_handle handle = NULL;
+ struct acpi_table_header *hest = NULL;
if (acpi_pci_disabled)
return -1;
@@ -51,5 +143,13 @@ int aer_osc_setup(struct pcie_device *pciedev)
return -1;
}
+ status = acpi_get_table(ACPI_SIG_HEST, 1, &hest);
+ if (ACPI_SUCCESS(status)) {
+ if (aer_hest_firmware_first(hest, pciedev) && !forceload) {
+ dev_printk(KERN_DEBUG, &pciedev->device,
+ "PCIe device errors handled by platform firmware\n");
+ return -1;
+ }
+ }
return 0;
}
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 3d88727..cbd959b 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -860,7 +860,7 @@ void aer_delete_rootport(struct aer_rpc *rpc)
*/
int aer_init(struct pcie_device *dev)
{
- if (aer_osc_setup(dev) && !forceload)
+ if (aer_osc_setup(dev, forceload) && !forceload)
return -ENXIO;
return AER_SUCCESS;
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 59ade07..5919d4c 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -558,8 +558,8 @@ struct acpi_hest_header {
enum acpi_hest_types {
ACPI_HEST_TYPE_XPF_MACHINE_CHECK = 0,
ACPI_HEST_TYPE_XPF_CORRECTED_MACHINE_CHECK = 1,
- ACPI_HEST_TYPE_XPF_UNUSED = 2,
- ACPI_HEST_TYPE_XPF_NON_MASKABLE_INTERRUPT = 3,
+ ACPI_HEST_TYPE_XPF_NON_MASKABLE_INTERRUPT = 2,
+ ACPI_HEST_TYPE_XPF_UNUSED = 3,
ACPI_HEST_TYPE_IPF_CORRECTED_MACHINE_CHECK = 4,
ACPI_HEST_TYPE_IPF_CORRECTED_PLATFORM_ERROR = 5,
ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
@@ -630,6 +630,10 @@ struct acpi_hest_aer_common {
u32 advanced_error_capabilities;
};
+/* Flags */
+#define ACPI_HEST_AER_FIRMWARE_FIRST (1)
+#define ACPI_HEST_AER_GLOBAL (1<<1)
+
/* Hardware Error Notification */
struct acpi_hest_notify {
--
1.6.2.5
Hi Matt,
Matt Domsch wrote:
> For review and comment.
>
> Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
> to every PCIe root port for which BIOS reports it should, via ACPI
> _OSC.
>
> However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
> 4.0 is the new Platform Environment Control Interface (PECI), which is
> a way for OS and BIOS to handshake over which errors for which
> components each will handle. One table in ACPI 4.0 is the Hardware
> Error Source Table (HEST), where BIOS can define that errors for
> certain PCIe devices (or all devices), should be handled by BIOS
> ("Firmware First mode"), rather than be handled by the OS.
>
> Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
> that it may manage such errors, log them to the System Event Log, and
> possibly take other actions. The aer driver should honor this, and
> not attach itself to devices noted as such.
>
>
> Signed-off-by: Matt Domsch <[email protected]>
>
Thank you for providing this patch.
This is a good step in the right direction, to support newer platform.
> ---
> drivers/pci/pcie/aer/aerdrv.h | 4 +-
> drivers/pci/pcie/aer/aerdrv_acpi.c | 106 +++++++++++++++++++++++++++++++++++-
> drivers/pci/pcie/aer/aerdrv_core.c | 2 +-
> include/acpi/actbl1.h | 8 ++-
> 4 files changed, 112 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
> index bbd7428..2e00a22 100644
> --- a/drivers/pci/pcie/aer/aerdrv.h
> +++ b/drivers/pci/pcie/aer/aerdrv.h
> @@ -128,9 +128,9 @@ extern void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> extern irqreturn_t aer_irq(int irq, void *context);
>
> #ifdef CONFIG_ACPI
> -extern int aer_osc_setup(struct pcie_device *pciedev);
> +extern int aer_osc_setup(struct pcie_device *pciedev, int forceload);
> #else
> -static inline int aer_osc_setup(struct pcie_device *pciedev)
> +static inline int aer_osc_setup(struct pcie_device *pciedev, int forceload)
> {
> return 0;
> }
> diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c
> index 8edb2f3..10bd83c 100644
> --- a/drivers/pci/pcie/aer/aerdrv_acpi.c
> +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c
> @@ -18,20 +18,112 @@
> #include <linux/delay.h>
> #include "aerdrv.h"
>
> +static unsigned long parse_aer_hest_xpf_machine_check(struct acpi_hest_xpf_machine_check *p)
> +{
> + return sizeof(*p) +
> + (sizeof(struct acpi_hest_xpf_error_bank) * p->num_hardware_banks);
> +}
> +
> +static unsigned long parse_aer_hest_xpf_corrected_machine_check(struct acpi_table_hest_xpf_corrected *p)
> +{
> + return sizeof(*p) +
> + (sizeof(struct acpi_hest_xpf_error_bank) * p->num_hardware_banks);
> +}
> +
> +static unsigned long parse_aer_hest_xpf_nmi(struct acpi_hest_xpf_nmi *p)
> +{
> + return sizeof(*p);
> +}
> +
> +static unsigned long parse_hest_generic(struct acpi_hest_generic *p)
> +{
> + return sizeof(*p);
> +}
> +
> +static unsigned long parse_hest_aer(void *hdr, int type, struct pcie_device *pciedev, int *firmware_first)
> +{
> + struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header);
> + unsigned long rc=0;
> + switch (type) {
> + case ACPI_HEST_TYPE_AER_ROOT_PORT:
> + rc = sizeof(struct acpi_hest_aer_root);
> + break;
> + case ACPI_HEST_TYPE_AER_ENDPOINT:
> + rc = sizeof(struct acpi_hest_aer);
> + break;
> + case ACPI_HEST_TYPE_AER_BRIDGE:
> + rc = sizeof(struct acpi_hest_aer_bridge);
> + break;
> + }
> +
> + if (p->flags & ACPI_HEST_AER_FIRMWARE_FIRST &&
> + (p->flags & ACPI_HEST_AER_GLOBAL ||
> + (p->bus == pciedev->port->bus->number &&
> + p->device == PCI_SLOT(pciedev->port->devfn) &&
> + p->function == PCI_FUNC(pciedev->port->devfn))))
> + *firmware_first = 1;
> + return rc;
> +}
> +
HEST is neither pcie specific nor pci specific.
As you know it can include error source structure for machine check,
NMI etc.
It will be nice if we can have shareable codes for HEST in proper place,
such as drivers/acpi/_foo_.c (_foo_.c would be hest.c, error.c, apei.c etc.)
... It likely means we will have a function acpi_table_parse_hest() which
is a kin of acpi_table_parse_madt/srat().
> +static int aer_hest_firmware_first(struct acpi_table_header *stdheader, struct pcie_device *pciedev)
> +{
> + struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader;
> + void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */
> + struct acpi_hest_header *hdr = p;
> +
> + int i;
> + int firmware_first = 0;
> +
> + for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) {
> + switch (hdr->type) {
> + case ACPI_HEST_TYPE_XPF_MACHINE_CHECK:
> + p += parse_aer_hest_xpf_machine_check(p);
> + break;
> + case ACPI_HEST_TYPE_XPF_CORRECTED_MACHINE_CHECK:
> + p += parse_aer_hest_xpf_corrected_machine_check(p);
> + break;
> + case ACPI_HEST_TYPE_XPF_NON_MASKABLE_INTERRUPT:
> + p += parse_aer_hest_xpf_nmi(p);
> + break;
> + /* These three should never appear */
> + case ACPI_HEST_TYPE_XPF_UNUSED:
> + case ACPI_HEST_TYPE_IPF_CORRECTED_MACHINE_CHECK:
> + case ACPI_HEST_TYPE_IPF_CORRECTED_PLATFORM_ERROR:
> + break;
> + case ACPI_HEST_TYPE_AER_ROOT_PORT:
> + case ACPI_HEST_TYPE_AER_ENDPOINT:
> + case ACPI_HEST_TYPE_AER_BRIDGE:
> + p += parse_hest_aer(p, hdr->type, pciedev, &firmware_first);
> + break;
> + case ACPI_HEST_TYPE_GENERIC_HARDWARE_ERROR_SOURCE:
> + p += parse_hest_generic(p);
> + break;
> + /* These should never appear either */
> + case ACPI_HEST_TYPE_RESERVED:
> + default:
> + break;
> + }
> + }
> + return firmware_first;
> +}
> +
You could have better code here if there were acpi_table_parse_hest().
> /**
> * aer_osc_setup - run ACPI _OSC method
> * @pciedev: pcie_device which AER is being enabled on
> *
> * @return: Zero on success. Nonzero otherwise.
> *
> - * Invoked when PCIE bus loads AER service driver. To avoid conflict with
> - * BIOS AER support requires BIOS to yield AER control to OS native driver.
> + * Invoked when PCIE bus loads AER service driver. To avoid conflict
> + * with BIOS AER support requires BIOS to yield AER control to OS
> + * native driver. If HEST is found, and BIOS requires FIRMWARE FIRST
> + * mode, expect the BIOS to continue managing AER.
> **/
> -int aer_osc_setup(struct pcie_device *pciedev)
> +int aer_osc_setup(struct pcie_device *pciedev, int forceload)
> {
> acpi_status status = AE_NOT_FOUND;
> struct pci_dev *pdev = pciedev->port;
> acpi_handle handle = NULL;
> + struct acpi_table_header *hest = NULL;
>
> if (acpi_pci_disabled)
> return -1;
> @@ -51,5 +143,13 @@ int aer_osc_setup(struct pcie_device *pciedev)
> return -1;
> }
>
> + status = acpi_get_table(ACPI_SIG_HEST, 1, &hest);
> + if (ACPI_SUCCESS(status)) {
> + if (aer_hest_firmware_first(hest, pciedev) && !forceload) {
> + dev_printk(KERN_DEBUG, &pciedev->device,
> + "PCIe device errors handled by platform firmware\n");
> + return -1;
> + }
> + }
> return 0;
> }
Should we check HEST before taking control of AER via _OSC?
Is the forceload only used to suppress the DEBUG level printk message?
I suppose the better procedure is:
[pseudo code:]
{
if (HEST_indicates_AER_is_firmwarefirst) {
printk "AER is firmware first";
goto NG;
}
if (OSC_failed) {
printk "OSC failed";
goto NG;
}
return 0;
NG:
if (forceload) {
printk "But force loading aerdrv";
return 0;
}
return -1;
}
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 3d88727..cbd959b 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -860,7 +860,7 @@ void aer_delete_rootport(struct aer_rpc *rpc)
> */
> int aer_init(struct pcie_device *dev)
> {
> - if (aer_osc_setup(dev) && !forceload)
> + if (aer_osc_setup(dev, forceload) && !forceload)
> return -ENXIO;
>
> return AER_SUCCESS;
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 59ade07..5919d4c 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -558,8 +558,8 @@ struct acpi_hest_header {
> enum acpi_hest_types {
> ACPI_HEST_TYPE_XPF_MACHINE_CHECK = 0,
> ACPI_HEST_TYPE_XPF_CORRECTED_MACHINE_CHECK = 1,
> - ACPI_HEST_TYPE_XPF_UNUSED = 2,
> - ACPI_HEST_TYPE_XPF_NON_MASKABLE_INTERRUPT = 3,
> + ACPI_HEST_TYPE_XPF_NON_MASKABLE_INTERRUPT = 2,
> + ACPI_HEST_TYPE_XPF_UNUSED = 3,
> ACPI_HEST_TYPE_IPF_CORRECTED_MACHINE_CHECK = 4,
> ACPI_HEST_TYPE_IPF_CORRECTED_PLATFORM_ERROR = 5,
> ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
> @@ -630,6 +630,10 @@ struct acpi_hest_aer_common {
> u32 advanced_error_capabilities;
> };
>
> +/* Flags */
> +#define ACPI_HEST_AER_FIRMWARE_FIRST (1)
> +#define ACPI_HEST_AER_GLOBAL (1<<1)
> +
> /* Hardware Error Notification */
>
> struct acpi_hest_notify {
> -- 1.6.2.5
It seems that these changes in include/acpi/actbl1.h are
already included in pci.git/linux-next (by fix from ACPICA).
Could you revise & rebase this patch on newer tree?
Thanks,
H.Seto
Matt Domsch wrote:
> For review and comment.
>
> Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
> to every PCIe root port for which BIOS reports it should, via ACPI
> _OSC.
>
> However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
> 4.0 is the new Platform Environment Control Interface (PECI), which is
> a way for OS and BIOS to handshake over which errors for which
> components each will handle. One table in ACPI 4.0 is the Hardware
> Error Source Table (HEST), where BIOS can define that errors for
> certain PCIe devices (or all devices), should be handled by BIOS
> ("Firmware First mode"), rather than be handled by the OS.
>
> Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
> that it may manage such errors, log them to the System Event Log, and
> possibly take other actions. The aer driver should honor this, and
> not attach itself to devices noted as such.
>
>
> Signed-off-by: Matt Domsch <[email protected]>
>
In the current AER driver implementation, correctable, non-fatal,
fatal, unsupported request reporting enable bits in PCIe device
control register can be changed by adapter card drivers through pci_enable_pcie_error_reporting() or pci_disable_pcie_error_reporting()
APIs, regardless of _OSC evaluation result.
I'm not sure, but I guess you might need to prevent those bits
from being changed in the Firmware First mode.
Thanks,
Kenji Kaneshige
On Fri, Oct 09, 2009 at 04:11:28PM +0900, Kenji Kaneshige wrote:
> In the current AER driver implementation, correctable, non-fatal,
> fatal, unsupported request reporting enable bits in PCIe device
> control register can be changed by adapter card drivers through pci_enable_pcie_error_reporting() or pci_disable_pcie_error_reporting()
> APIs, regardless of _OSC evaluation result.
>
> I'm not sure, but I guess you might need to prevent those bits
> from being changed in the Firmware First mode.
You are correct. Thank you for the catch. I don't know if we can
prevent changing these bits via the raw operations, but we can prevent
them in these functions. Patch to follow does so.
--
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
For review and comment. Feedback from Hidetoshi Seto and Kenji
Kaneshige incorporated.
Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
to every PCIe root port for which BIOS reports it should, via ACPI
_OSC.
However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
for OS and BIOS to handshake over which errors for which components
each will handle. One table in ACPI 4.0 is the Hardware Error Source
Table (HEST), where BIOS can define that errors for certain PCIe
devices (or all devices), should be handled by BIOS ("Firmware First
mode"), rather than be handled by the OS.
Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
that it may manage such errors, log them to the System Event Log, and
possibly take other actions. The aer driver should honor this, and
not attach itself to devices noted as such.
Furthermore, Kenji Kaneshige reminded us to disallow changing the AER
registers when respecting Firmware First mode. Platform firmware is
expected to manage these, and if changes to them are allowed, it could
break that firmware's behavior.
The HEST parsing code may be replaced in the future by a more
feature-rich implementation. This patch provides the minimum needed
to prevent breakage until that implementation is available.
Signed-off-by: Matt Domsch <[email protected]>
---
drivers/acpi/Makefile | 1 +
drivers/acpi/hest.c | 106 ++++++++++++++++++++++++++++++++++++
drivers/pci/pcie/aer/aerdrv_core.c | 26 ++++++++-
include/acpi/acpi_hest.h | 8 +++
include/linux/pci.h | 1 +
5 files changed, 140 insertions(+), 2 deletions(-)
create mode 100644 drivers/acpi/hest.c
create mode 100644 include/acpi/acpi_hest.h
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 7702118..9ab0d6d 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -19,6 +19,7 @@ obj-y += acpi.o \
# All the builtin files are in the "acpi." module_param namespace.
acpi-y += osl.o utils.o reboot.o
+obj-y += hest.o
# sleep related files
acpi-y += wakeup.o
diff --git a/drivers/acpi/hest.c b/drivers/acpi/hest.c
new file mode 100644
index 0000000..9684f50
--- /dev/null
+++ b/drivers/acpi/hest.c
@@ -0,0 +1,106 @@
+#include <linux/acpi.h>
+#include <linux/pci.h>
+
+static unsigned long parse_acpi_hest_ia_machine_check(struct acpi_hest_ia_machine_check *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
+}
+
+static unsigned long parse_acpi_hest_ia_corrected(struct acpi_hest_ia_corrected *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
+}
+
+static unsigned long parse_acpi_hest_ia_nmi(struct acpi_hest_ia_nmi *p)
+{
+ return sizeof(*p);
+}
+
+static unsigned long parse_acpi_hest_generic(struct acpi_hest_generic *p)
+{
+ return sizeof(*p);
+}
+
+static unsigned long parse_acpi_hest_aer(void *hdr, int type, struct pci_dev *pci, int *firmware_first)
+{
+ struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header);
+ unsigned long rc=0;
+ switch (type) {
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ rc = sizeof(struct acpi_hest_aer_root);
+ break;
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ rc = sizeof(struct acpi_hest_aer);
+ break;
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ rc = sizeof(struct acpi_hest_aer_bridge);
+ break;
+ }
+
+ if (p->flags & ACPI_HEST_FIRMWARE_FIRST &&
+ (p->flags & ACPI_HEST_GLOBAL ||
+ (p->bus == pci->bus->number &&
+ p->device == PCI_SLOT(pci->devfn) &&
+ p->function == PCI_FUNC(pci->devfn))))
+ *firmware_first = 1;
+ return rc;
+}
+
+static int acpi_hest_firmware_first(struct acpi_table_header *stdheader, struct pci_dev *pci)
+{
+ struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader;
+ void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */
+ struct acpi_hest_header *hdr = p;
+
+ int i;
+ int firmware_first = 0;
+
+ for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) {
+ switch (hdr->type) {
+ case ACPI_HEST_TYPE_IA32_CHECK:
+ p += parse_acpi_hest_ia_machine_check(p);
+ break;
+ case ACPI_HEST_TYPE_IA32_CORRECTED_CHECK:
+ p += parse_acpi_hest_ia_corrected(p);
+ break;
+ case ACPI_HEST_TYPE_IA32_NMI:
+ p += parse_acpi_hest_ia_nmi(p);
+ break;
+ /* These three should never appear */
+ case ACPI_HEST_TYPE_NOT_USED3:
+ case ACPI_HEST_TYPE_NOT_USED4:
+ case ACPI_HEST_TYPE_NOT_USED5:
+ break;
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ p += parse_acpi_hest_aer(p, hdr->type, pci, &firmware_first);
+ break;
+ case ACPI_HEST_TYPE_GENERIC_ERROR:
+ p += parse_acpi_hest_generic(p);
+ break;
+ /* These should never appear either */
+ case ACPI_HEST_TYPE_RESERVED:
+ default:
+ break;
+ }
+ }
+ return firmware_first;
+}
+
+int acpi_hest_firmware_first_pci(struct pci_dev *pci)
+{
+ acpi_status status = AE_NOT_FOUND;
+ struct acpi_table_header *hest = NULL;
+ status = acpi_get_table(ACPI_SIG_HEST, 1, &hest);
+
+ if (ACPI_SUCCESS(status)) {
+ if (acpi_hest_firmware_first(hest, pci)) {
+ return 1;
+ }
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 9f5ccbe..aef2db2 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -23,6 +23,7 @@
#include <linux/pm.h>
#include <linux/suspend.h>
#include <linux/delay.h>
+#include <acpi/acpi_hest.h>
#include "aerdrv.h"
static int forceload;
@@ -35,6 +36,9 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
u16 reg16 = 0;
int pos;
+ if (dev->aer_firmware_first)
+ return -EIO;
+
pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
if (!pos)
return -EIO;
@@ -60,6 +64,9 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev)
u16 reg16 = 0;
int pos;
+ if (dev->aer_firmware_first)
+ return -EIO;
+
pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
if (!pos)
return -EIO;
@@ -874,8 +881,23 @@ void aer_delete_rootport(struct aer_rpc *rpc)
*/
int aer_init(struct pcie_device *dev)
{
- if (aer_osc_setup(dev) && !forceload)
- return -ENXIO;
+ if (acpi_hest_firmware_first_pci(dev->port)) {
+ dev_printk(KERN_DEBUG, &dev->device,
+ "PCIe device errors handled by platform firmware.\n");
+ dev->port->aer_firmware_first=1;
+ goto out;
+ }
+
+ if (aer_osc_setup(dev))
+ goto out;
return 0;
+out:
+ if (forceload) {
+ dev_printk(KERN_DEBUG, &dev->device,
+ "aerdrv forceload requested.\n");
+ dev->port->aer_firmware_first=0;
+ return 0;
+ }
+ return -ENXIO;
}
diff --git a/include/acpi/acpi_hest.h b/include/acpi/acpi_hest.h
new file mode 100644
index 0000000..0d41408
--- /dev/null
+++ b/include/acpi/acpi_hest.h
@@ -0,0 +1,8 @@
+#ifndef __ACPI_HEST_H
+#define __ACPI_HEST_H
+
+#include <linux/pci.h>
+
+extern int acpi_hest_firmware_first_pci(struct pci_dev *pci);
+
+#endif
diff --git a/include/linux/pci.h b/include/linux/pci.h
index da4128f..9d646e6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -280,6 +280,7 @@ struct pci_dev {
unsigned int is_virtfn:1;
unsigned int reset_fn:1;
unsigned int is_hotplug_bridge:1;
+ unsigned int aer_firmware_first:1;
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
--
1.6.2.5
On Fri, Oct 09, 2009 at 01:33:20PM -0500, Matt Domsch wrote:
> For review and comment. Feedback from Hidetoshi Seto and Kenji
> Kaneshige incorporated.
With no more comments received, I would like to see this applied.
Thanks,
Matt
--
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
to every PCIe root port for which BIOS reports it should, via ACPI
_OSC.
However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
for OS and BIOS to handshake over which errors for which components
each will handle. One table in ACPI 4.0 is the Hardware Error Source
Table (HEST), where BIOS can define that errors for certain PCIe
devices (or all devices), should be handled by BIOS ("Firmware First
mode"), rather than be handled by the OS.
Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
that it may manage such errors, log them to the System Event Log, and
possibly take other actions. The aer driver should honor this, and
not attach itself to devices noted as such.
Furthermore, Kenji Kaneshige reminded us to disallow changing the AER
registers when respecting Firmware First mode. Platform firmware is
expected to manage these, and if changes to them are allowed, it could
break that firmware's behavior.
The HEST parsing code may be replaced in the future by a more
feature-rich implementation. This patch provides the minimum needed
to prevent breakage until that implementation is available.
Signed-off-by: Matt Domsch <[email protected]>
---
drivers/acpi/Makefile | 1 +
drivers/acpi/hest.c | 106 ++++++++++++++++++++++++++++++++++++
drivers/pci/pcie/aer/aerdrv_core.c | 26 ++++++++-
include/acpi/acpi_hest.h | 8 +++
include/linux/pci.h | 1 +
5 files changed, 140 insertions(+), 2 deletions(-)
create mode 100644 drivers/acpi/hest.c
create mode 100644 include/acpi/acpi_hest.h
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 7702118..9ab0d6d 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -19,6 +19,7 @@ obj-y += acpi.o \
# All the builtin files are in the "acpi." module_param namespace.
acpi-y += osl.o utils.o reboot.o
+obj-y += hest.o
# sleep related files
acpi-y += wakeup.o
diff --git a/drivers/acpi/hest.c b/drivers/acpi/hest.c
new file mode 100644
index 0000000..9684f50
--- /dev/null
+++ b/drivers/acpi/hest.c
@@ -0,0 +1,106 @@
+#include <linux/acpi.h>
+#include <linux/pci.h>
+
+static unsigned long parse_acpi_hest_ia_machine_check(struct acpi_hest_ia_machine_check *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
+}
+
+static unsigned long parse_acpi_hest_ia_corrected(struct acpi_hest_ia_corrected *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
+}
+
+static unsigned long parse_acpi_hest_ia_nmi(struct acpi_hest_ia_nmi *p)
+{
+ return sizeof(*p);
+}
+
+static unsigned long parse_acpi_hest_generic(struct acpi_hest_generic *p)
+{
+ return sizeof(*p);
+}
+
+static unsigned long parse_acpi_hest_aer(void *hdr, int type, struct pci_dev *pci, int *firmware_first)
+{
+ struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header);
+ unsigned long rc=0;
+ switch (type) {
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ rc = sizeof(struct acpi_hest_aer_root);
+ break;
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ rc = sizeof(struct acpi_hest_aer);
+ break;
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ rc = sizeof(struct acpi_hest_aer_bridge);
+ break;
+ }
+
+ if (p->flags & ACPI_HEST_FIRMWARE_FIRST &&
+ (p->flags & ACPI_HEST_GLOBAL ||
+ (p->bus == pci->bus->number &&
+ p->device == PCI_SLOT(pci->devfn) &&
+ p->function == PCI_FUNC(pci->devfn))))
+ *firmware_first = 1;
+ return rc;
+}
+
+static int acpi_hest_firmware_first(struct acpi_table_header *stdheader, struct pci_dev *pci)
+{
+ struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader;
+ void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */
+ struct acpi_hest_header *hdr = p;
+
+ int i;
+ int firmware_first = 0;
+
+ for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) {
+ switch (hdr->type) {
+ case ACPI_HEST_TYPE_IA32_CHECK:
+ p += parse_acpi_hest_ia_machine_check(p);
+ break;
+ case ACPI_HEST_TYPE_IA32_CORRECTED_CHECK:
+ p += parse_acpi_hest_ia_corrected(p);
+ break;
+ case ACPI_HEST_TYPE_IA32_NMI:
+ p += parse_acpi_hest_ia_nmi(p);
+ break;
+ /* These three should never appear */
+ case ACPI_HEST_TYPE_NOT_USED3:
+ case ACPI_HEST_TYPE_NOT_USED4:
+ case ACPI_HEST_TYPE_NOT_USED5:
+ break;
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ p += parse_acpi_hest_aer(p, hdr->type, pci, &firmware_first);
+ break;
+ case ACPI_HEST_TYPE_GENERIC_ERROR:
+ p += parse_acpi_hest_generic(p);
+ break;
+ /* These should never appear either */
+ case ACPI_HEST_TYPE_RESERVED:
+ default:
+ break;
+ }
+ }
+ return firmware_first;
+}
+
+int acpi_hest_firmware_first_pci(struct pci_dev *pci)
+{
+ acpi_status status = AE_NOT_FOUND;
+ struct acpi_table_header *hest = NULL;
+ status = acpi_get_table(ACPI_SIG_HEST, 1, &hest);
+
+ if (ACPI_SUCCESS(status)) {
+ if (acpi_hest_firmware_first(hest, pci)) {
+ return 1;
+ }
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 9f5ccbe..aef2db2 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -23,6 +23,7 @@
#include <linux/pm.h>
#include <linux/suspend.h>
#include <linux/delay.h>
+#include <acpi/acpi_hest.h>
#include "aerdrv.h"
static int forceload;
@@ -35,6 +36,9 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
u16 reg16 = 0;
int pos;
+ if (dev->aer_firmware_first)
+ return -EIO;
+
pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
if (!pos)
return -EIO;
@@ -60,6 +64,9 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev)
u16 reg16 = 0;
int pos;
+ if (dev->aer_firmware_first)
+ return -EIO;
+
pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
if (!pos)
return -EIO;
@@ -874,8 +881,23 @@ void aer_delete_rootport(struct aer_rpc *rpc)
*/
int aer_init(struct pcie_device *dev)
{
- if (aer_osc_setup(dev) && !forceload)
- return -ENXIO;
+ if (acpi_hest_firmware_first_pci(dev->port)) {
+ dev_printk(KERN_DEBUG, &dev->device,
+ "PCIe device errors handled by platform firmware.\n");
+ dev->port->aer_firmware_first=1;
+ goto out;
+ }
+
+ if (aer_osc_setup(dev))
+ goto out;
return 0;
+out:
+ if (forceload) {
+ dev_printk(KERN_DEBUG, &dev->device,
+ "aerdrv forceload requested.\n");
+ dev->port->aer_firmware_first=0;
+ return 0;
+ }
+ return -ENXIO;
}
diff --git a/include/acpi/acpi_hest.h b/include/acpi/acpi_hest.h
new file mode 100644
index 0000000..0d41408
--- /dev/null
+++ b/include/acpi/acpi_hest.h
@@ -0,0 +1,8 @@
+#ifndef __ACPI_HEST_H
+#define __ACPI_HEST_H
+
+#include <linux/pci.h>
+
+extern int acpi_hest_firmware_first_pci(struct pci_dev *pci);
+
+#endif
diff --git a/include/linux/pci.h b/include/linux/pci.h
index da4128f..9d646e6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -280,6 +280,7 @@ struct pci_dev {
unsigned int is_virtfn:1;
unsigned int reset_fn:1;
unsigned int is_hotplug_bridge:1;
+ unsigned int aer_firmware_first:1;
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
--
1.6.2.5
On Thu, 29 Oct 2009 09:15:59 -0500
Matt Domsch <[email protected]> wrote:
> On Fri, Oct 09, 2009 at 01:33:20PM -0500, Matt Domsch wrote:
> > For review and comment. Feedback from Hidetoshi Seto and Kenji
> > Kaneshige incorporated.
>
> With no more comments received, I would like to see this applied.
Kenji-san and Hidetoshi-san, are you ok with me adding your Reviewed-by
tags to this patch?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
Hi Matt,
Sorry to late response.
Matt Domsch wrote:
> For review and comment. Feedback from Hidetoshi Seto and Kenji
> Kaneshige incorporated.
>
> Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
> to every PCIe root port for which BIOS reports it should, via ACPI
> _OSC.
>
> However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
> 4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
> for OS and BIOS to handshake over which errors for which components
> each will handle. One table in ACPI 4.0 is the Hardware Error Source
> Table (HEST), where BIOS can define that errors for certain PCIe
> devices (or all devices), should be handled by BIOS ("Firmware First
> mode"), rather than be handled by the OS.
>
> Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
> that it may manage such errors, log them to the System Event Log, and
> possibly take other actions. The aer driver should honor this, and
> not attach itself to devices noted as such.
>
> Furthermore, Kenji Kaneshige reminded us to disallow changing the AER
> registers when respecting Firmware First mode. Platform firmware is
> expected to manage these, and if changes to them are allowed, it could
> break that firmware's behavior.
>
> The HEST parsing code may be replaced in the future by a more
> feature-rich implementation. This patch provides the minimum needed
> to prevent breakage until that implementation is available.
>
> Signed-off-by: Matt Domsch <[email protected]>
> ---
> drivers/acpi/Makefile | 1 +
> drivers/acpi/hest.c | 106 ++++++++++++++++++++++++++++++++++++
> drivers/pci/pcie/aer/aerdrv_core.c | 26 ++++++++-
> include/acpi/acpi_hest.h | 8 +++
> include/linux/pci.h | 1 +
> 5 files changed, 140 insertions(+), 2 deletions(-)
> create mode 100644 drivers/acpi/hest.c
> create mode 100644 include/acpi/acpi_hest.h
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 7702118..9ab0d6d 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -19,6 +19,7 @@ obj-y += acpi.o \
>
> # All the builtin files are in the "acpi." module_param namespace.
> acpi-y += osl.o utils.o reboot.o
> +obj-y += hest.o
>
> # sleep related files
> acpi-y += wakeup.o
> diff --git a/drivers/acpi/hest.c b/drivers/acpi/hest.c
> new file mode 100644
> index 0000000..9684f50
> --- /dev/null
> +++ b/drivers/acpi/hest.c
> @@ -0,0 +1,106 @@
> +#include <linux/acpi.h>
> +#include <linux/pci.h>
> +
> +static unsigned long parse_acpi_hest_ia_machine_check(struct acpi_hest_ia_machine_check *p)
> +{
> + return sizeof(*p) +
> + (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
> +}
> +
> +static unsigned long parse_acpi_hest_ia_corrected(struct acpi_hest_ia_corrected *p)
> +{
> + return sizeof(*p) +
> + (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
> +}
> +
> +static unsigned long parse_acpi_hest_ia_nmi(struct acpi_hest_ia_nmi *p)
> +{
> + return sizeof(*p);
> +}
> +
> +static unsigned long parse_acpi_hest_generic(struct acpi_hest_generic *p)
> +{
> + return sizeof(*p);
> +}
> +
> +static unsigned long parse_acpi_hest_aer(void *hdr, int type, struct pci_dev *pci, int *firmware_first)
> +{
> + struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header);
> + unsigned long rc=0;
> + switch (type) {
> + case ACPI_HEST_TYPE_AER_ROOT_PORT:
> + rc = sizeof(struct acpi_hest_aer_root);
> + break;
> + case ACPI_HEST_TYPE_AER_ENDPOINT:
> + rc = sizeof(struct acpi_hest_aer);
> + break;
> + case ACPI_HEST_TYPE_AER_BRIDGE:
> + rc = sizeof(struct acpi_hest_aer_bridge);
> + break;
> + }
> +
> + if (p->flags & ACPI_HEST_FIRMWARE_FIRST &&
> + (p->flags & ACPI_HEST_GLOBAL ||
> + (p->bus == pci->bus->number &&
> + p->device == PCI_SLOT(pci->devfn) &&
> + p->function == PCI_FUNC(pci->devfn))))
> + *firmware_first = 1;
> + return rc;
> +}
> +
> +static int acpi_hest_firmware_first(struct acpi_table_header *stdheader, struct pci_dev *pci)
> +{
> + struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader;
> + void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */
> + struct acpi_hest_header *hdr = p;
> +
> + int i;
> + int firmware_first = 0;
> +
> + for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) {
> + switch (hdr->type) {
> + case ACPI_HEST_TYPE_IA32_CHECK:
> + p += parse_acpi_hest_ia_machine_check(p);
> + break;
> + case ACPI_HEST_TYPE_IA32_CORRECTED_CHECK:
> + p += parse_acpi_hest_ia_corrected(p);
> + break;
> + case ACPI_HEST_TYPE_IA32_NMI:
> + p += parse_acpi_hest_ia_nmi(p);
> + break;
> + /* These three should never appear */
> + case ACPI_HEST_TYPE_NOT_USED3:
> + case ACPI_HEST_TYPE_NOT_USED4:
> + case ACPI_HEST_TYPE_NOT_USED5:
> + break;
Yes, these should never. But if there, what will happen?
I'd like to see a error message rather than hang in infinite loops.
> + case ACPI_HEST_TYPE_AER_ROOT_PORT:
> + case ACPI_HEST_TYPE_AER_ENDPOINT:
> + case ACPI_HEST_TYPE_AER_BRIDGE:
> + p += parse_acpi_hest_aer(p, hdr->type, pci, &firmware_first);
> + break;
> + case ACPI_HEST_TYPE_GENERIC_ERROR:
> + p += parse_acpi_hest_generic(p);
> + break;
> + /* These should never appear either */
> + case ACPI_HEST_TYPE_RESERVED:
> + default:
> + break;
Ditto.
> + }
> + }
> + return firmware_first;
> +}
> +
> +int acpi_hest_firmware_first_pci(struct pci_dev *pci)
> +{
It is OK, but if args of this function were (bus_n, dev_n, fn_n)
then "#include <linux/pci.h>" will not be required.
One another concern I got here is if there was a seg_n, segment
number, how we can handle it... but it will be one of future works,
so this would be OK at this time.
> + acpi_status status = AE_NOT_FOUND;
> + struct acpi_table_header *hest = NULL;
> + status = acpi_get_table(ACPI_SIG_HEST, 1, &hest);
> +
> + if (ACPI_SUCCESS(status)) {
> + if (acpi_hest_firmware_first(hest, pci)) {
> + return 1;
> + }
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci);
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 9f5ccbe..aef2db2 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -23,6 +23,7 @@
> #include <linux/pm.h>
> #include <linux/suspend.h>
> #include <linux/delay.h>
> +#include <acpi/acpi_hest.h>
> #include "aerdrv.h"
>
> static int forceload;
> @@ -35,6 +36,9 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> u16 reg16 = 0;
> int pos;
>
> + if (dev->aer_firmware_first)
> + return -EIO;
> +
The aer_init() will be called for root ports (via aer_probe() of
aer service driver), but not for end point devices or so on.
So you need to call aer_init() for end points from here or somewhere
else, to set aer_firmware_first correctly.
> pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> if (!pos)
> return -EIO;
> @@ -60,6 +64,9 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev)
> u16 reg16 = 0;
> int pos;
>
> + if (dev->aer_firmware_first)
> + return -EIO;
> +
> pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
> if (!pos)
> return -EIO;
> @@ -874,8 +881,23 @@ void aer_delete_rootport(struct aer_rpc *rpc)
> */
> int aer_init(struct pcie_device *dev)
> {
> - if (aer_osc_setup(dev) && !forceload)
> - return -ENXIO;
> + if (acpi_hest_firmware_first_pci(dev->port)) {
> + dev_printk(KERN_DEBUG, &dev->device,
> + "PCIe device errors handled by platform firmware.\n");
> + dev->port->aer_firmware_first=1;
Coding style requests you to add spaces before and after of "=".
> + goto out;
> + }
> +
> + if (aer_osc_setup(dev))
> + goto out;
>
> return 0;
> +out:
> + if (forceload) {
> + dev_printk(KERN_DEBUG, &dev->device,
> + "aerdrv forceload requested.\n");
> + dev->port->aer_firmware_first=0;
Ditto.
Rests are seems good.
Thanks.
H.Seto
> + return 0;
> + }
> + return -ENXIO;
> }
> diff --git a/include/acpi/acpi_hest.h b/include/acpi/acpi_hest.h
> new file mode 100644
> index 0000000..0d41408
> --- /dev/null
> +++ b/include/acpi/acpi_hest.h
> @@ -0,0 +1,8 @@
> +#ifndef __ACPI_HEST_H
> +#define __ACPI_HEST_H
> +
> +#include <linux/pci.h>
> +
> +extern int acpi_hest_firmware_first_pci(struct pci_dev *pci);
> +
> +#endif
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index da4128f..9d646e6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -280,6 +280,7 @@ struct pci_dev {
> unsigned int is_virtfn:1;
> unsigned int reset_fn:1;
> unsigned int is_hotplug_bridge:1;
> + unsigned int aer_firmware_first:1;
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
On Fri, Oct 30, 2009 at 11:16:34AM +0900, Hidetoshi Seto wrote:
> Hi Matt,
>
> Sorry to late response.
Thank you for your review.
> Matt Domsch wrote:
> > + /* These three should never appear */
> > + case ACPI_HEST_TYPE_NOT_USED3:
> > + case ACPI_HEST_TYPE_NOT_USED4:
> > + case ACPI_HEST_TYPE_NOT_USED5:
> > + break;
>
> Yes, these should never. But if there, what will happen?
> I'd like to see a error message rather than hang in infinite loops.
>
> > + /* These should never appear either */
> > + case ACPI_HEST_TYPE_RESERVED:
> > + default:
> > + break;
>
> Ditto.
It won't infinite loop, due to i incrementing. If one of these types
appears early in the error_source list, it would prevent us from
seeing the (correct) source types later in the list.
But we can't advance the pointer because we can't know by how much to
advance it. We could print a debug message. How about:
printk(KERN_DEBUG PREFIX
"HEST Error Source list contains an obsolete type.\n");
At some point, the RESERVED type will move to higher number as new
sources are defined in the spec, so that would be different message.
How about:
printk(KERN_DEBUG PREFIX
"HEST Error Source list contains a reserved type.\n");
and I'll have it print only once.
> > +int acpi_hest_firmware_first_pci(struct pci_dev *pci)
> > +{
>
> It is OK, but if args of this function were (bus_n, dev_n, fn_n)
> then "#include <linux/pci.h>" will not be required.
I prefer to pass the pci_dev rather than 3 or 4 parts thereof down
through the functions. Several other .c files in drivers/acpi do
likewise.
> One another concern I got here is if there was a seg_n, segment
> number, how we can handle it... but it will be one of future works,
> so this would be OK at this time.
Yes, but this ACPI table doesn't have a domain / segment number in
it. Does this test:
if (p->flags & ACPI_HEST_FIRMWARE_FIRST &&
(p->flags & ACPI_HEST_GLOBAL ||
(p->bus == pci->bus->number &&
p->device == PCI_SLOT(pci->devfn) &&
p->function == PCI_FUNC(pci->devfn))))
*firmware_first = 1;
need to add an explicit test for && 0 == pci_domain_nr(pci->bus) ?
> > @@ -35,6 +36,9 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> > u16 reg16 = 0;
> > int pos;
> >
> > + if (dev->aer_firmware_first)
> > + return -EIO;
> > +
>
> The aer_init() will be called for root ports (via aer_probe() of
> aer service driver), but not for end point devices or so on.
> So you need to call aer_init() for end points from here or somewhere
> else, to set aer_firmware_first correctly.
Good catch. I'll move the setting of dev->aer_firmware_first into the
PCI device discovery path. By that point, ACPI is configured and
available.
> > + if (acpi_hest_firmware_first_pci(dev->port)) {
> > + dev_printk(KERN_DEBUG, &dev->device,
> > + "PCIe device errors handled by platform firmware.\n");
> > + dev->port->aer_firmware_first=1;
>
> Coding style requests you to add spaces before and after of "=".
OK. This and the next will move as noted above.
--
Matt Domsch
Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
Matt Domsch wrote:
>> Matt Domsch wrote:
>>> + /* These three should never appear */
>>> + case ACPI_HEST_TYPE_NOT_USED3:
>>> + case ACPI_HEST_TYPE_NOT_USED4:
>>> + case ACPI_HEST_TYPE_NOT_USED5:
>>> + break;
>> Yes, these should never. But if there, what will happen?
>> I'd like to see a error message rather than hang in infinite loops.
>>
>>> + /* These should never appear either */
>>> + case ACPI_HEST_TYPE_RESERVED:
>>> + default:
>>> + break;
>> Ditto.
>
> It won't infinite loop, due to i incrementing. If one of these types
> appears early in the error_source list, it would prevent us from
> seeing the (correct) source types later in the list.
Ah, you are right. Thanks for correcting me.
> But we can't advance the pointer because we can't know by how much to
> advance it. We could print a debug message. How about:
>
> printk(KERN_DEBUG PREFIX
> "HEST Error Source list contains an obsolete type.\n");
Good.
And it would be informative to print the type number.
... "HEST Error Source list contains an obsolete type (%d).\n", hdr->type);
> At some point, the RESERVED type will move to higher number as new
> sources are defined in the spec, so that would be different message.
> How about:
>
> printk(KERN_DEBUG PREFIX
> "HEST Error Source list contains a reserved type.\n");
Of course good.
And print the type number too, please.
> and I'll have it print only once.
Nice!
>> One another concern I got here is if there was a seg_n, segment
>> number, how we can handle it... but it will be one of future works,
>> so this would be OK at this time.
>
> Yes, but this ACPI table doesn't have a domain / segment number in
> it. Does this test:
>
> if (p->flags & ACPI_HEST_FIRMWARE_FIRST &&
> (p->flags & ACPI_HEST_GLOBAL ||
> (p->bus == pci->bus->number &&
> p->device == PCI_SLOT(pci->devfn) &&
> p->function == PCI_FUNC(pci->devfn))))
> *firmware_first = 1;
>
> need to add an explicit test for && 0 == pci_domain_nr(pci->bus) ?
Maybe it would be better to have, to find problem early if it happens.
>>> @@ -35,6 +36,9 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>>> u16 reg16 = 0;
>>> int pos;
>>>
>>> + if (dev->aer_firmware_first)
>>> + return -EIO;
>>> +
>> The aer_init() will be called for root ports (via aer_probe() of
>> aer service driver), but not for end point devices or so on.
>> So you need to call aer_init() for end points from here or somewhere
>> else, to set aer_firmware_first correctly.
>
> Good catch. I'll move the setting of dev->aer_firmware_first into the
> PCI device discovery path. By that point, ACPI is configured and
> available.
Please be careful that there could be hot-plugged devices later.
>>> + if (acpi_hest_firmware_first_pci(dev->port)) {
>>> + dev_printk(KERN_DEBUG, &dev->device,
>>> + "PCIe device errors handled by platform firmware.\n");
>>> + dev->port->aer_firmware_first=1;
>> Coding style requests you to add spaces before and after of "=".
>
> OK. This and the next will move as noted above.
Thanks,
H.Seto
>From 97ec121f91e5c76dc037f8d2abaea84c5476676e Mon Sep 17 00:00:00 2001
From: Matt Domsch <[email protected]>
Date: Thu, 8 Oct 2009 23:18:11 -0500
Subject: [PATCH] PCIe AER: honor ACPI HEST FIRMWARE FIRST mode
Feedback from Hidetoshi Seto and Kenji Kaneshige incorporated. This
correctly handles PCI-X bridges, PCIe root ports and endpoints, and
prints debug messages when invalid/reserved types are found in the
HEST. PCI devices not in domain/segment 0 are not represented in
HEST, thus will be ignored.
Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
to every PCIe root port for which BIOS reports it should, via ACPI
_OSC.
However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
for OS and BIOS to handshake over which errors for which components
each will handle. One table in ACPI 4.0 is the Hardware Error Source
Table (HEST), where BIOS can define that errors for certain PCIe
devices (or all devices), should be handled by BIOS ("Firmware First
mode"), rather than be handled by the OS.
Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
that it may manage such errors, log them to the System Event Log, and
possibly take other actions. The aer driver should honor this, and
not attach itself to devices noted as such.
Furthermore, Kenji Kaneshige reminded us to disallow changing the AER
registers when respecting Firmware First mode. Platform firmware is
expected to manage these, and if changes to them are allowed, it could
break that firmware's behavior.
The HEST parsing code may be replaced in the future by a more
feature-rich implementation. This patch provides the minimum needed
to prevent breakage until that implementation is available.
Signed-off-by: Matt Domsch <[email protected]>
---
drivers/acpi/Makefile | 1 +
drivers/acpi/hest.c | 135 ++++++++++++++++++++++++++++++++++++
drivers/pci/pcie/aer/aerdrv_core.c | 24 ++++++-
drivers/pci/probe.c | 8 ++
include/acpi/acpi_hest.h | 12 +++
include/linux/pci.h | 1 +
6 files changed, 179 insertions(+), 2 deletions(-)
create mode 100644 drivers/acpi/hest.c
create mode 100644 include/acpi/acpi_hest.h
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 7702118..c7b10b4 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -19,6 +19,7 @@ obj-y += acpi.o \
# All the builtin files are in the "acpi." module_param namespace.
acpi-y += osl.o utils.o reboot.o
+acpi-y += hest.o
# sleep related files
acpi-y += wakeup.o
diff --git a/drivers/acpi/hest.c b/drivers/acpi/hest.c
new file mode 100644
index 0000000..4bb18c9
--- /dev/null
+++ b/drivers/acpi/hest.c
@@ -0,0 +1,135 @@
+#include <linux/acpi.h>
+#include <linux/pci.h>
+
+#define PREFIX "ACPI: "
+
+static inline unsigned long parse_acpi_hest_ia_machine_check(struct acpi_hest_ia_machine_check *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
+}
+
+static inline unsigned long parse_acpi_hest_ia_corrected(struct acpi_hest_ia_corrected *p)
+{
+ return sizeof(*p) +
+ (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks);
+}
+
+static inline unsigned long parse_acpi_hest_ia_nmi(struct acpi_hest_ia_nmi *p)
+{
+ return sizeof(*p);
+}
+
+static inline unsigned long parse_acpi_hest_generic(struct acpi_hest_generic *p)
+{
+ return sizeof(*p);
+}
+
+static inline unsigned int hest_match_pci(struct acpi_hest_aer_common *p, struct pci_dev *pci)
+{
+ return (0 == pci_domain_nr(pci->bus) &&
+ p->bus == pci->bus->number &&
+ p->device == PCI_SLOT(pci->devfn) &&
+ p->function == PCI_FUNC(pci->devfn));
+}
+
+static unsigned long parse_acpi_hest_aer(void *hdr, int type, struct pci_dev *pci, int *firmware_first)
+{
+ struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header);
+ unsigned long rc=0;
+ u8 pcie_type = 0;
+ u8 bridge = 0;
+ switch (type) {
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ rc = sizeof(struct acpi_hest_aer_root);
+ pcie_type = PCI_EXP_TYPE_ROOT_PORT;
+ break;
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ rc = sizeof(struct acpi_hest_aer);
+ pcie_type = PCI_EXP_TYPE_ENDPOINT;
+ break;
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ rc = sizeof(struct acpi_hest_aer_bridge);
+ if ((pci->class >> 16) == PCI_BASE_CLASS_BRIDGE)
+ bridge = 1;
+ break;
+ }
+
+ if (p->flags & ACPI_HEST_GLOBAL) {
+ if ((pci->is_pcie && (pci->pcie_type == pcie_type)) || bridge)
+ *firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
+ }
+ else
+ if (hest_match_pci(p, pci))
+ *firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
+ return rc;
+}
+
+static int acpi_hest_firmware_first(struct acpi_table_header *stdheader, struct pci_dev *pci)
+{
+ struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader;
+ void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */
+ struct acpi_hest_header *hdr = p;
+
+ int i;
+ int firmware_first = 0;
+ static unsigned char printed_unused = 0;
+ static unsigned char printed_reserved = 0;
+
+ for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) {
+ switch (hdr->type) {
+ case ACPI_HEST_TYPE_IA32_CHECK:
+ p += parse_acpi_hest_ia_machine_check(p);
+ break;
+ case ACPI_HEST_TYPE_IA32_CORRECTED_CHECK:
+ p += parse_acpi_hest_ia_corrected(p);
+ break;
+ case ACPI_HEST_TYPE_IA32_NMI:
+ p += parse_acpi_hest_ia_nmi(p);
+ break;
+ /* These three should never appear */
+ case ACPI_HEST_TYPE_NOT_USED3:
+ case ACPI_HEST_TYPE_NOT_USED4:
+ case ACPI_HEST_TYPE_NOT_USED5:
+ if (!printed_unused) {
+ printk(KERN_DEBUG PREFIX
+ "HEST Error Source list contains an obsolete type (%d).\n", hdr->type);
+ printed_unused = 1;
+ }
+ break;
+ case ACPI_HEST_TYPE_AER_ROOT_PORT:
+ case ACPI_HEST_TYPE_AER_ENDPOINT:
+ case ACPI_HEST_TYPE_AER_BRIDGE:
+ p += parse_acpi_hest_aer(p, hdr->type, pci, &firmware_first);
+ break;
+ case ACPI_HEST_TYPE_GENERIC_ERROR:
+ p += parse_acpi_hest_generic(p);
+ break;
+ /* These should never appear either */
+ case ACPI_HEST_TYPE_RESERVED:
+ default:
+ if (!printed_reserved) {
+ printk(KERN_DEBUG PREFIX
+ "HEST Error Source list contains a reserved type (%d).\n", hdr->type);
+ printed_reserved = 1;
+ }
+ break;
+ }
+ }
+ return firmware_first;
+}
+
+int acpi_hest_firmware_first_pci(struct pci_dev *pci)
+{
+ acpi_status status = AE_NOT_FOUND;
+ struct acpi_table_header *hest = NULL;
+ status = acpi_get_table(ACPI_SIG_HEST, 1, &hest);
+
+ if (ACPI_SUCCESS(status)) {
+ if (acpi_hest_firmware_first(hest, pci)) {
+ return 1;
+ }
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 9f5ccbe..f4512fe 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -35,6 +35,9 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
u16 reg16 = 0;
int pos;
+ if (dev->aer_firmware_first)
+ return -EIO;
+
pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
if (!pos)
return -EIO;
@@ -60,6 +63,9 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev)
u16 reg16 = 0;
int pos;
+ if (dev->aer_firmware_first)
+ return -EIO;
+
pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
if (!pos)
return -EIO;
@@ -874,8 +880,22 @@ void aer_delete_rootport(struct aer_rpc *rpc)
*/
int aer_init(struct pcie_device *dev)
{
- if (aer_osc_setup(dev) && !forceload)
- return -ENXIO;
+ if (dev->port->aer_firmware_first) {
+ dev_printk(KERN_DEBUG, &dev->device,
+ "PCIe errors handled by platform firmware.\n");
+ goto out;
+ }
+
+ if (aer_osc_setup(dev))
+ goto out;
return 0;
+out:
+ if (forceload) {
+ dev_printk(KERN_DEBUG, &dev->device,
+ "aerdrv forceload requested.\n");
+ dev->port->aer_firmware_first = 0;
+ return 0;
+ }
+ return -ENXIO;
}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8105e32..102995d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/cpumask.h>
#include <linux/pci-aspm.h>
+#include <acpi/acpi_hest.h>
#include "pci.h"
#define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
@@ -714,6 +715,12 @@ static void set_pcie_hotplug_bridge(struct pci_dev *pdev)
pdev->is_hotplug_bridge = 1;
}
+static void set_pci_aer_firmware_first(struct pci_dev *pdev)
+{
+ if (acpi_hest_firmware_first_pci(pdev))
+ pdev->aer_firmware_first = 1;
+}
+
#define LEGACY_IO_RESOURCE (IORESOURCE_IO | IORESOURCE_PCI_FIXED)
/**
@@ -742,6 +749,7 @@ int pci_setup_device(struct pci_dev *dev)
dev->multifunction = !!(hdr_type & 0x80);
dev->error_state = pci_channel_io_normal;
set_pcie_port_type(dev);
+ set_pci_aer_firmware_first(dev);
list_for_each_entry(slot, &dev->bus->slots, list)
if (PCI_SLOT(dev->devfn) == slot->number)
diff --git a/include/acpi/acpi_hest.h b/include/acpi/acpi_hest.h
new file mode 100644
index 0000000..63194d0
--- /dev/null
+++ b/include/acpi/acpi_hest.h
@@ -0,0 +1,12 @@
+#ifndef __ACPI_HEST_H
+#define __ACPI_HEST_H
+
+#include <linux/pci.h>
+
+#ifdef CONFIG_ACPI
+extern int acpi_hest_firmware_first_pci(struct pci_dev *pci);
+#else
+static inline int acpi_hest_firmware_first_pci(struct pci_dev *pci) { return 0; }
+#endif
+
+#endif
diff --git a/include/linux/pci.h b/include/linux/pci.h
index da4128f..9d646e6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -280,6 +280,7 @@ struct pci_dev {
unsigned int is_virtfn:1;
unsigned int reset_fn:1;
unsigned int is_hotplug_bridge:1;
+ unsigned int aer_firmware_first:1;
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
--
1.6.2.5
On Mon, 2 Nov 2009 11:51:24 -0600
Matt Domsch <[email protected]> wrote:
> From 97ec121f91e5c76dc037f8d2abaea84c5476676e Mon Sep 17 00:00:00 2001
> From: Matt Domsch <[email protected]>
> Date: Thu, 8 Oct 2009 23:18:11 -0500
> Subject: [PATCH] PCIe AER: honor ACPI HEST FIRMWARE FIRST mode
>
> Feedback from Hidetoshi Seto and Kenji Kaneshige incorporated. This
> correctly handles PCI-X bridges, PCIe root ports and endpoints, and
> prints debug messages when invalid/reserved types are found in the
> HEST. PCI devices not in domain/segment 0 are not represented in
> HEST, thus will be ignored.
>
> Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
> to every PCIe root port for which BIOS reports it should, via ACPI
> _OSC.
>
> However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
> 4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
> for OS and BIOS to handshake over which errors for which components
> each will handle. One table in ACPI 4.0 is the Hardware Error Source
> Table (HEST), where BIOS can define that errors for certain PCIe
> devices (or all devices), should be handled by BIOS ("Firmware First
> mode"), rather than be handled by the OS.
>
> Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
> that it may manage such errors, log them to the System Event Log, and
> possibly take other actions. The aer driver should honor this, and
> not attach itself to devices noted as such.
>
> Furthermore, Kenji Kaneshige reminded us to disallow changing the AER
> registers when respecting Firmware First mode. Platform firmware is
> expected to manage these, and if changes to them are allowed, it could
> break that firmware's behavior.
>
> The HEST parsing code may be replaced in the future by a more
> feature-rich implementation. This patch provides the minimum needed
> to prevent breakage until that implementation is available.
>
> Signed-off-by: Matt Domsch <[email protected]>
> ---
Applied to my linux-next branch, thanks.
Jesse
On Wed, Nov 4, 2009 at 9:11 AM, Jesse Barnes <[email protected]> wrote:
> On Mon, 2 Nov 2009 11:51:24 -0600
> Matt Domsch <[email protected]> wrote:
>
>> From 97ec121f91e5c76dc037f8d2abaea84c5476676e Mon Sep 17 00:00:00 2001
>> From: Matt Domsch <[email protected]>
>> Date: Thu, 8 Oct 2009 23:18:11 -0500
>> Subject: [PATCH] PCIe AER: honor ACPI HEST FIRMWARE FIRST mode
>>
>> Feedback from Hidetoshi Seto and Kenji Kaneshige incorporated. ?This
>> correctly handles PCI-X bridges, PCIe root ports and endpoints, and
>> prints debug messages when invalid/reserved types are found in the
>> HEST. ?PCI devices not in domain/segment 0 are not represented in
>> HEST, thus will be ignored.
>>
>> Today, the PCIe Advanced Error Reporting (AER) driver attaches itself
>> to every PCIe root port for which BIOS reports it should, via ACPI
>> _OSC.
>>
>> However, _OSC alone is insufficient for newer BIOSes. ?Part of ACPI
>> 4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
>> for OS and BIOS to handshake over which errors for which components
>> each will handle. ?One table in ACPI 4.0 is the Hardware Error Source
>> Table (HEST), where BIOS can define that errors for certain PCIe
>> devices (or all devices), should be handled by BIOS ("Firmware First
>> mode"), rather than be handled by the OS.
>>
>> Dell PowerEdge 11G server BIOS defines Firmware First mode in HEST, so
>> that it may manage such errors, log them to the System Event Log, and
>> possibly take other actions. ?The aer driver should honor this, and
>> not attach itself to devices noted as such.
>>
>> Furthermore, Kenji Kaneshige reminded us to disallow changing the AER
>> registers when respecting Firmware First mode. ?Platform firmware is
>> expected to manage these, and if changes to them are allowed, it could
>> break that firmware's behavior.
>>
>> The HEST parsing code may be replaced in the future by a more
>> feature-rich implementation. ?This patch provides the minimum needed
>> to prevent breakage until that implementation is available.
>>
>> Signed-off-by: Matt Domsch <[email protected]>
>> ---
>
> Applied to my linux-next branch, thanks.
>
can not find acpi_hest.c and acpi_hest.h
maybe need to wait acpi hest code is there?
YH
On Wed, 4 Nov 2009 12:55:50 -0800
Yinghai Lu <[email protected]> wrote:
> On Wed, Nov 4, 2009 at 9:11 AM, Jesse Barnes
> <[email protected]> wrote:
> > On Mon, 2 Nov 2009 11:51:24 -0600
> > Matt Domsch <[email protected]> wrote:
> >
> >> From 97ec121f91e5c76dc037f8d2abaea84c5476676e Mon Sep 17 00:00:00
> >> 2001 From: Matt Domsch <[email protected]>
> >> Date: Thu, 8 Oct 2009 23:18:11 -0500
> >> Subject: [PATCH] PCIe AER: honor ACPI HEST FIRMWARE FIRST mode
> >>
> >> Feedback from Hidetoshi Seto and Kenji Kaneshige incorporated.
> >> This correctly handles PCI-X bridges, PCIe root ports and
> >> endpoints, and prints debug messages when invalid/reserved types
> >> are found in the HEST. PCI devices not in domain/segment 0 are
> >> not represented in HEST, thus will be ignored.
> >>
> >> Today, the PCIe Advanced Error Reporting (AER) driver attaches
> >> itself to every PCIe root port for which BIOS reports it should,
> >> via ACPI _OSC.
> >>
> >> However, _OSC alone is insufficient for newer BIOSes. Part of ACPI
> >> 4.0 is the new APEI (ACPI Platform Error Interfaces) which is a way
> >> for OS and BIOS to handshake over which errors for which components
> >> each will handle. One table in ACPI 4.0 is the Hardware Error
> >> Source Table (HEST), where BIOS can define that errors for certain
> >> PCIe devices (or all devices), should be handled by BIOS
> >> ("Firmware First mode"), rather than be handled by the OS.
> >>
> >> Dell PowerEdge 11G server BIOS defines Firmware First mode in
> >> HEST, so that it may manage such errors, log them to the System
> >> Event Log, and possibly take other actions. The aer driver should
> >> honor this, and not attach itself to devices noted as such.
> >>
> >> Furthermore, Kenji Kaneshige reminded us to disallow changing the
> >> AER registers when respecting Firmware First mode. Platform
> >> firmware is expected to manage these, and if changes to them are
> >> allowed, it could break that firmware's behavior.
> >>
> >> The HEST parsing code may be replaced in the future by a more
> >> feature-rich implementation. This patch provides the minimum
> >> needed to prevent breakage until that implementation is available.
> >>
> >> Signed-off-by: Matt Domsch <[email protected]>
> >> ---
> >
> > Applied to my linux-next branch, thanks.
> >
>
> can not find acpi_hest.c and acpi_hest.h
>
> maybe need to wait acpi hest code is there?
Arg, when I fixed up a conflict in the patch I forgot to add those
files to the commit. They were in my tree so I didn't see a build
error... Fixing now.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 4 Nov 2009 13:05:15 -0800
Jesse Barnes <[email protected]> wrote:
> > can not find acpi_hest.c and acpi_hest.h
> >
> > maybe need to wait acpi hest code is there?
>
> Arg, when I fixed up a conflict in the patch I forgot to add those
> files to the commit. They were in my tree so I didn't see a build
> error... Fixing now.
Should be all fixed now, thanks for catching it Yinghai.
--
Jesse Barnes, Intel Open Source Technology Center