Patchworkβ PCIe AER: honor ACPI HEST FIRMWARE FIRST mode

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 <Matt_Domsch@dell.com>
---
 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

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 <Matt_Domsch@dell.com>
> ---
>  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 */
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html