[PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[Jon Derrick]

Add a bit in pci_host_bridge to indicate to leave the System Error
Interrupts as configured by the pre-boot environment. Propagate this to
the AER driver which disables System Error Interrupts.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pcie/aer.c | 7 +++++--
 include/linux/pci.h    | 3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 83180ed..6a4af63 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1360,6 +1360,7 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
 static void aer_enable_rootport(struct aer_rpc *rpc)
 {
 	struct pci_dev *pdev = rpc->rpd;
+	struct pci_host_bridge *host;
 	int aer_pos;
 	u16 reg16;
 	u32 reg32;
@@ -1369,8 +1370,10 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
 	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, reg16);
 
 	/* Disable system error generation in response to error messages */
-	pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
-				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
+	host = pci_find_host_bridge(pdev->bus);
+	if (!host->no_disable_sys_err)
+		pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
+					   SYSTEM_ERROR_INTR_ON_MESG_MASK);
 
 	aer_pos = pdev->aer_cap;
 	/* Clear error status */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6925828..6fcfab4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -484,6 +484,9 @@ struct pci_host_bridge {
 	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
+	unsigned int	no_disable_sys_err:1;	/* Don't disable system
+						   error interrupts */
+
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,
-- 
1.8.3.1

[PATCH 2/3] PCI: Export pci_find_host_bridge()

[Jon Derrick]

This symbol is generally useful to navigate up to the host bridge and
may be used in modules.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/host-bridge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index e01d53f..afa50b4 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -23,6 +23,7 @@ struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus)
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
+EXPORT_SYMBOL_GPL(pci_find_host_bridge);
 
 struct device *pci_get_host_bridge_device(struct pci_dev *dev)
 {
-- 
1.8.3.1

[PATCH 3/3] PCI/VMD: Enable firmware-first combined model

[Jon Derrick]

The VMD endpoint acts as a host bridge for a nested PCIe segment. This
segment and its ports and endpoints are not represented in ACPI tables
such as DSDT and HEST, so "firmware first" error handling is currently
unsupported in the driver in favor of OS native error handling.

VMD does support firmware error handling where errors are signaled to
host firmware via SMI. VMD supports a "combined model" [1] where
OS-native error handling is used in addition to firmware. Because of
this combined model, this patch does not subscribe to the kernel
firmware-first architecture which would prevent OS native error
handling.

The VMD segment does not support NMI callbacks from firmware to the OS.
After error handling, firmware instead causes VMD to issue an interrupt
via MSI vector 0, which is the same interrupt already used to signal
PCIe errors for OS native handling in the VMD PCIe segment.

In order to generate SMI on PCIe errors, BIOS is responsible for setting
the System Error bits pre-boot in the Root Control register for the root
ports in the VMD segment.

BIOS signals its intent to use firmware first in the VMD segment by
setting the Programming Interface to 0x1 in VMD's Class Code register.
This patch checks for this value, and if set, prevents the native AER
driver from clearing the System Error bits for root ports in the VMD
segment.

[1]
https://firmware.intel.com/sites/default/files/resources/A_Tour_beyond_BIOS_Implementing_APEI_with_UEFI_White_Paper.pdf
p. 10.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 46ed80f..7269081 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -589,6 +589,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	LIST_HEAD(resources);
 	resource_size_t offset[2] = {0};
 	resource_size_t membar2_offset = 0x2000, busn_start = 0;
+	u8 interface;
 
 	/*
 	 * Shadow registers may exist in certain VMD device ids which allow
@@ -716,8 +717,40 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	vmd_attach_resources(vmd);
 	vmd_setup_dma_ops(vmd);
 	dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
-	pci_rescan_bus(vmd->bus);
 
+	/*
+	 * Certain VMD devices may request firmware-first error handling
+	 * support on the VMD domain. These domains are not described by ACPI
+	 * tables such as DSDT and HEST, so "firmware first" error handling is
+	 * currently unsupported in the driver in favor of OS native error
+	 * handing.
+	 *
+	 * VMD does support firmware error handling where errors are signaled
+	 * to firmware via SMI. It also supports a "combined model" where the
+	 * OS native error handling may be used in addition to firmware error
+	 * handling.
+	 *
+	 * Because of the lack of ACPI support on the domain and the capability
+	 * to use the 'combined model', the typical firmware-first architecture
+	 * is not used because it would disable OS native error handling.
+	 *
+	 * VMD domains do not support NMI callbacks to the OS. Pass-back to the
+	 * kernel from firmware is handled with a synthesized MSI on VMD device
+	 * vector 0, which is the same interrupt already used to signal PCIe
+	 * errors for OS native handling in the VMD domain and will trigger any
+	 * remaining required OS native error handling.
+	 *
+	 * Because the VMD domain is not described by ACPI, the intent to use
+	 * firmware-first error handling in the root ports is instead described
+	 * by the VMD device's programming interface bit being set to 0x1.
+	 */
+	pci_read_config_byte(vmd->dev, PCI_CLASS_PROG, &interface);
+	if (interface == 0x1) {
+		struct pci_host_bridge *host = pci_find_host_bridge(vmd->bus);
+		host->no_disable_sys_err = 1;
+	}
+
+	pci_rescan_bus(vmd->bus);
 	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
 			       "domain"), "Can't create symlink to domain\n");
 	return 0;
