Re: [PATCH v7 17/26] PCI: hotplug: Ignore the MEM BAR offsets from BIOS/bootloader

[Sergei Miroshnichenko <s.miroshnichenko@yadro.com>]

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.

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