From: Bjorn Helgaas <helgaas@kernel.org>
To: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linux@yadro.com" <linux@yadro.com>, "sr@denx.de" <sr@denx.de>
Subject: Re: [PATCH v7 17/26] PCI: hotplug: Ignore the MEM BAR offsets from BIOS/bootloader
Date: Wed, 5 Feb 2020 10:42:56 -0600 [thread overview]
Message-ID: <20200205164255.GA205474@google.com> (raw)
In-Reply-To: <8d3dbb43dfc668b6e436fbcc78d63f14c88a056f.camel@yadro.com>
On Wed, Feb 05, 2020 at 11:01:32AM +0000, Sergei Miroshnichenko wrote:
> On Fri, 2020-01-31 at 14:31 -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 29, 2020 at 06:29:28PM +0300, Sergei Miroshnichenko
> > wrote:
> > > BAR allocation by BIOS/UEFI/bootloader/firmware may be
> > > non-optimal and it may even clash with the kernel's BAR
> > > assignment algorithm.
> > >
> > > For example, sometimes BIOS doesn't reserve space for SR-IOV
> > > BARs, and this bridge window can neither extend (blocked by
> > > immovable BARs) nor move (the device itself is immovable).
> > >
> > > With this patch the kernel will use its own methods of BAR
> > > allocating when possible, increasing the chances of successful
> > > boot and hotplug.
> > >
> > > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
> > > ---
> > > drivers/pci/probe.c | 16 ++++++++++++++--
> > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index bb584038d5b4..f8f643dac6d1 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -306,6 +306,14 @@ int __pci_read_base(struct pci_dev *dev, enum
> > > pci_bar_type type,
> > > pos, (unsigned long long)region.start);
> > > }
> > >
> > > + if (pci_can_move_bars && res->start && !(res->flags &
> > > IORESOURCE_IO)) {
> > > + pci_warn(dev, "ignore the current offset of BAR %llx-
> > > %llx\n",
> > > + l64, l64 + sz64 - 1);
> > > + res->start = 0;
> > > + res->end = sz64 - 1;
> > > + res->flags |= IORESOURCE_SIZEALIGN;
> > > + }
> > > +
> > > goto out;
> > >
> > >
> > > @@ -528,8 +536,10 @@ void pci_read_bridge_bases(struct pci_bus
> > > *child)
> > > child->resource[i] = &dev-
> > > >resource[PCI_BRIDGE_RESOURCES+i];
> > >
> > > pci_read_bridge_io(child);
> > > - pci_read_bridge_mmio(child);
> > > - pci_read_bridge_mmio_pref(child);
> > > + if (!pci_can_move_bars) {
> > > + pci_read_bridge_mmio(child);
> > > + pci_read_bridge_mmio_pref(child);
> > > + }
> >
> > I mentioned this in another response, but I'll repeat it here to
> > comment on the code directly: I don't think we should have feature
> > tests like this "!pci_can_move_bars" scattered around, and I don't
> > want basic behaviors like reading bridge windows during enumeration
> > to
> > depend on it.
> >
> > There's no obvious reason why we should ignore bridge windows if we
> > support movable BARs.
>
> That patch solves a problem which is non-fatal during boot, but is
> breaking this whole patchset when trying a PCI rescan. On a specific
> machine we have a popular i350 network card:
>
> $ lspci -tv
> -[0000:00]-...
> +-01.1-[02]--+-00.0 Intel Corporation I350 Gigabit Network
> Connection
> | +-00.1 Intel Corporation I350 Gigabit Network
> Connection
> | +-00.2 Intel Corporation I350 Gigabit Network
> Connection
> | \-00.3 Intel Corporation I350 Gigabit Network
> Connection
>
> On the "ROG STRIX Z370-F GAMING, BIOS 0612 03/01/2018" motherboard and
> vanilla kernel, not every BAR is allocated:
>
> $ dmesg -t
> ...
> pci 0000:00:01.0: PCI bridge to [bus 01]
> pci 0000:00:01.0: bridge window [mem 0xf7700000-0xf78fffff]
> pci 0000:02:00.0: BAR 7: assigned [mem 0xf7490000-0xf74affff]
> pci 0000:02:00.0: BAR 10: assigned [mem 0xf74b0000-0xf74cffff]
> pci 0000:02:00.1: BAR 7: assigned [mem 0xf74d0000-0xf74effff]
> pci 0000:02:00.1: BAR 10: no space for [mem size 0x00020000]
> pci 0000:02:00.1: BAR 10: failed to assign [mem size 0x00020000]
> pci 0000:02:00.2: BAR 7: no space for [mem size 0x00020000]
> pci 0000:02:00.2: BAR 7: failed to assign [mem size 0x00020000]
> pci 0000:02:00.2: BAR 10: no space for [mem size 0x00020000]
> pci 0000:02:00.2: BAR 10: failed to assign [mem size 0x00020000]
> pci 0000:02:00.3: BAR 7: no space for [mem size 0x00020000]
> pci 0000:02:00.3: BAR 7: failed to assign [mem size 0x00020000]
> pci 0000:02:00.3: BAR 10: no space for [mem size 0x00020000]
> pci 0000:02:00.3: BAR 10: failed to assign [mem size 0x00020000]
> pci 0000:00:01.1: PCI bridge to [bus 02]
>
> $ sudo cat /proc/iomem
> ...
> f7000000-f74fffff : PCI Bus 0000:02
> f7000000-f70fffff : 0000:02:00.3
> f7000000-f70fffff : igb
> f7100000-f71fffff : 0000:02:00.2
> f7100000-f71fffff : igb
> f7200000-f72fffff : 0000:02:00.1
> f7200000-f72fffff : igb
> f7300000-f73fffff : 0000:02:00.0
> f7300000-f73fffff : igb
> f7400000-f747ffff : 0000:02:00.0
> f7480000-f7483fff : 0000:02:00.3
> f7480000-f7483fff : igb
> f7484000-f7487fff : 0000:02:00.2
> f7484000-f7487fff : igb
> f7488000-f748bfff : 0000:02:00.1
> f7488000-f748bfff : igb
> f748c000-f748ffff : 0000:02:00.0
> f748c000-f748ffff : igb
> f7490000-f74affff : 0000:02:00.0
> f74b0000-f74cffff : 0000:02:00.0
> f74d0000-f74effff : 0000:02:00.1
>
> But when allowing the kernel to allocate BARs properly, the map is
> full:
>
> c8200000-c87fffff : PCI Bus 0000:02
> c8200000-c82fffff : 0000:02:00.0
> c8200000-c82fffff : igb
> c8300000-c83fffff : 0000:02:00.1
> c8300000-c83fffff : igb
> c8400000-c84fffff : 0000:02:00.2
> c8400000-c84fffff : igb
> c8500000-c85fffff : 0000:02:00.3
> c8500000-c85fffff : igb
> c8600000-c867ffff : 0000:02:00.0
> c8680000-c8683fff : 0000:02:00.0
> c8680000-c8683fff : igb
> c8684000-c86a3fff : 0000:02:00.0
> c86a4000-c86c3fff : 0000:02:00.0
> c86c4000-c86c7fff : 0000:02:00.1
> c86c4000-c86c7fff : igb
> c86c8000-c86e7fff : 0000:02:00.1
> c86e8000-c8707fff : 0000:02:00.1
> c8708000-c870bfff : 0000:02:00.2
> c8708000-c870bfff : igb
> c870c000-c872bfff : 0000:02:00.2
> c872c000-c874bfff : 0000:02:00.2
> c874c000-c874ffff : 0000:02:00.3
> c874c000-c874ffff : igb
> c8750000-c876ffff : 0000:02:00.3
> c8770000-c878ffff : 0000:02:00.3
>
> In this particular case the "repaired" BARs are not vital and are not
> used by the igb driver, but in general such behavior of BIOS can lead
> to a non-working setup.
>
> So ignoring pre-set BARs and bridge windows may be useful on its own,
> but it is also provides a working starting point required by this
> patchset, otherwise it will need to track such BARs impossible to
> assign, and don't try to assign them during a next PCI rescan.
>
> The reason I've tied this feature to the "movable BARs" flag is that I
> know at least one exception demanding a workaround - VGA. So I wanted
> to provide a flag to disable it in case of other unforeseen
> consequences, and the only feature depends on this - is movable BARs.
If we need exceptions for broken or legacy devices, we should check
for those explicitly.
If we fail to assign some BARs at boot, I think it's reasonable to try
to reassign things before drivers claim the devices. But support for
that should be in a reassignment path, not in the general enumeration
path. And, since drivers aren't involved yet, it probably doesn't
even depend on pci_can_move_bars.
> The [07/26] "PCI: hotplug: Don't allow hot-added devices to steal
> resources" patch introduces an additional step in BAR assignment:
> - try to assign every existing BAR + BARs of the hot-added device;
> - it if fails, disable BARs for the hot-added device and retry without
> them.
>
> A possible way to work-around non-working BARs could be adding more
> steps:
> - first try to assign every existing BAR + BARs not worked previously
> + "hot-added" BARs;
> - if it fails, retry without those BARs which weren't working before;
> - if it still fails, retry without "hot-added" BARs.
>
> Best regards,
> Serge
>
> > > if (dev->transparent) {
> > > pci_bus_for_each_resource(child->parent, res, i) {
> > > @@ -2945,6 +2955,8 @@ int pci_host_probe(struct pci_host_bridge
> > > *bridge)
> > > pci_bus_claim_resources(bus);
> > > } else {
> > > pci_bus_size_bridges(bus);
> > > + if (pci_can_move_bars)
> > > + pci_bus_update_realloc_range(bus);
> > > pci_bus_assign_resources(bus);
> > >
> > > list_for_each_entry(child, &bus->children, node)
> > > --
> > > 2.24.1
> > >
next prev parent reply other threads:[~2020-02-05 16:43 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-29 15:29 [PATCH v7 00/26] PCI: Allow BAR movement during boot and hotplug Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 01/26] PCI: Fix race condition in pci_enable/disable_device() Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 02/26] PCI: Enable bridge's I/O and MEM access for hotplugged devices Sergei Miroshnichenko
2020-01-30 23:12 ` Bjorn Helgaas
2020-01-29 15:29 ` [PATCH v7 03/26] PCI: hotplug: Initial support of the movable BARs feature Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 04/26] PCI: Add version of release_child_resources() aware of immovable BARs Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 05/26] PCI: hotplug: Fix reassigning the released BARs Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 06/26] PCI: hotplug: Recalculate every bridge window during rescan Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 07/26] PCI: hotplug: Don't allow hot-added devices to steal resources Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 08/26] PCI: hotplug: Try to reassign movable BARs only once Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 09/26] PCI: hotplug: Calculate immovable parts of bridge windows Sergei Miroshnichenko
2020-01-30 21:26 ` Bjorn Helgaas
2020-01-31 16:59 ` Sergei Miroshnichenko
2020-01-31 20:10 ` Bjorn Helgaas
2020-01-29 15:29 ` [PATCH v7 10/26] PCI: Include fixed and immovable BARs into the bus size calculating Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 11/26] PCI: hotplug: movable BARs: Compute limits for relocated bridge windows Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 12/26] PCI: Make sure bridge windows include their fixed BARs Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 13/26] PCI: hotplug: Add support of immovable BARs to pci_assign_resource() Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 14/26] PCI: hotplug: Sort immovable BARs before assignment Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 15/26] PCI: hotplug: Enable the movable BARs feature by default Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 16/26] PCI: Ignore PCIBIOS_MIN_MEM Sergei Miroshnichenko
2020-01-30 23:52 ` Bjorn Helgaas
2020-01-31 18:19 ` Sergei Miroshnichenko
2020-01-31 20:27 ` Bjorn Helgaas
2020-02-05 13:04 ` Sergei Miroshnichenko
2020-02-05 16:32 ` Bjorn Helgaas
2020-02-12 12:16 ` Sergei Miroshnichenko
2020-02-12 14:13 ` Bjorn Helgaas
2020-01-29 15:29 ` [PATCH v7 17/26] PCI: hotplug: Ignore the MEM BAR offsets from BIOS/bootloader Sergei Miroshnichenko
2020-01-31 20:31 ` Bjorn Helgaas
2020-02-05 11:01 ` Sergei Miroshnichenko
2020-02-05 16:42 ` Bjorn Helgaas [this message]
2020-02-12 12:29 ` Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 18/26] PCI: Treat VGA BARs as immovable Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 19/26] PCI: hotplug: Configure MPS for hot-added bridges during bus rescan Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 20/26] PNP: Don't reserve BARs for PCI when enabled movable BARs Sergei Miroshnichenko
2020-01-30 14:39 ` Rafael J. Wysocki
2020-01-30 21:14 ` Bjorn Helgaas
2020-01-31 15:48 ` Sergei Miroshnichenko
2020-01-31 19:39 ` Bjorn Helgaas
2020-01-29 15:29 ` [PATCH v7 21/26] PCI: hotplug: Don't disable the released bridge windows immediately Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 22/26] PCI: pciehp: Trigger a domain rescan on hp events when enabled movable BARs Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 23/26] PCI: Don't claim immovable BARs Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 24/26] PCI: hotplug: Don't reserve bus space when enabled movable BARs Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 25/26] nvme-pci: Handle " Sergei Miroshnichenko
2020-01-29 15:29 ` Sergei Miroshnichenko
2020-01-29 15:29 ` [PATCH v7 26/26] PCI/portdrv: Declare support of " Sergei Miroshnichenko
2020-01-30 23:37 ` [PATCH v7 00/26] PCI: Allow BAR movement during boot and hotplug Bjorn Helgaas
2020-02-03 4:56 ` Oliver O'Halloran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200205164255.GA205474@google.com \
--to=helgaas@kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@yadro.com \
--cc=s.miroshnichenko@yadro.com \
--cc=sr@denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.