-- 
1.8.3.1

Re: [PATCH 3/3] PCI/VMD: Enable firmware-first combined model

[Christoph Hellwig]

On Fri, Oct 26, 2018 at 02:19:06PM -0600, Jon Derrick wrote:
> The VMD endpoint acts as a host bridge for a nested PCIe segment. This
> segment and its ports and endpoints are not represented in ACPI tables
> such as DSDT and HEST, so "firmware first" error handling is currently
> unsupported in the driver in favor of OS native error handling.
> 
> VMD does support firmware error handling where errors are signaled to
> host firmware via SMI. VMD supports a "combined model" [1] where
> OS-native error handling is used in addition to firmware. Because of
> this combined model, this patch does not subscribe to the kernel
> firmware-first architecture which would prevent OS native error
> handling.

This sounds really horrible.  Why can't Intel just offer a tweak to
disable this whole VMD nightmare and treat it like a normal PCIe bus,
which would leave everyone much better off?

Re: [PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[Bjorn Helgaas]

[+cc Rafael, Len, Tony, Borislav, Tyler, Christoph, linux-acpi, LKML]

On Fri, Oct 26, 2018 at 02:19:04PM -0600, Jon Derrick wrote:
> Add a bit in pci_host_bridge to indicate to leave the System Error
> Interrupts as configured by the pre-boot environment. Propagate this to
> the AER driver which disables System Error Interrupts.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pcie/aer.c | 7 +++++--
>  include/linux/pci.h    | 3 +++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 83180ed..6a4af63 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1360,6 +1360,7 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
>  static void aer_enable_rootport(struct aer_rpc *rpc)
>  {
>  	struct pci_dev *pdev = rpc->rpd;
> +	struct pci_host_bridge *host;
>  	int aer_pos;
>  	u16 reg16;
>  	u32 reg32;
> @@ -1369,8 +1370,10 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>  	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, reg16);
>  
>  	/* Disable system error generation in response to error messages */
> -	pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
> -				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
> +	host = pci_find_host_bridge(pdev->bus);
> +	if (!host->no_disable_sys_err)
> +		pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
> +					   SYSTEM_ERROR_INTR_ON_MESG_MASK);

If I squint hard enough this sort of makes sense, but it also makes me
confused about the normal APEI firmware-first model works.

In the NON-firmare-first case, firmware isn't involved in handling AER
errors.  The Linux AER driver fields an interrupt from a Root Port,
reads AER log registers, etc.

In the normal APEI firmware-first case, when the hardware reports an
AER event, I think firmware gets control first, and *it* reads the AER
log registers, packages them up, and generates an interrupt to the OS,
which reads the packaged error state from the firmware via the HEST.

If I understand this special Intel VMD firmware-first case correctly,
firmware gets control first, reads the AER log registers, and
synthesizes what looks to the OS like a normal AER interrupt.  The
Linux AER driver gets what it thinks is an interrupt from a Root Port,
and it reads AER log registers from the hardware just like it does in
the NON-firmware-first case.

My confusion is about how we manage the mechanism by which the
firmware gets control first.  In the Intel VMD case, it looks like
firmware fields the System Errors controlled by the Root Control
register of Root Ports.  This patch adds some framework so we know not
to touch something set up by firmware.

But in the normal APEI firmware-first case, we disable those System
Errors in the Root Control registers, so firmware must get control
some other way.

How does the OS know what mechanism the firmware uses, so it can make
sure to preserve it?  This patch might be part of the solution, but it
seems pretty ad hoc, and of course it does nothing for the APEI
firmware-first case.  How does firmware get control in that case?

