[Skiboot] [PATCH 2/3] pci-quirk: Microsemi switch UR workaround

[Alistair Popple]

On Friday, 19 July 2019 7:38:20 PM AEST Oliver O'Halloran wrote:
> Some Microsemi switches have a bug where they send URs if you perform a
> config read outside of a PCI Capability register block. Linux allows
> users to read the whole of config space and some tools (like lspci) will
> do this.
> 
> On POWER chips this UR triggers a spurious EEH which causes all manner
> of hell. This patch adds a pci-quirk for the relevant switch that blocks
> config space read and writes to the switch to prevent the issue.
> 
> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
> ---
>  core/pci-quirk.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/core/pci-quirk.c b/core/pci-quirk.c
> index 0862e8dc4072..371ff62b4b72 100644
> --- a/core/pci-quirk.c
> +++ b/core/pci-quirk.c
> @@ -18,10 +18,67 @@
> 
>  #include <skiboot.h>
>  #include <pci.h>
> +#include <pci-cfg.h>
>  #include <pci-quirk.h>
>  #include <platform.h>
>  #include <ast.h>
> 
> +static int64_t cfg_block_filter(void *dev __unused,
> +				struct pci_cfg_reg_filter *pcrf __unused,
> +				uint32_t offset __unused, uint32_t len,
> +				uint32_t *data, bool write)
> +{
> +	if (write)
> +		return OPAL_SUCCESS;
> +
> +	switch (len) {
> +	case 4:
> +		*data = 0xF0F0F0F0;

Why 0xF0 and not 0x00? The PCIe spec seems to imply that unimplemented config 
space registers should read 0. Although if everything followed the spec this 
patch wouldn't exist.

> +		return OPAL_SUCCESS;
> +	case 2:
> +		*((uint16_t *)data) = 0xF0F0;
> +		return OPAL_SUCCESS;
> +	case 1:
> +		*((uint8_t *)data) = 0xF0;
> +		return OPAL_SUCCESS;
> +	}

Couldn't the above could be simplified to:

	*data = 0xF0F0F0F0;
	return OPAL_SUCCESS;

> +	return OPAL_PARAMETER; /* should never happen */
> +}
> +
> +/* blocks config accesses to registers in the range: [start, end] */
> +#define BLOCK_CFG_RANGE(pd, start, end) \
> +	pci_add_cfg_reg_filter(pd, start, end - start + 1, \
> +		PCI_REG_FLAG_WRITE | PCI_REG_FLAG_READ, \
> +		cfg_block_filter);
> +
> +static void quirk_microsemi_gen4_sw(struct phb *phb, struct pci_device *pd)
> +{
> +	/*
> +	 * The gen4 pcie switch used on some ZZ systems has a bug where it'll
> +	 * throw URs in response to a cfg read to a range that's "reserved"
> +	 * work around it by blackholing.
> +	 */
> +	if (pd->dev_type == PCIE_TYPE_ENDPOINT && pd->class == 0x058000) {
> +		/*
> +		 * we match on the class code too since the switch might
> +		 * support an NTB endpoint.
> +		 */
> +		BLOCK_CFG_RANGE(pd, 0xa0, 0xff);
> +		BLOCK_CFG_RANGE(pd, 0x17c, 0xfff);
> +	} else if (pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) {
> +		BLOCK_CFG_RANGE(pd, 0x09c, 0xff);
> +		BLOCK_CFG_RANGE(pd, 0x284, 0x7f7);
> +	} else if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT) {
> +		BLOCK_CFG_RANGE(pd, 0x09c, 0xff);
> +		BLOCK_CFG_RANGE(pd, 0x288, 0x7f7);

I assume we don't expect a device with the same VDID to ever have a different 
config space layout? Would it be worth matching on the RevID as well? You would 
hope at least that would get bumped if the config space changed.

- Alistair

> +	} else {
> +		return;
> +	}
> +
> +	PCINOTICE(phb, pd->bdfn, "Applied Microsemi Gen4 UR workaround\n");
> +}
> +
>  static void quirk_astbmc_vga(struct phb *phb __unused,
>  			     struct pci_device *pd)
>  {
> @@ -53,6 +110,7 @@ static void quirk_astbmc_vga(struct phb *phb __unused,
>  static const struct pci_quirk quirk_table[] = {
>  	/* ASPEED 2400 VGA device */
>  	{ 0x1a03, 0x2000, &quirk_astbmc_vga },
> +	{ 0x11f8, 0x4052, &quirk_microsemi_gen4_sw },
>  	{ 0, 0, NULL }
>  };