[PATCH] Restore PCI bridge configuration space on bridge reset

[PATCH] Restore PCI bridge configuration space on bridge reset

[geoff]

According to PCI-to-PCI Bridge Architecture Specification 3.2.5.17

> The bridge’s secondary bus interface and any buffers between
> the two interfaces (primary and secondary) must be initialized
> back to their default state whenever this bit is set.

Failure to observe this causes inability to access devices on the 
secondary bus
on the AMD Threadripper platform after device reset when the device is 
being
used for PCI passthrough with KVM.

The following patch corrects this by saving the pci state and restoring 
it after
the bus has been reset.

--Signed-off-by: Geoffrey McRae<geoff@hostfission.com>

--- ./drivers/pci/pci.c.orig	2018-01-24 18:30:23.913953332 +1100
+++ ./drivers/pci/pci.c	2018-01-24 18:46:31.752819451 +1100
@@ -1112,12 +1112,12 @@ int pci_save_state(struct pci_dev *dev)
  EXPORT_SYMBOL(pci_save_state);

  static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
-				     u32 saved_val, int retry)
+				     u32 saved_val, int retry, int force)
  {
  	u32 val;

  	pci_read_config_dword(pdev, offset, &val);
-	if (val == saved_val)
+	if (!force && val == saved_val)
  		return;

  	for (;;) {
@@ -1136,33 +1136,29 @@ static void pci_restore_config_dword(str
  }

  static void pci_restore_config_space_range(struct pci_dev *pdev,
-					   int start, int end, int retry)
+					   int start, int end, int retry, int force)
  {
  	int index;

  	for (index = end; index >= start; index--)
  		pci_restore_config_dword(pdev, 4 * index,
  					 pdev->saved_config_space[index],
-					 retry);
+					 retry, force);
  }

-static void pci_restore_config_space(struct pci_dev *pdev)
+static void pci_restore_config_space(struct pci_dev *pdev, int force)
  {
  	if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
-		pci_restore_config_space_range(pdev, 10, 15, 0);
+		pci_restore_config_space_range(pdev, 10, 15, 0, force);
  		/* Restore BARs before the command register. */
-		pci_restore_config_space_range(pdev, 4, 9, 10);
-		pci_restore_config_space_range(pdev, 0, 3, 0);
+		pci_restore_config_space_range(pdev, 4, 9, 10, force);
+		pci_restore_config_space_range(pdev, 0, 3, 0, force);
  	} else {
-		pci_restore_config_space_range(pdev, 0, 15, 0);
+		pci_restore_config_space_range(pdev, 0, 15, 0, force);
  	}
  }

