From: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
To: "helgaas@kernel.org" <helgaas@kernel.org>
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, 12 Feb 2020 12:29:50 +0000 [thread overview]
Message-ID: <602f752c774378bccd6f2d0a47e3a86e8151bdd9.camel@yadro.com> (raw)
In-Reply-To: <20200205164255.GA205474@google.com>
On Wed, 2020-02-05 at 10:42 -0600, Bjorn Helgaas wrote:
> 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.
>
Thanks for the advice, it worked! Now I'm validating in the end of
__init pci_assign_unassigned_resources() that every device got desired
BARs, otherwise call to pci_rescan_bus(), which is allowed to move BARs
- indeed not yet captured by drivers at this stage. This will be
submitted in v8.
Serge
> > 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-12 12:29 UTC|newest]
Thread overview: 48+ 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
2020-02-12 12:29 ` Sergei Miroshnichenko [this message]
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 ` [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=602f752c774378bccd6f2d0a47e3a86e8151bdd9.camel@yadro.com \
--to=s.miroshnichenko@yadro.com \
--cc=helgaas@kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).