Re: [PATCH] pci: acpiphp: try to reassign resources on bridge if necessary

[Igor Mammedov]

On Mon, 24 Apr 2023 20:50:14 +0200
Igor Mammedov <imammedo@xxxxxxxxxx> wrote:

> On Tue, 18 Apr 2023 11:31:14 -0500
> Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> 
> > [+cc Mika, who made previous changes in this area]
> > 
> > On Tue, Apr 18, 2023 at 05:38:15PM +0200, Rafael J. Wysocki wrote:  
> > > On Tue, Apr 18, 2023 at 4:17 PM Igor Mammedov <imammedo@xxxxxxxxxx> wrote:    
> > > > On Tue, 18 Apr 2023 14:55:29 +0200
> > > > "Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote:    
> > > > > On Tue, Apr 18, 2023 at 10:50 AM Igor Mammedov <imammedo@xxxxxxxxxx> wrote:    
> > > > > >
> > > > > > When using ACPI PCI hotplug, hotplugging a device with
> > > > > > large BARs may fail if bridge windows programmed by
> > > > > > firmware are not large enough.
> > > > > >
> > > > > > Reproducer:
> > > > > >   $ qemu-kvm -monitor stdio -M q35  -m 4G \
> > > > > >       -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=on \
> > > > > >       -device id=rp1,pcie-root-port,bus=pcie.0,chassis=4 \
> > > > > >       disk_image
> > > > > >
> > > > > >  wait till linux guest boots, then hotplug device
> > > > > >    (qemu) device_add qxl,bus=rp1
> > > > > >
> > > > > >  hotplug on guest side fails with:
> > > > > >    pci 0000:01:00.0: [1b36:0100] type 00 class 0x038000
> > > > > >    pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x03ffffff]
> > > > > >    pci 0000:01:00.0: reg 0x14: [mem 0x00000000-0x03ffffff]
> > > > > >    pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x00001fff]
> > > > > >    pci 0000:01:00.0: reg 0x1c: [io  0x0000-0x001f]
> > > > > >    pci 0000:01:00.0: BAR 0: no space for [mem size 0x04000000]
> > > > > >    pci 0000:01:00.0: BAR 0: failed to assign [mem size 0x04000000]
> > > > > >    pci 0000:01:00.0: BAR 1: no space for [mem size 0x04000000]
> > > > > >    pci 0000:01:00.0: BAR 1: failed to assign [mem size 0x04000000]
> > > > > >    pci 0000:01:00.0: BAR 2: assigned [mem 0xfe800000-0xfe801fff]
> > > > > >    pci 0000:01:00.0: BAR 3: assigned [io  0x1000-0x101f]
> > > > > >    qxl 0000:01:00.0: enabling device (0000 -> 0003)
> > > > > >    Unable to create vram_mapping
> > > > > >    qxl: probe of 0000:01:00.0 failed with error -12
> > > > > >
> > > > > > However when using native PCIe hotplug
> > > > > >   '-global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off'
> > > > > > it works fine, since kernel attempts to reassign unused resources.
> > > > > > Use the same machinery as native PCIe hotplug to (re)assign resources.    
> > 
> > Thanks for the nice reproducer and logs!
> >   
> > > > > > Signed-off-by: Igor Mammedov <imammedo@xxxxxxxxxx>    
> > > > >
> > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > >
> > > > > or please let me know if you want me to pick this up.    
> > > >
> > > > It would be nice if you could pick it up.    
> > > 
> > > OK, I'll do that unless Bjorn tells me that he prefers to take it via
> > > the PCI tree.    
> > 
> > It's OK with me if you pick this up, but please update the subject to
> > use the style of previous commits, e.g.,
> > 
> >   PCI: acpiphp: Reassign resources on bridge if necessary
> > 
> > Previous changes involving pci_assign_unassigned_bridge_resources() in
> > enable_slot() (these are from Mika, so I cc'd him in case he wants to
> > comment):
> > 
> >   84c8b58ed3ad ("ACPI / hotplug / PCI: Don't scan bridges managed by native hotplug")
> >   77adf9355304 ("ACPI / hotplug / PCI: Allocate resources directly under the non-hotplug bridge")
> >   
> > > > > > ---
> > > > > > tested in QEMU with Q35 machine on PCIE root port and also
> > > > > > with nested conventional bridge attached to root port.
> > > > > > ---
> > > > > >  drivers/pci/hotplug/acpiphp_glue.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > > > > > index 5b1f271c6034..9aebde28a92f 100644
> > > > > > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > > > > > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > > > > > @@ -517,7 +517,7 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)    
> > 
> > Previous context:
> > 
> >                                              __pci_bus_size_bridges(dev->subordinate,
> >                                                                     &add_list);
> >   
> > > > > >                                 }
> > > > > >                         }
> > > > > >                 }
> > > > > > -               __pci_bus_assign_resources(bus, &add_list, NULL);
> > > > > > +               pci_assign_unassigned_bridge_resources(bus->self);    
> > 
> > "add_list" is now used only for __pci_bus_size_bridges(), which
> > *looks* unnecessary unless there's some obscure side-effect of that
> > path when that parameter is non-NULL.
> > 
> > If "add_list" is unnecessary, you would probably use
> > pci_bus_size_bridges() above instead of __pci_bus_size_bridges().  
> 
> pci_assign_unassigned_bridge_resources() calls __pci_bus_size_bridges()
> so original one is not needed anymore, in addition it might leak entries
> added to add_list.
> I'll remove __pci_bus_size_bridges() and respin patch (incl. updated subject)
> 
>  
> > After this patch, we have:
> > 
> >   if (bridge && bus->self && hotplug_is_native(bus->self)) {
> >     for_each_pci_bridge(dev, bus)
> >       acpiphp_native_scan_bridge(dev);
> >   } else {
> >     ...
> >     pci_assign_unassigned_bridge_resources(bus->self);
> >   }
> > 
> > We do not do pci_assign_unassigned_bridge_resources() in the "then"
> > part of the "if".  Per the comment, that case may be used for adding
> > Thunderbolt controllers.  Is there a reason we do not want
> > pci_assign_unassigned_bridge_resources() in that path,
> > or should it be
> > in both cases?  
> acpiphp_native_scan_bridge() looks very similar to 'else'
> branch modulo skip native hp bridge condition.
> Otherwise both branches look similar, 
> but I don't have means to test that usecase,
> so I'd avoid touching something nobody complains about.

I gave some more testing to the case with hotplugged sub-bridges,
and this patch doesn't help much with that, meaning that
pci_assign_unassigned_bridge_resources() when re-sizing goes
only to parent for extra resources. So case:
   1. hotplug SHPC bridge first & then hotplug a device into
      hotplugged SHPC bridge, may still fail as SHPC calling
      its own pci_assign_unassigned_bridge_resources(), will reach
      only to its parent (root port) which in turn might not have enough
      resources.
   2. hotplugging compound bridge+device attached to it in one go,
      works as expected since pci_assign_unassigned_bridge_resources()
      on root port accounts for all needed resources and asks for them
      from host-bridge.
 
So basically patch fixes reassignment in case of hotplug into root port
(it brings acpiphp on root port on par with native hotplug).

It doesn't work for nested bridges, but the same applies to native
hotplug as well.
Perhaps we should make pci_assign_unassigned_bridge_resources() ask for
more resources all the way down to host bridge (crude but might be sufficient).

PS:
I've found an attempt to make reassignment work properly (dating to 2012)
https://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git/log/
but it doesn't look like it's been merged.

> > > > > >         }
> > > > > >
> > > > > >         acpiphp_sanitize_bus(bus);    
> >   
>