diff mbox

[1/2] PCI: Prevent bus conflicts while checking for bridge apertures

Message ID 20131206001947.27659.14981.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bjorn Helgaas Dec. 6, 2013, 12:19 a.m. UTC 
pci_bridge_check_ranges() determines whether the bridge supports an I/O
aperture and a prefetchable memory aperture.

Previously, if the I/O aperture was unsupported, disabled, or configured at
[io 0x0000-0x0fff], we wrote 0xf0 to PCI_IO_BASE and PCI_IO_LIMIT, which,
if the bridge supports it, enables the I/O aperture at [io 0xf000-0xffff].
The enabled aperture may conflict with other devices in the system.

Similarly, we wrote 0xfff0 to PCI_PREF_MEMORY_BASE and
PCI_PREF_MEMORY_LIMIT, which enables the prefetchable memory aperture at
[mem 0xfff00000-0xffffffff], and that may also conflict with other devices.

All we need to know is whether the base and limit registers are writable,
so we can use values that leave the apertures disabled, e.g., PCI_IO_BASE =
0xf0, PCI_IO_LIMIT = 0xe0, PCI_PREF_MEMORY_BASE = 0xfff0,
PCI_PREF_MEMORY_LIMIT = 0xffe0.

Writing non-zero values to both the base and limit registers means we
detect whether either or both are writable, as we did before.

Reported-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Based-on-patch-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/setup-bus.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


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

Comments

Jason Gunthorpe Dec. 9, 2013, 7:31 p.m. UTC | #1 
On Thu, Dec 05, 2013 at 05:19:47PM -0700, Bjorn Helgaas wrote:
> pci_bridge_check_ranges() determines whether the bridge supports an I/O
> aperture and a prefetchable memory aperture.
> 
> Previously, if the I/O aperture was unsupported, disabled, or configured at
> [io 0x0000-0x0fff], we wrote 0xf0 to PCI_IO_BASE and PCI_IO_LIMIT, which,
> if the bridge supports it, enables the I/O aperture at [io 0xf000-0xffff].
> The enabled aperture may conflict with other devices in the system.
> 
> Similarly, we wrote 0xfff0 to PCI_PREF_MEMORY_BASE and
> PCI_PREF_MEMORY_LIMIT, which enables the prefetchable memory aperture at
> [mem 0xfff00000-0xffffffff], and that may also conflict with other devices.
> 
> All we need to know is whether the base and limit registers are writable,
> so we can use values that leave the apertures disabled, e.g., PCI_IO_BASE =
> 0xf0, PCI_IO_LIMIT = 0xe0, PCI_PREF_MEMORY_BASE = 0xfff0,
> PCI_PREF_MEMORY_LIMIT = 0xffe0.
> 
> Writing non-zero values to both the base and limit registers means we
> detect whether either or both are writable, as we did before.

Looks sane to me. The only refinement would be to detect if IO is
enabled and use that as a first check.

Cheers,
Jason
--
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
Bjorn Helgaas Dec. 9, 2013, 8 p.m. UTC | #2 
On Mon, Dec 9, 2013 at 12:31 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Dec 05, 2013 at 05:19:47PM -0700, Bjorn Helgaas wrote:
>> pci_bridge_check_ranges() determines whether the bridge supports an I/O
>> aperture and a prefetchable memory aperture.
>>
>> Previously, if the I/O aperture was unsupported, disabled, or configured at
>> [io 0x0000-0x0fff], we wrote 0xf0 to PCI_IO_BASE and PCI_IO_LIMIT, which,
>> if the bridge supports it, enables the I/O aperture at [io 0xf000-0xffff].
>> The enabled aperture may conflict with other devices in the system.
>>
>> Similarly, we wrote 0xfff0 to PCI_PREF_MEMORY_BASE and
>> PCI_PREF_MEMORY_LIMIT, which enables the prefetchable memory aperture at
>> [mem 0xfff00000-0xffffffff], and that may also conflict with other devices.
>>
>> All we need to know is whether the base and limit registers are writable,
>> so we can use values that leave the apertures disabled, e.g., PCI_IO_BASE =
>> 0xf0, PCI_IO_LIMIT = 0xe0, PCI_PREF_MEMORY_BASE = 0xfff0,
>> PCI_PREF_MEMORY_LIMIT = 0xffe0.
>>
>> Writing non-zero values to both the base and limit registers means we
>> detect whether either or both are writable, as we did before.
>
> Looks sane to me. The only refinement would be to detect if IO is
> enabled and use that as a first check.

I assume you mean checking PCI_COMMAND_IO, as in this fragment you
posted earlier:

+               pci_read_config_word(bridge, PCI_COMMAND, &command);
+               if (command & PCI_COMMAND_IO)
+                       b_res[0].flags |= IORESOURCE_IO;
+               else {
+                       pci_write_config_word(bridge, PCI_IO_BASE, 0x00f0);

That would avoid the writes to PCI_IO_BASE in the case where an I/O
aperture is already enabled and configured at [io 0x0000-0x0fff],
which has the advantage of not temporarily disabling the aperture.

I don't think it's 100% spec compliant because the spec allows
PCI_COMMAND_IO to be set if the bridge has an enabled I/O BAR but does
not support an I/O aperture.  I admit I have never seen such a device,
and I doubt one exists :)

To my mind, it's not worth adding the check, because we shouldn't be
sending I/O accesses to devices behind the bridge at this point
anyway, so the temporary disable shouldn't be a problem.  If we *are*
sending accesses to such a device, the current code moves the
aperture, so the bridge won't claim them, and we already have a
problem.  So I don't think my patch will add any problems there, and
it can conceivably prevent a conflict with another device at [io
0xf000-0xffff].

Bjorn
--
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
Jason Gunthorpe Dec. 9, 2013, 8:15 p.m. UTC | #3 
On Mon, Dec 09, 2013 at 01:00:36PM -0700, Bjorn Helgaas wrote:

> I don't think it's 100% spec compliant because the spec allows
> PCI_COMMAND_IO to be set if the bridge has an enabled I/O BAR but does
> not support an I/O aperture.  I admit I have never seen such a device,
> and I doubt one exists :)

Yes, fair enough!

Jason
--
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
diff mbox

Patch

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 219a4106480a..80350299a6ea 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -665,21 +665,23 @@  static void pci_bridge_check_ranges(struct pci_bus *bus)
 
 	pci_read_config_word(bridge, PCI_IO_BASE, &io);
 	if (!io) {
-		pci_write_config_word(bridge, PCI_IO_BASE, 0xf0f0);
+		pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0);
 		pci_read_config_word(bridge, PCI_IO_BASE, &io);
 		pci_write_config_word(bridge, PCI_IO_BASE, 0x0);
 	}
 	if (io)
 		b_res[0].flags |= IORESOURCE_IO;
+
 	/*  DECchip 21050 pass 2 errata: the bridge may miss an address
 	    disconnect boundary by one PCI data phase.
 	    Workaround: do not use prefetching on this device. */
 	if (bridge->vendor == PCI_VENDOR_ID_DEC && bridge->device == 0x0001)
 		return;
+
 	pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &pmem);
 	if (!pmem) {
 		pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE,
-					       0xfff0fff0);
+					       0xffe0fff0);
 		pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &pmem);
 		pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, 0x0);
 	}