>  	aer_pos = pdev->aer_cap;
>  	/* Clear error status */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6925828..6fcfab4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -484,6 +484,9 @@ struct pci_host_bridge {
>  	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
> +	unsigned int	no_disable_sys_err:1;	/* Don't disable system
> +						   error interrupts */
> +
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
>  			const struct resource *res,
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[Borislav Petkov]

On Mon, Oct 29, 2018 at 04:06:51PM -0500, Bjorn Helgaas wrote:
> [+cc Rafael, Len, Tony, Borislav, Tyler, Christoph, linux-acpi, LKML]
> 
> On Fri, Oct 26, 2018 at 02:19:04PM -0600, Jon Derrick wrote:
> > Add a bit in pci_host_bridge to indicate to leave the System Error
> > Interrupts as configured by the pre-boot environment. Propagate this to
> > the AER driver which disables System Error Interrupts.

This commit message should not explain what the patch does - that's
obvious - but why it is doing it.


> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  drivers/pci/pcie/aer.c | 7 +++++--
> >  include/linux/pci.h    | 3 +++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index 83180ed..6a4af63 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -1360,6 +1360,7 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
> >  static void aer_enable_rootport(struct aer_rpc *rpc)
> >  {
> >  	struct pci_dev *pdev = rpc->rpd;
> > +	struct pci_host_bridge *host;
> >  	int aer_pos;
> >  	u16 reg16;
> >  	u32 reg32;
> > @@ -1369,8 +1370,10 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
> >  	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, reg16);
> >  
> >  	/* Disable system error generation in response to error messages */
> > -	pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
> > -				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
> > +	host = pci_find_host_bridge(pdev->bus);
> > +	if (!host->no_disable_sys_err)

Double negation

	if (! .. ->no..

could simply be

	if (host->disable_sys_err...

> > +		pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
> > +					   SYSTEM_ERROR_INTR_ON_MESG_MASK);
> 
> If I squint hard enough this sort of makes sense, but it also makes me
> confused about the normal APEI firmware-first model works.
> 
> In the NON-firmare-first case, firmware isn't involved in handling AER
> errors.  The Linux AER driver fields an interrupt from a Root Port,
> reads AER log registers, etc.
> 
> In the normal APEI firmware-first case, when the hardware reports an
> AER event, I think firmware gets control first, and *it* reads the AER
> log registers, packages them up, and generates an interrupt to the OS,
> which reads the packaged error state from the firmware via the HEST.
> 
> If I understand this special Intel VMD firmware-first case correctly,
> firmware gets control first, reads the AER log registers, and
> synthesizes what looks to the OS like a normal AER interrupt.  The

Why?

Why the faking?

If firmware needs to get control, why doesn't it then *retain* control
and report the error through HEST, like others do?

AFAIUC, fw wants to do something underneath. What's wrong with making it
a normal firmware-first case?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[Keith Busch]

On Fri, Nov 02, 2018 at 10:53:00AM +0100, Borislav Petkov wrote:
> On Mon, Oct 29, 2018 at 04:06:51PM -0500, Bjorn Helgaas wrote:
> > If I squint hard enough this sort of makes sense, but it also makes me
> > confused about the normal APEI firmware-first model works.
> > 
> > In the NON-firmare-first case, firmware isn't involved in handling AER
> > errors.  The Linux AER driver fields an interrupt from a Root Port,
> > reads AER log registers, etc.
> > 
> > In the normal APEI firmware-first case, when the hardware reports an
> > AER event, I think firmware gets control first, and *it* reads the AER
> > log registers, packages them up, and generates an interrupt to the OS,
> > which reads the packaged error state from the firmware via the HEST.
> > 
> > If I understand this special Intel VMD firmware-first case correctly,
> > firmware gets control first, reads the AER log registers, and
> > synthesizes what looks to the OS like a normal AER interrupt.  The
> 
> Why?
> 
> Why the faking?
> 
> If firmware needs to get control, why doesn't it then *retain* control
> and report the error through HEST, like others do?
> 
> AFAIUC, fw wants to do something underneath. What's wrong with making it
> a normal firmware-first case?

VMD acts a bit like a host-bus adapter. The firmware knows about the
adapter, but not about anything on the bus that it attaches to.

This "hybrid" approach is basically saying that the firmware knows about
the HBA, and it wants a chance to be notified of errors on the bus it
attaches to, but the firmware can't do anything about such errors.

The bus in this case is PCIe, where we have capable error handling in the
kernel driver, so we ultimately want the AER driver handling the errors.

Re: [PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[Borislav Petkov]

On Fri, Nov 02, 2018 at 10:17:30AM -0600, Keith Busch wrote:
> VMD acts a bit like a host-bus adapter. The firmware knows about the
> adapter, but not about anything on the bus that it attaches to.
> 
> This "hybrid" approach is basically saying that the firmware knows about
> the HBA, and it wants a chance to be notified of errors on the bus it
> attaches to, but the firmware can't do anything about such errors.
> 
> The bus in this case is PCIe, where we have capable error handling in the
> kernel driver, so we ultimately want the AER driver handling the errors.

Not a problem - GHES already knows about AER and calls into it for
CPER_SEC_PCIE errors:

ghes_do_proc
-> ghes_handle_aer
  |-> aer_recover_queue

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/3] PCI/AER: Option to leave System Error Interrupts as-is

[Keith Busch]

On Fri, Nov 02, 2018 at 05:26:23PM +0100, Borislav Petkov wrote:
> On Fri, Nov 02, 2018 at 10:17:30AM -0600, Keith Busch wrote:
> > VMD acts a bit like a host-bus adapter. The firmware knows about the
> > adapter, but not about anything on the bus that it attaches to.
> > 
> > This "hybrid" approach is basically saying that the firmware knows about
> > the HBA, and it wants a chance to be notified of errors on the bus it
> > attaches to, but the firmware can't do anything about such errors.
> > 
> > The bus in this case is PCIe, where we have capable error handling in the
> > kernel driver, so we ultimately want the AER driver handling the errors.
> 
> Not a problem - GHES already knows about AER and calls into it for
> CPER_SEC_PCIE errors:
> 
> ghes_do_proc
> -> ghes_handle_aer
>   |-> aer_recover_queue

That requires firmware know about the PCIe domain that experienced an
error so that it can provide an appropriate CPER. That wouldn't be
possible for errors occuring within a VMD domain.