Re: [PATCH] PCI: Check whether bridges allow access to extended config space

[Gilles Buloz]

Le 04/05/2018 00:31, Bjorn Helgaas a écrit :
> [+cc LKML]
>
> On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
>> Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR
>>
>> Even if a device supports extended config access, no such access must be
>> done to this device If there's a bridge not supporting that in the path
>> to this device. Doing such access with UR reporting enabled on the root
>> bridge leads to an exception.
>>
>> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
>> the following bus topology :
>>    LS1043 PCIe root
>>      -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>>        -> PMC slot connector (for legacy PMC modules)
>> With a PMC module topology as follows :
>>    PMC connector
>>      -> PCI-to-PCIe bridge
>>        -> PCIe switch (4 ports)
>>          -> 4 PCIe devices (one on each port)
>> In this case all devices behind the PEX8112 are supporting extended config
>> access but this is prohibited by the PEX8112. Without this patch, an
>> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
>>
>> This patch checks the parent bridge of each allocated child bus to know if
>> extended config access is supported on the child bus, and sets a flag in
>> child->bus_flags if not supported. This  flag is inherited by all children
>> buses of this child bus and then is checked to avoid this unsupported
>> accesses to every device on these buses.
> Hi Gilles,
>
> Thanks for the patch!  I reworked it a little bit to simplify the code
> in pci_alloc_child_bus().  Can you test it and make sure I didn't
> break anything?
>
Hi Bjorn,

Your rework works as expected. Tested on LS1043A platform with kernel 4.17-rc1, and with some backport on kernel 4.1.35

Suggestion : maybe change the pci_info() string to have :
     pci_bus 0000:xx: extended config space not accessible
instead of
     pci_bus 0000:xx: extended config space not accessible on secondary bus
as xx is already the number of the secondary bus

Info : with kernel 4.17-rc1, it turns out I need pcie_aspm=off to have the PMC devices behind the
PCI-to-PCIe bridge of the PMC safely detected/configured. But this is not caused by the patch.
Without pcie_aspm=off I saw this at one boot :
    "pci 0000:02:0e.0: ASPM: Could not configure common clock" for this bridge, but devices
    correctly detected/configured
but at most boots I get :
    no ASPM message but "pci 0000:04:02.0: bridge configuration invalid ([bus ff-ff]), reconfiguring "
    instead, and some devices are missing. Also lspci show "rev ff" for some devices.
I don't see this problem on 4.1.35 with the same backported patch.

Gilles
> commit cbaaa85b558a8f974e301fa0364d568efaf491ce
> Author: Gilles Buloz <Gilles.Buloz@xxxxxxxxxxx>
> Date:   Thu May 3 15:21:44 2018 -0500
>
>      PCI: Check whether bridges allow access to extended config space
>      
>      Even if a device supports extended config space, i.e., it is a PCI-X Mode 2
>      or a PCI Express device, the extended space may not be accessible if
>      there's a conventional PCI bus in the path to it.
>      
>      We currently figure that out in pci_cfg_space_size() by reading the first
>      dword of extended config space.  On most platforms that returns ~0 data if
>      the space is inaccessible, but it may set error bits in PCI status
>      registers, and on some platforms it causes exceptions that we currently
>      don't recover from.
>      
>      For example, a PCIe-to-conventional PCI bridge treats config transactions
>      with a non-zero Extended Register Address as an Unsupported Request on PCIe
>      and a received Master-Abort on the destination bus (see PCI Express to
>      PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
>      
>      A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with the
>      following bus topology:
>      
>        LS1043 PCIe Root Port
>          -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI side)
>            -> PMC slot connector (for legacy PMC modules)
>      
>      With a PMC module topology as follows:
>      
>        PMC connector
>          -> PCI-to-PCIe bridge
>            -> PCIe switch (4 ports)
>              -> 4 PCIe devices (one on each port)
>      
>      The PCIe devices on the PMC module support extended config space, but we
>      can't reach it because the PEX8112 can't generate accesses to the extended
>      space on its secondary bus.  Attempts to access it cause Unsupported
>      Request errors, which result in synchronous aborts on this platform.
>      
>      To avoid these errors, check whether bridges are capable of generating
>      extended config space addresses on their secondary interfaces.  If they
>      can't, we restrict devices below the bridge to only the 256-byte
>      PCI-compatible config space.
>      
>      Signed-off-by: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
>      [bhelgaas: changelog, rework patch so bus_flags testing is all in
>      pci_bridge_child_ext_cfg_accessible()]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac91b6fd0bcd..7b1b7b2e01e4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>   	return err;
>   }
>   
> +static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
> +{
> +	int pos;
> +	u32 status;
> +
> +	/*
> +	 * If extended config space isn't accessible on a bridge's primary
> +	 * bus, we certainly can't access it on the secondary bus.
> +	 */
> +	if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return false;
> +
> +	/*
> +	 * PCIe Root Ports and switch ports are PCIe on both sides, so if
> +	 * extended config space is accessible on the primary, it's also
> +	 * accessible on the secondary.
> +	 */
> +	if (pci_is_pcie(bridge) &&
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM ||
> +	     pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM))
> +		return true;
> +
> +	/*
> +	 * For the other bridge types:
> +	 *   - PCI-to-PCI bridges
> +	 *   - PCIe-to-PCI/PCI-X forward bridges
> +	 *   - PCI/PCI-X-to-PCIe reverse bridges
> +	 * extended config space on the secondary side is only accessible
> +	 * if the bridge supports PCI-X Mode 2.
> +	 */
> +	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> +	if (!pos)
> +		return false;
> +
> +	pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> +	return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
> +}
> +
>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   					   struct pci_dev *bridge, int busnr)
>   {
> @@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   	pci_set_bus_of_node(child);
>   	pci_set_bus_speed(child);
>   
> +	/*
> +	 * Check whether extended config space is accessible on the child
> +	 * bus.  Note that we currently assume it is always accessible on
> +	 * the root bus.
> +	 */
> +	if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
> +		pci_info(child, "extended config space not accessible on secondary bus\n");
> +	}
> +
>   	/* Set up default resource pointers and names */
>   	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
>   		child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
> @@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
>   	u32 status;
>   	u16 class;
>   
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return PCI_CFG_SPACE_SIZE;
> +
>   	class = dev->class >> 8;
>   	if (class == PCI_CLASS_BRIDGE_HOST)
>   		return pci_cfg_space_size_ext(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 230615620a4a..f7aa6d9f8999 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -217,6 +217,7 @@ enum pci_bus_flags {
>   	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
>   	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
>   	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
> +	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
>   };
>   
>   /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
>
> .
>