-/**
- * pci_restore_state - Restore the saved state of a PCI device
- * @dev: - PCI device that we're dealing with
- */
-void pci_restore_state(struct pci_dev *dev)
+static void _pci_restore_state(struct pci_dev *dev, int force)
  {
  	if (!dev->state_saved)
  		return;
@@ -1176,7 +1172,7 @@ void pci_restore_state(struct pci_dev *d

  	pci_cleanup_aer_error_status_regs(dev);

-	pci_restore_config_space(dev);
+	pci_restore_config_space(dev, force);

  	pci_restore_pcix_state(dev);
  	pci_restore_msi_state(dev);
@@ -1187,6 +1183,15 @@ void pci_restore_state(struct pci_dev *d

  	dev->state_saved = false;
  }
+
+/**
+ * pci_restore_state - Restore the saved state of a PCI device
+ * @dev: - PCI device that we're dealing with
+ */
+void pci_restore_state(struct pci_dev *dev)
+{
+	_pci_restore_state(dev, 0);
+}
  EXPORT_SYMBOL(pci_restore_state);

  struct pci_saved_state {
@@ -4083,6 +4088,8 @@ void pci_reset_secondary_bus(struct pci_
  {
  	u16 ctrl;

+  pci_save_state(dev);
+
  	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &ctrl);
  	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
@@ -4092,10 +4099,23 @@ void pci_reset_secondary_bus(struct pci_
  	 */
  	msleep(2);

+	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &ctrl);
  	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
  	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);

  	/*
+	 * According to PCI-to-PCI Bridge Architecture Specification 3.2.5.17
+	 *
+	 * "The bridge’s secondary bus interface and any buffers between
+	 * the two interfaces (primary and secondary) must be initialized
+	 * back to their default state whenever this bit is set."
+	 *
+	 * Failure to observe this causes inability to access devices on the
+	 * secondary bus on the AMD Threadripper platform.
+	 */
+	_pci_restore_state(dev, 1);
+
+	/*
  	 * Trhfa for conventional PCI is 2^25 clock cycles.
  	 * Assuming a minimum 33MHz clock this results in a 1s
  	 * delay before we can consider subordinate devices to

Re: [PATCH] Restore PCI bridge configuration space on bridge reset

[Alex Williamson]

On Wed, 24 Jan 2018 19:02:33 +1100
geoff@hostfission.com wrote:

> According to PCI-to-PCI Bridge Architecture Specification 3.2.5.17

Correction, rev 1.2 section 3.2.5.18, in reference to the secondary bus
reset bit in the bridge control register.
 
> > The bridge’s secondary bus interface and any buffers between
> > the two interfaces (primary and secondary) must be initialized
> > back to their default state whenever this bit is set.  
> 
> Failure to observe this causes inability to access devices on the 
> secondary bus
> on the AMD Threadripper platform after device reset when the device is 
> being
> used for PCI passthrough with KVM.
> 
> The following patch corrects this by saving the pci state and restoring 
> it after
> the bus has been reset.

How do configuration registers on the primary bus interface fall into
this requirement?  It's not very clear from the spec what these
"buffers" are and the secondary interface has no configuration
registers itself.  Figure 1-2 shows Transaction/Data Buffers which are
clearly separate from the Primary Interface Configuration Registers.
I'd tend to say this excerpt of the spec is describing a hardware
requirement, not a software requirement.

I know that people have found that re-writing bridge registers on
threadripper solves the reset problem, but this seems like a bit of a
stretch to attribute it to this spec statement.  Maybe it can be
handled via a quirk if AMD isn't planning to release firmware that
resolves this issue?  AMD...  Thanks,

Alex

Re: [PATCH] Restore PCI bridge configuration space on bridge reset

[geoff]

On 2018-01-25 08:10, Alex Williamson wrote:
> On Wed, 24 Jan 2018 19:02:33 +1100
> geoff@hostfission.com wrote:
> 
>> According to PCI-to-PCI Bridge Architecture Specification 3.2.5.17
> 
> Correction, rev 1.2 section 3.2.5.18, in reference to the secondary bus
> reset bit in the bridge control register.
> 

Thanks, I will make this correction if the patch is deemed valid re 
below.
Please excuse any confusing terminology/wording, I am still coming to
terms with how PCI operates.

>> > The bridge’s secondary bus interface and any buffers between
>> > the two interfaces (primary and secondary) must be initialized
>> > back to their default state whenever this bit is set.
>> 
>> Failure to observe this causes inability to access devices on the
>> secondary bus
>> on the AMD Threadripper platform after device reset when the device is
>> being
>> used for PCI passthrough with KVM.
>> 
>> The following patch corrects this by saving the pci state and 
>> restoring
>> it after
>> the bus has been reset.
> 
> How do configuration registers on the primary bus interface fall into
> this requirement?  It's not very clear from the spec what these
> "buffers" are and the secondary interface has no configuration
> registers itself.  Figure 1-2 shows Transaction/Data Buffers which are
> clearly separate from the Primary Interface Configuration Registers.
> I'd tend to say this excerpt of the spec is describing a hardware
> requirement, not a software requirement.

These are not the configuration registers on the primary bus but on the
secondary bus, in the case of a TR system a "PCIe GPP Bridge" device is
created and the PCI device is placed under it. It is this bridge that
needs it's configuration space rewritten.

Unless I am mistaken, currently pci.c is inconsistent with secondary bus
resets as it is. In `pci_reset_bus` the bus configuration space is saved
via `pci_bus_save_and_disable`, the bus is reset, and then the 
configuration
is reloaded using `pci_bus_restore`.

`pci_try_reset_bus` is different again, in that it calls
`pci_reset_bridge_secondary_bus` also.

In short, it is already happening under certain circumstances, but 
because
on TR the CPU view of the PCI configuration space seems to be cached, it 
is
unable to determine the changes and thus a blind re-write is required.

> 
> I know that people have found that re-writing bridge registers on
> threadripper solves the reset problem, but this seems like a bit of a
> stretch to attribute it to this spec statement.  Maybe it can be
> handled via a quirk if AMD isn't planning to release firmware that
> resolves this issue?  AMD...  Thanks,
> 

I'd love to see this fixed in firmware/bios/microcode, etc... but as the
spec reads, it is unclear if this is a software or hardware requirement, 
IMO
it is a software requirement to reconfigure the configuration space of 
the
secondary bus, but my understanding of PCI at this time is quite new so 
I
am ready to accept a final decision by someone with more experience.

Re: [PATCH] Restore PCI bridge configuration space on bridge reset

[Alex Williamson]

On Thu, 25 Jan 2018 09:28:59 +1100
geoff@hostfission.com wrote:

> On 2018-01-25 08:10, Alex Williamson wrote:
> > On Wed, 24 Jan 2018 19:02:33 +1100
> > geoff@hostfission.com wrote:
> >   
> >> According to PCI-to-PCI Bridge Architecture Specification 3.2.5.17  
> > 
> > Correction, rev 1.2 section 3.2.5.18, in reference to the secondary bus
> > reset bit in the bridge control register.
> >   
> 
> Thanks, I will make this correction if the patch is deemed valid re 
> below.
> Please excuse any confusing terminology/wording, I am still coming to
> terms with how PCI operates.
> 
> >> > The bridge’s secondary bus interface and any buffers between
> >> > the two interfaces (primary and secondary) must be initialized
> >> > back to their default state whenever this bit is set.  
> >> 
> >> Failure to observe this causes inability to access devices on the
> >> secondary bus
> >> on the AMD Threadripper platform after device reset when the device is
> >> being
> >> used for PCI passthrough with KVM.
> >> 
> >> The following patch corrects this by saving the pci state and 
> >> restoring
> >> it after
> >> the bus has been reset.  
> > 
> > How do configuration registers on the primary bus interface fall into
> > this requirement?  It's not very clear from the spec what these
> > "buffers" are and the secondary interface has no configuration
> > registers itself.  Figure 1-2 shows Transaction/Data Buffers which are
> > clearly separate from the Primary Interface Configuration Registers.
> > I'd tend to say this excerpt of the spec is describing a hardware
> > requirement, not a software requirement.  
> 
> These are not the configuration registers on the primary bus but on the
> secondary bus, in the case of a TR system a "PCIe GPP Bridge" device is
> created and the PCI device is placed under it. It is this bridge that
> needs it's configuration space rewritten.

There are no configuration registers on the secondary interface, see:

3.1.1 Type 0 Configuration Transaction Support

  A bridge only responds to Type 0 configuration transactions on its
  primary PCI interface when being configured. A bridge ignores Type 0
  configuration transactions that originate on the secondary interface
  of the bridge. Thus, the bridge does not implement IDSEL on its
  secondary interface. A Type 0 configuration transaction is used to
  configure the bridge and is not forwarded downstream by the bridge
  (from its primary to secondary interface). 

We interact with the bridge on the primary interface in order to reset
the secondary interface.

> Unless I am mistaken, currently pci.c is inconsistent with secondary
>   bus resets as it is. In `pci_reset_bus` the bus configuration space
>   is saved via `pci_bus_save_and_disable`, the bus is reset, and then
>   the configuration
> is reloaded using `pci_bus_restore`.

pci_reset_bus
  pci_bus_save_and_disable
  pci_bus_reset
    pci_bus_lock
    pci_reset_bridge_secondary_bus
      pcibios_reset_secondary_bus
        pci_reset_secondary_bus
    pci_bus_unlock
  pci_bus_restore

> `pci_try_reset_bus` is different again, in that it calls
> `pci_reset_bridge_secondary_bus` also.

pci_try_reset_bus
  pci_bus_save_and_disable
  if (pci_bus_trylock()) {
    pci_reset_bridge_secondary_bus
      pcibios_reset_secondary_bus
        pci_reset_secondary_bus
    pci_bus_unlock
  }
  pci_bus_restore

What's inconsistent here?
 
> In short, it is already happening under certain circumstances, but 
> because
> on TR the CPU view of the PCI configuration space seems to be cached,
>   it is
> unable to determine the changes and thus a blind re-write is required.

Hmm, we'd be in real trouble if the CPU is caching config space.  Seems
more like we just can't trust the read value of the bridge registers.
This "write only if read value doesn't match saved" comes from here:

commit 04d9c1a1100b6bdeffa7e1bfc30080bdac28e183
Author: Dave Jones <davej@redhat.com>
Date:   Tue Apr 18 21:06:51 2006 -0700

    [PATCH] PCI: Improve PCI config space writeback
    
    At least one laptop blew up on resume from suspend with a black screen due
    to a lack of this patch.  By only writing back config space that is
    different, we minimise the possibility of accidents like this.
    
    Signed-off-by: Dave Jones <davej@redhat.com>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

It's unfortunate that we don't really have any more data than that, but
are we better off with something like:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4a7c6864fdf4..f4f625a08094 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1117,7 +1117,7 @@ static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
        u32 val;
 
        pci_read_config_dword(pdev, offset, &val);
-       if (val == saved_val)
+       if (!(pdev->dev_flags & PCI_DEV_FLAGS_RESTORE_ALL) && (val == saved_val))
                return;
 
        for (;;) {

And a quirk to set that flag on these bridges?

> > I know that people have found that re-writing bridge registers on
> > threadripper solves the reset problem, but this seems like a bit of
> >   a stretch to attribute it to this spec statement.  Maybe it can be
> > handled via a quirk if AMD isn't planning to release firmware that
> > resolves this issue?  AMD...  Thanks,
> >   
> 
> I'd love to see this fixed in firmware/bios/microcode, etc... but as
>   the spec reads, it is unclear if this is a software or hardware
>   requirement, IMO
> it is a software requirement to reconfigure the configuration space
>   of the
> secondary bus, but my understanding of PCI at this time is quite new
>   so I
> am ready to accept a final decision by someone with more experience.

I can't bring myself to support that interpretation of the spec.  The
hardware is in an inconsistent state, the read value doesn't match the
internal logic, that's not normal.  That sounds like a "we need a quirk
to identify this device as untrusted for restore" kind of bug.  Thanks,

Alex