| Message ID | 1372177330-28013-7-git-send-email-mika.westerberg@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 25 Jun 2013 19:22:10 +0300 Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > + pr_info("Thunderbolt host router detected disabling ROMs\n"); > + pci_probe |= PCI_NOASSIGN_ROMS; > + } I wonder if this should just be the default on x86? Or do we allocate ROM space to address some other platform where we need it and the BIOS doesn't do it for the devices we care about?
On Tue, Jun 25, 2013 at 02:15:56PM -0700, Jesse Barnes wrote: > On Tue, 25 Jun 2013 19:22:10 +0300 > Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > > + pr_info("Thunderbolt host router detected disabling ROMs\n"); > > + pci_probe |= PCI_NOASSIGN_ROMS; > > + } > > I wonder if this should just be the default on x86? Or do we allocate > ROM space to address some other platform where we need it and the BIOS > doesn't do it for the devices we care about? Good question. In our case it definitely helps to have pci=norom the default. Can't tell if it might break something that depends on the current behaviour. Bjorn, Greg, Rafael, What do you think? -- 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
On Wed, Jun 26, 2013 at 03:17:57PM +0300, Mika Westerberg wrote: > On Tue, Jun 25, 2013 at 02:15:56PM -0700, Jesse Barnes wrote: > > On Tue, 25 Jun 2013 19:22:10 +0300 > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > > > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > > > + pr_info("Thunderbolt host router detected disabling ROMs\n"); > > > + pci_probe |= PCI_NOASSIGN_ROMS; > > > + } > > > > I wonder if this should just be the default on x86? Or do we allocate > > ROM space to address some other platform where we need it and the BIOS > > doesn't do it for the devices we care about? > > Good question. In our case it definitely helps to have pci=norom the > default. Can't tell if it might break something that depends on the current > behaviour. > > Bjorn, Greg, Rafael, > > What do you think? I can't recall any specific reason to not do this, so no objection from me, but make it a nice and small patch that can easily be reverted if problems show up in the wild :) thanks, greg k-h -- 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
[+cc Alex] On Wed, Jun 26, 2013 at 6:17 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Jun 25, 2013 at 02:15:56PM -0700, Jesse Barnes wrote: >> On Tue, 25 Jun 2013 19:22:10 +0300 >> Mika Westerberg <mika.westerberg@linux.intel.com> wrote: >> >> > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { >> > + pr_info("Thunderbolt host router detected disabling ROMs\n"); >> > + pci_probe |= PCI_NOASSIGN_ROMS; >> > + } >> >> I wonder if this should just be the default on x86? Or do we allocate >> ROM space to address some other platform where we need it and the BIOS >> doesn't do it for the devices we care about? > > Good question. In our case it definitely helps to have pci=norom the > default. Can't tell if it might break something that depends on the current > behaviour. I think the current default behavior is that if the BIOS has assigned the ROM BAR, we keep that assignment, and if it hasn't, we allocate MEM space for it. And "pci=norom" means that we don't allocate MEM space for it, even if the BIOS hasn't assigned it. "pci=norom" is only implemented on x86. I think most other arches allocate MEM space for ROMs, with no way to turn that off. PA-RISC seems to ignore ROMs (dino_fixup_bus()), but that looks like the exception. I'm slightly concerned that if we make the x86 default be "never assign space for ROMs unless the BIOS has done it," we might break virtualized guests that need access to ROMs. Alex? 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
On Wed, 2013-06-26 at 14:59 -0600, Bjorn Helgaas wrote: > [+cc Alex] > > On Wed, Jun 26, 2013 at 6:17 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Tue, Jun 25, 2013 at 02:15:56PM -0700, Jesse Barnes wrote: > >> On Tue, 25 Jun 2013 19:22:10 +0300 > >> Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > >> > >> > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > >> > + pr_info("Thunderbolt host router detected disabling ROMs\n"); > >> > + pci_probe |= PCI_NOASSIGN_ROMS; > >> > + } > >> > >> I wonder if this should just be the default on x86? Or do we allocate > >> ROM space to address some other platform where we need it and the BIOS > >> doesn't do it for the devices we care about? > > > > Good question. In our case it definitely helps to have pci=norom the > > default. Can't tell if it might break something that depends on the current > > behaviour. > > I think the current default behavior is that if the BIOS has assigned > the ROM BAR, we keep that assignment, and if it hasn't, we allocate > MEM space for it. And "pci=norom" means that we don't allocate MEM > space for it, even if the BIOS hasn't assigned it. > > "pci=norom" is only implemented on x86. I think most other arches > allocate MEM space for ROMs, with no way to turn that off. PA-RISC > seems to ignore ROMs (dino_fixup_bus()), but that looks like the > exception. > > I'm slightly concerned that if we make the x86 default be "never > assign space for ROMs unless the BIOS has done it," we might break > virtualized guests that need access to ROMs. Alex? Yep, I'm more than slightly concerned by that too. Whenever possible we want to pass the ROM to the guest since it may end up being a boot device or drivers within the guest may require it. We can pass the ROM to the guest from an image file, but that requires someplace from which to dump the image, which is usually PCI sysfs. Thanks, Alex -- 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
On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. > This means that the BIOS will allocate bridge resources based on some > assumptions of a maximum Thunderbolt chain. It also disables native PCIe > hotplug of the root port where the Thunderbolt host router is connected. The BIOS often assigns PCI bridge windows, e.g., for root ports leading to ExpressCard slots. I assume BIOSes make similar assumptions there about what ExpressCards are likely to be plugged in. Is your BIOS assistance the same sort of thing, or is there something additional happening at hot-add time? (I think there might be AML that does Thunderbolt-specific stuff at hotplug-time, but I assume that's not related to resource assignment, because the OS owns those resources.) > In order to support this we must make sure that the kernel does not try to > be too smart and resize / open the bridge windows during PCI enumeration. > For example by default the kernel will add certain amount of space to the > bridge memory/io windows (this is configurable via pci=hp[mem|io]size=xxx > command line option). Eventually we run out of space that the BIOS has > allocated. ROMs usually aren't very big compared to regular memory BARs. Is the problem just that adding space the BIOS didn't plan for causes the OS to increase the window size and bump into space assigned to a peer of the Thunderbolt controller? > Also address space for expansion ROMs should not be allocated (BIOS does > not execute them for Thunderbolt endpoints). If we don't prevent this the > kernel might find expansion ROM associated with some endpoint and reopen > the bridge window which the BIOS already closed leading again resource > exhaustion. I assume the only reason we should not allocate expansion ROM space is to avoid resource exhaustion. If we had enough resources, allocating ROM space wouldn't cause anything bad to happen, would it? > Fix this by adding a quirk that matches known Thunderbolt PCI-to-PCI > bridges and in that case prevents allocation of expansion ROM resources and > makes sure that the PCI core does not increase size of bridge windows. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > arch/x86/pci/fixup.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c > index f5809fa..924822b 100644 > --- a/arch/x86/pci/fixup.c > +++ b/arch/x86/pci/fixup.c > @@ -7,6 +7,8 @@ > #include <linux/pci.h> > #include <linux/init.h> > #include <linux/vgaarb.h> > +#include <linux/acpi.h> > +#include <linux/pci-acpi.h> > #include <asm/pci_x86.h> > > static void pci_fixup_i450nx(struct pci_dev *d) > @@ -539,3 +541,52 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev) > } > } > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone); > + > +#ifdef CONFIG_ACPI > +/* > + * BIOS assisted Thunderbolt PCI enumeration should handle all resource > + * allocation on behalf of OS. > + */ > +static void quirk_thunderbolt(struct pci_dev *dev) > +{ > + struct acpi_pci_root *root; > + acpi_handle handle; > + > + handle = acpi_find_root_bridge_handle(dev); > + if (!handle) > + return; > + > + root = acpi_pci_find_root(handle); > + if (!root) > + return; > + > + /* > + * Native PCIe hotplug should be disabled when BIOS assisted > + * hotplug is in use. > + */ > + if (root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL) > + return; I'm not really sure why this test is here. I dimly recall hearing that Thunderbolt hotplug requires some work at hotplug-time, and this is not all public, and hence is done by AML. But that in itself doesn't seem related to the question of allocating ROM space. > + /* > + * Make sure that we don't allocate resources for expansion ROMs. > + * This may accidentally increase the size of the bridge window > + * causing us to run out of resources. > + */ > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > + pr_info("Thunderbolt host router detected disabling ROMs\n"); We have a pci_dev, so this should use dev_info(). > + pci_probe |= PCI_NOASSIGN_ROMS; I think you really only care about the space for ROMs on devices connected via Thunderbolt, so it's kind of a shame that the switch is global and affects ROMs on *all* devices. I guess there's nothing simple to be done about that, though. > + } > + > + /* > + * Don't add anything to the BIOS allocated bridge window size for > + * the same reason. > + */ > + dev->is_hotplug_bridge = 0; "is_hotplug_bridge" is also used in MPS management (pcie_find_smpss()). Did you investigate that and make sure this is safe? I think the Thunderbolt controller supports hotplug, and I'm not sure what will happen if the MPS code assumes it doesn't. > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1513, quirk_thunderbolt); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x151a, quirk_thunderbolt); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x151b, quirk_thunderbolt); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1547, quirk_thunderbolt); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1548, quirk_thunderbolt); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1549, quirk_thunderbolt); > +#endif > -- > 1.8.3.1 > -- 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
On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: >> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >> This means that the BIOS will allocate bridge resources based on some >> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >> hotplug of the root port where the Thunderbolt host router is connected. We should not need tricks in this patch after https://patchwork.kernel.org/patch/2766521/ [2/3] PCI / ACPI: Use boot-time resource allocation rules during hotplug Thanks Yinghai -- 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
On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >>> This means that the BIOS will allocate bridge resources based on some >>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >>> hotplug of the root port where the Thunderbolt host router is connected. > > We should not need tricks in this patch after > > https://patchwork.kernel.org/patch/2766521/ > > [2/3] PCI / ACPI: Use boot-time resource allocation rules during hotplug > BTW, Rafael, looks like that "boot-time" is not accurate here. During acpi hotplug, firmare could do extra help for us like assign some resources to pci device bars, so it is NOT "boot-time". Yinghai -- 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
On Wed, Jun 26, 2013 at 3:44 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Wednesday, June 26, 2013 03:31:18 PM Yinghai Lu wrote: >> On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> > On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >> >> <mika.westerberg@linux.intel.com> wrote: >> >>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >> >>> This means that the BIOS will allocate bridge resources based on some >> >>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >> >>> hotplug of the root port where the Thunderbolt host router is connected. >> > >> > We should not need tricks in this patch after >> > >> > https://patchwork.kernel.org/patch/2766521/ >> > >> > [2/3] PCI / ACPI: Use boot-time resource allocation rules during hotplug >> > >> >> BTW, Rafael, looks like that "boot-time" is not accurate here. >> >> During acpi hotplug, firmare could do extra help for us like assign >> some resources to pci device bars, so it is NOT "boot-time". > > Well, Linus has merged it already and besides "boot-time rules" need not > mean "boot-time resources", right? ok. -- 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
On Wednesday, June 26, 2013 03:31:18 PM Yinghai Lu wrote: > On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > >> <mika.westerberg@linux.intel.com> wrote: > >>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. > >>> This means that the BIOS will allocate bridge resources based on some > >>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe > >>> hotplug of the root port where the Thunderbolt host router is connected. > > > > We should not need tricks in this patch after > > > > https://patchwork.kernel.org/patch/2766521/ > > > > [2/3] PCI / ACPI: Use boot-time resource allocation rules during hotplug > > > > BTW, Rafael, looks like that "boot-time" is not accurate here. > > During acpi hotplug, firmare could do extra help for us like assign > some resources to pci device bars, so it is NOT "boot-time". Well, Linus has merged it already and besides "boot-time rules" need not mean "boot-time resources", right? Rafael
On Wed, Jun 26, 2013 at 4:31 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >>> <mika.westerberg@linux.intel.com> wrote: >>>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >>>> This means that the BIOS will allocate bridge resources based on some >>>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >>>> hotplug of the root port where the Thunderbolt host router is connected. > ... > During acpi hotplug, firmare could do extra help for us like assign > some resources to pci device bars, so it is NOT "boot-time". Really? How can firmware assign BARs at hotplug-time? I mean, obviously firmware *can* write things to the BARs before giving the device to the OS, but how would it know what to write? I assume the OS owns the address space, and it can change the upstream bridge windows or the BARs of another device on the bus at any time, subject to the OS's own issues as far as quiescing or unbinding drivers, etc., but without coordinating with the BIOS. 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
On Wed, Jun 26, 2013 at 3:55 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Wed, Jun 26, 2013 at 4:31 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>> On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>>> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >>>> <mika.westerberg@linux.intel.com> wrote: >>>>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >>>>> This means that the BIOS will allocate bridge resources based on some >>>>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >>>>> hotplug of the root port where the Thunderbolt host router is connected. >> ... >> During acpi hotplug, firmare could do extra help for us like assign >> some resources to pci device bars, so it is NOT "boot-time". > > Really? How can firmware assign BARs at hotplug-time? I mean, > obviously firmware *can* write things to the BARs before giving the > device to the OS, but how would it know what to write? should be acpi code, or SMI code or even BMC firmware via sideband. > I assume the > OS owns the address space, and it can change the upstream bridge > windows or the BARs of another device on the bus at any time, subject > to the OS's own issues as far as quiescing or unbinding drivers, etc., > but without coordinating with the BIOS. for thunderbolt or dock with acpiphp, then all children devices/bridges should not have drivers loaded yet. Yinghai -- 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
On Wed, Jun 26, 2013 at 04:15:01PM -0600, Alex Williamson wrote: > On Wed, 2013-06-26 at 14:59 -0600, Bjorn Helgaas wrote: > > [+cc Alex] > > > > On Wed, Jun 26, 2013 at 6:17 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Tue, Jun 25, 2013 at 02:15:56PM -0700, Jesse Barnes wrote: > > >> On Tue, 25 Jun 2013 19:22:10 +0300 > > >> Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > >> > > >> > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > > >> > + pr_info("Thunderbolt host router detected disabling ROMs\n"); > > >> > + pci_probe |= PCI_NOASSIGN_ROMS; > > >> > + } > > >> > > >> I wonder if this should just be the default on x86? Or do we allocate > > >> ROM space to address some other platform where we need it and the BIOS > > >> doesn't do it for the devices we care about? > > > > > > Good question. In our case it definitely helps to have pci=norom the > > > default. Can't tell if it might break something that depends on the current > > > behaviour. > > > > I think the current default behavior is that if the BIOS has assigned > > the ROM BAR, we keep that assignment, and if it hasn't, we allocate > > MEM space for it. And "pci=norom" means that we don't allocate MEM > > space for it, even if the BIOS hasn't assigned it. > > > > "pci=norom" is only implemented on x86. I think most other arches > > allocate MEM space for ROMs, with no way to turn that off. PA-RISC > > seems to ignore ROMs (dino_fixup_bus()), but that looks like the > > exception. > > > > I'm slightly concerned that if we make the x86 default be "never > > assign space for ROMs unless the BIOS has done it," we might break > > virtualized guests that need access to ROMs. Alex? > > Yep, I'm more than slightly concerned by that too. Whenever possible we > want to pass the ROM to the guest since it may end up being a boot > device or drivers within the guest may require it. We can pass the ROM > to the guest from an image file, but that requires someplace from which > to dump the image, which is usually PCI sysfs. Thanks, OK, then I guess changing the default is out of the question. We don't want to break things. -- 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
On Wed, Jun 26, 2013 at 04:18:53PM -0600, Bjorn Helgaas wrote: > On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. > > This means that the BIOS will allocate bridge resources based on some > > assumptions of a maximum Thunderbolt chain. It also disables native PCIe > > hotplug of the root port where the Thunderbolt host router is connected. > > The BIOS often assigns PCI bridge windows, e.g., for root ports > leading to ExpressCard slots. I assume BIOSes make similar > assumptions there about what ExpressCards are likely to be plugged in. > Is your BIOS assistance the same sort of thing, or is there something > additional happening at hot-add time? (I think there might be AML > that does Thunderbolt-specific stuff at hotplug-time, but I assume > that's not related to resource assignment, because the OS owns those > resources.) Yes, if I understand it right the BIOS gets SCI on hotplug, then it enumerates and configures the devices, and finally sends an ACPI bus check event to the OS. > > In order to support this we must make sure that the kernel does not try to > > be too smart and resize / open the bridge windows during PCI enumeration. > > For example by default the kernel will add certain amount of space to the > > bridge memory/io windows (this is configurable via pci=hp[mem|io]size=xxx > > command line option). Eventually we run out of space that the BIOS has > > allocated. > > ROMs usually aren't very big compared to regular memory BARs. Is the > problem just that adding space the BIOS didn't plan for causes the OS > to increase the window size and bump into space assigned to a peer of > the Thunderbolt controller? You are right. I did some more investigation on this and the BIOS seems to close the bridge windows if there are no devices behind the bridge or the device is not using certain type of resource (io/pmem). However, Linux then finds out that the bridge supports these optional windows (in pci_bridge_check_ranges()) and because is_hotplug_bridge == 1, it adds the default sizes to the bridge window resources causing this: [ 15.753262] pci 0000:07:06.0: scanning [bus 80-80] behind bridge, pass 0 [ 15.754458] pci_bus 0000:80: scanning bus [ 15.755563] pci_bus 0000:80: fixups for bus [ 15.756668] pci 0000:07:06.0: PCI bridge to [bus 80] ... [ 15.873433] pci 0000:07:06.0: BAR 7: can't assign io (size 0x1000) [ 15.874542] pci 0000:07:06.0: BAR 8: can't assign mem (size 0x200000) [ 15.875632] pci 0000:07:06.0: BAR 9: can't assign mem pref (size 0x200000) The above bridge has all the windows closed by the BIOS. If I have "pci=hpmemsize=0,hpiosize=0" in the kernel command line this works. > > Also address space for expansion ROMs should not be allocated (BIOS does > > not execute them for Thunderbolt endpoints). If we don't prevent this the > > kernel might find expansion ROM associated with some endpoint and reopen > > the bridge window which the BIOS already closed leading again resource > > exhaustion. > > I assume the only reason we should not allocate expansion ROM space is > to avoid resource exhaustion. If we had enough resources, allocating > ROM space wouldn't cause anything bad to happen, would it? Right. > > Fix this by adding a quirk that matches known Thunderbolt PCI-to-PCI > > bridges and in that case prevents allocation of expansion ROM resources and > > makes sure that the PCI core does not increase size of bridge windows. > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > arch/x86/pci/fixup.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c > > index f5809fa..924822b 100644 > > --- a/arch/x86/pci/fixup.c > > +++ b/arch/x86/pci/fixup.c > > @@ -7,6 +7,8 @@ > > #include <linux/pci.h> > > #include <linux/init.h> > > #include <linux/vgaarb.h> > > +#include <linux/acpi.h> > > +#include <linux/pci-acpi.h> > > #include <asm/pci_x86.h> > > > > static void pci_fixup_i450nx(struct pci_dev *d) > > @@ -539,3 +541,52 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev) > > } > > } > > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone); > > + > > +#ifdef CONFIG_ACPI > > +/* > > + * BIOS assisted Thunderbolt PCI enumeration should handle all resource > > + * allocation on behalf of OS. > > + */ > > +static void quirk_thunderbolt(struct pci_dev *dev) > > +{ > > + struct acpi_pci_root *root; > > + acpi_handle handle; > > + > > + handle = acpi_find_root_bridge_handle(dev); > > + if (!handle) > > + return; > > + > > + root = acpi_pci_find_root(handle); > > + if (!root) > > + return; > > + > > + /* > > + * Native PCIe hotplug should be disabled when BIOS assisted > > + * hotplug is in use. > > + */ > > + if (root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL) > > + return; > > I'm not really sure why this test is here. I dimly recall hearing > that Thunderbolt hotplug requires some work at hotplug-time, and this > is not all public, and hence is done by AML. But that in itself > doesn't seem related to the question of allocating ROM space. The check is here because we need to check that the native PCIe hotplug is disabled by the BIOS. Thunderbolt supports standard PCIe hotplug but some operating systems (not including Linux) can't handle that properly so BIOS will do that on behalf of the OS. It is possible that some systems use native PCIe hotplug (although we haven't seen such yet). Hence we need to check it here and not apply the quirk in that case. > > + /* > > + * Make sure that we don't allocate resources for expansion ROMs. > > + * This may accidentally increase the size of the bridge window > > + * causing us to run out of resources. > > + */ > > + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { > > + pr_info("Thunderbolt host router detected disabling ROMs\n"); > > We have a pci_dev, so this should use dev_info(). OK. > > + pci_probe |= PCI_NOASSIGN_ROMS; > > I think you really only care about the space for ROMs on devices > connected via Thunderbolt, so it's kind of a shame that the switch is > global and affects ROMs on *all* devices. I guess there's nothing > simple to be done about that, though. Well, now that I understand this a bit better, I think we don't need to set the PCI_NOASSIGN_ROMS anymore... > > + } > > + > > + /* > > + * Don't add anything to the BIOS allocated bridge window size for > > + * the same reason. > > + */ > > + dev->is_hotplug_bridge = 0; ...nor do this I think that we can get this working so that we add a new flag to struct pci_dev, something like 'no_additional_hotplug_bus_space' and in this quirk set that. Then in __pci_bus_size_bridges() we do: pci_bridge_check_ranges(bus); if (bus->self->is_hotplug_bridge && !bus->self->no_additional_hotplug_bus_space) { additional_io_size = pci_hotplug_io_size; additional_mem_size = pci_hotplug_mem_size; } This should prevent the problem this patch was trying to solve. Does that work for you? Of course, once we do that the user is not able to change the defaults for Thunderbolt PCI bridges anymore by passing new values from the kernel command line. Not sure if it is needed, though. -- 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
On Wed, Jun 26, 2013 at 03:26:59PM -0700, Yinghai Lu wrote: > On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > >> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. > >> This means that the BIOS will allocate bridge resources based on some > >> assumptions of a maximum Thunderbolt chain. It also disables native PCIe > >> hotplug of the root port where the Thunderbolt host router is connected. > > We should not need tricks in this patch after > > https://patchwork.kernel.org/patch/2766521/ > > [2/3] PCI / ACPI: Use boot-time resource allocation rules during hotplug Unfortunately that patch is not enough :-( We still need to make sure that we don't add anything to the bridge window sizes like __pci_bus_size_bridges() is currently doing (e.g similar to pci=hpmemsize=0,hpiosize=0 for the Thunderbolt bridges). -- 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
On Wed, Jun 26, 2013 at 5:56 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jun 26, 2013 at 3:55 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Wed, Jun 26, 2013 at 4:31 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>> On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>>>> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >>>>> <mika.westerberg@linux.intel.com> wrote: >>>>>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >>>>>> This means that the BIOS will allocate bridge resources based on some >>>>>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >>>>>> hotplug of the root port where the Thunderbolt host router is connected. >>> ... >>> During acpi hotplug, firmare could do extra help for us like assign >>> some resources to pci device bars, so it is NOT "boot-time". >> >> Really? How can firmware assign BARs at hotplug-time? I mean, >> obviously firmware *can* write things to the BARs before giving the >> device to the OS, but how would it know what to write? > > should be acpi code, or SMI code or even BMC firmware via sideband. How would that code (ACPI code (by which I assume you mean AML), SMI code, BMC firmware) know what values to write to BARs? Is it reading the windows of upstream bridges from config space? Is it assuming the OS never changes the windows of bridges upstream from the Thunderbolt controller? >> I assume the >> OS owns the address space, and it can change the upstream bridge >> windows or the BARs of another device on the bus at any time, subject >> to the OS's own issues as far as quiescing or unbinding drivers, etc., >> but without coordinating with the BIOS. > > for thunderbolt or dock with acpiphp, then all children devices/bridges should > not have drivers loaded yet. I said "upstream bridge windows or ... another device on the bus," i.e., I'm referring to devices other than the Thunderbolt controller. I assume the OS is free to change resource assignments for those non-Thunderbolt devices, and obviously those assignments affect what is available for Thunderbolt. 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
On Thu, Jun 27, 2013 at 04:54:05PM +0300, Mika Westerberg wrote: > I think that we can get this working so that we add a new flag to struct > pci_dev, something like 'no_additional_hotplug_bus_space' and in this quirk > set that. > > Then in __pci_bus_size_bridges() we do: > > pci_bridge_check_ranges(bus); > if (bus->self->is_hotplug_bridge && > !bus->self->no_additional_hotplug_bus_space) { > additional_io_size = pci_hotplug_io_size; > additional_mem_size = pci_hotplug_mem_size; > } > > This should prevent the problem this patch was trying to solve. Does that > work for you? Forget about that -- It looks like these messages are harmless: pcieport 0000:0a:05.0: BAR 8: can't assign mem (size 0x200000) pcieport 0000:0a:05.0: BAR 9: can't assign mem pref (size 0x200000) It just means that we tried to allocate the resource but failed because the bridge has that window closed, if I got it right. If user doesn't want to see those, he/she can always pass 'pci=hpmensize=0,hpiosize=0' in the kernel command line. With pcibios_resource_survey_bus() fix, looks like this quirk is not needed at all. -- 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
On Thu, Jun 27, 2013 at 9:27 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Thu, Jun 27, 2013 at 04:54:05PM +0300, Mika Westerberg wrote: >> I think that we can get this working so that we add a new flag to struct >> pci_dev, something like 'no_additional_hotplug_bus_space' and in this quirk >> set that. >> >> Then in __pci_bus_size_bridges() we do: >> >> pci_bridge_check_ranges(bus); >> if (bus->self->is_hotplug_bridge && >> !bus->self->no_additional_hotplug_bus_space) { >> additional_io_size = pci_hotplug_io_size; >> additional_mem_size = pci_hotplug_mem_size; >> } >> >> This should prevent the problem this patch was trying to solve. Does that >> work for you? > > Forget about that -- It looks like these messages are harmless: > > pcieport 0000:0a:05.0: BAR 8: can't assign mem (size 0x200000) > pcieport 0000:0a:05.0: BAR 9: can't assign mem pref (size 0x200000) > > It just means that we tried to allocate the resource but failed because the > bridge has that window closed, if I got it right. If user doesn't want to > see those, he/she can always pass 'pci=hpmensize=0,hpiosize=0' in the > kernel command line. Yes, that extra range for hotplug is optional, so if must+optional fails, second try will be must only. > > With pcibios_resource_survey_bus() fix, looks like this quirk is not needed > at all. Good. -- 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
On Thu, Jun 27, 2013 at 9:00 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Wed, Jun 26, 2013 at 5:56 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Wed, Jun 26, 2013 at 3:55 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Wed, Jun 26, 2013 at 4:31 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> On Wed, Jun 26, 2013 at 3:26 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>>>> On Wed, Jun 26, 2013 at 3:18 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>>>>> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg >>>>>> <mika.westerberg@linux.intel.com> wrote: >>>>>>> Thunderbolt PCI-to-PCI bridges typically use BIOS "assisted" enumeration. >>>>>>> This means that the BIOS will allocate bridge resources based on some >>>>>>> assumptions of a maximum Thunderbolt chain. It also disables native PCIe >>>>>>> hotplug of the root port where the Thunderbolt host router is connected. >>>> ... >>>> During acpi hotplug, firmare could do extra help for us like assign >>>> some resources to pci device bars, so it is NOT "boot-time". >>> >>> Really? How can firmware assign BARs at hotplug-time? I mean, >>> obviously firmware *can* write things to the BARs before giving the >>> device to the OS, but how would it know what to write? >> >> should be acpi code, or SMI code or even BMC firmware via sideband. > > How would that code (ACPI code (by which I assume you mean AML), SMI > code, BMC firmware) know what values to write to BARs? Is it reading > the windows of upstream bridges from config space? Is it assuming the > OS never changes the windows of bridges upstream from the Thunderbolt > controller? Kernel only try to change upstream bridge for pciehp during hot-add. for acpiphp, because could have other devices already there before hot-add, kernel does not try to expand the upstream bridge. size and assign only to the new added devices. > >>> I assume the >>> OS owns the address space, and it can change the upstream bridge >>> windows or the BARs of another device on the bus at any time, subject >>> to the OS's own issues as far as quiescing or unbinding drivers, etc., >>> but without coordinating with the BIOS. >> >> for thunderbolt or dock with acpiphp, then all children devices/bridges should >> not have drivers loaded yet. > > I said "upstream bridge windows or ... another device on the bus," > i.e., I'm referring to devices other than the Thunderbolt controller. > I assume the OS is free to change resource assignments for those > non-Thunderbolt devices, and obviously those assignments affect what > is available for Thunderbolt. ok. that upstream one can not be touched from firmware. -- 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 --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c index f5809fa..924822b 100644 --- a/arch/x86/pci/fixup.c +++ b/arch/x86/pci/fixup.c @@ -7,6 +7,8 @@ #include <linux/pci.h> #include <linux/init.h> #include <linux/vgaarb.h> +#include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <asm/pci_x86.h> static void pci_fixup_i450nx(struct pci_dev *d) @@ -539,3 +541,52 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev) } } DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone); + +#ifdef CONFIG_ACPI +/* + * BIOS assisted Thunderbolt PCI enumeration should handle all resource + * allocation on behalf of OS. + */ +static void quirk_thunderbolt(struct pci_dev *dev) +{ + struct acpi_pci_root *root; + acpi_handle handle; + + handle = acpi_find_root_bridge_handle(dev); + if (!handle) + return; + + root = acpi_pci_find_root(handle); + if (!root) + return; + + /* + * Native PCIe hotplug should be disabled when BIOS assisted + * hotplug is in use. + */ + if (root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL) + return; + + /* + * Make sure that we don't allocate resources for expansion ROMs. + * This may accidentally increase the size of the bridge window + * causing us to run out of resources. + */ + if (!(pci_probe & PCI_NOASSIGN_ROMS)) { + pr_info("Thunderbolt host router detected disabling ROMs\n"); + pci_probe |= PCI_NOASSIGN_ROMS; + } + + /* + * Don't add anything to the BIOS allocated bridge window size for + * the same reason. + */ + dev->is_hotplug_bridge = 0; +} +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1513, quirk_thunderbolt); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x151a, quirk_thunderbolt); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x151b, quirk_thunderbolt); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1547, quirk_thunderbolt); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1548, quirk_thunderbolt); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1549, quirk_thunderbolt); +#endif