diff mbox series

[v2] PCI: acpiphp: Reassign resources on bridge if necessary

Message ID 20230424191557.2464760-1-imammedo@redhat.com
State New
Headers show
Series [v2] PCI: acpiphp: Reassign resources on bridge if necessary | expand

Commit Message

Igor Mammedov April 24, 2023, 7:15 p.m. UTC
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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
tested in QEMU with Q35 machine on PCIE root port and also
with nested conventional bridge attached to root port.

v2:
  * fixup subject to match expected style
  * drop no longer needed __pci_bus_size_bridges() to avoid
    memory leak (Bjorn Helgaas <helgaas@kernel.org>)
---
 drivers/pci/hotplug/acpiphp_glue.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Michael S. Tsirkin April 24, 2023, 8:47 p.m. UTC | #1
On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> tested in QEMU with Q35 machine on PCIE root port and also
> with nested conventional bridge attached to root port.
> 
> v2:
>   * fixup subject to match expected style
>   * drop no longer needed __pci_bus_size_bridges() to avoid
>     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..328d1e416014 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				acpiphp_native_scan_bridge(dev);
>  		}
>  	} else {
> -		LIST_HEAD(add_list);
>  		int max, pass;
>  
>  		acpiphp_rescan_slot(slot);
> @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				if (pass && dev->subordinate) {
>  					check_hotplug_bridge(slot, dev);
>  					pcibios_resource_survey_bus(dev->subordinate);
> -					__pci_bus_size_bridges(dev->subordinate,
> -							       &add_list);
>  				}
>  			}
>  		}
> -		__pci_bus_assign_resources(bus, &add_list, NULL);
> +		pci_assign_unassigned_bridge_resources(bus->self);
>  	}
>  
>  	acpiphp_sanitize_bus(bus);
> -- 
> 2.39.1
Igor Mammedov May 9, 2023, 12:44 p.m. UTC | #2
On Mon, 24 Apr 2023 21:15:57 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

Bjorn,

I've fixed up issues you've mentioned on v1 review,
is it good enough to take it in now?

> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> tested in QEMU with Q35 machine on PCIE root port and also
> with nested conventional bridge attached to root port.
> 
> v2:
>   * fixup subject to match expected style
>   * drop no longer needed __pci_bus_size_bridges() to avoid
>     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..328d1e416014 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				acpiphp_native_scan_bridge(dev);
>  		}
>  	} else {
> -		LIST_HEAD(add_list);
>  		int max, pass;
>  
>  		acpiphp_rescan_slot(slot);
> @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				if (pass && dev->subordinate) {
>  					check_hotplug_bridge(slot, dev);
>  					pcibios_resource_survey_bus(dev->subordinate);
> -					__pci_bus_size_bridges(dev->subordinate,
> -							       &add_list);
>  				}
>  			}
>  		}
> -		__pci_bus_assign_resources(bus, &add_list, NULL);
> +		pci_assign_unassigned_bridge_resources(bus->self);
>  	}
>  
>  	acpiphp_sanitize_bus(bus);
Igor Mammedov May 22, 2023, 11:06 a.m. UTC | #3
On Mon, 24 Apr 2023 21:15:57 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> When using ACPI PCI hotplug, hotplugging a device with
> large BARs may fail if bridge windows programmed by
> firmware are not large enough.

Rafael,

Since there was no more comments for a while,
can you merge this patch through your tree?

> 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> tested in QEMU with Q35 machine on PCIE root port and also
> with nested conventional bridge attached to root port.
> 
> v2:
>   * fixup subject to match expected style
>   * drop no longer needed __pci_bus_size_bridges() to avoid
>     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..328d1e416014 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				acpiphp_native_scan_bridge(dev);
>  		}
>  	} else {
> -		LIST_HEAD(add_list);
>  		int max, pass;
>  
>  		acpiphp_rescan_slot(slot);
> @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				if (pass && dev->subordinate) {
>  					check_hotplug_bridge(slot, dev);
>  					pcibios_resource_survey_bus(dev->subordinate);
> -					__pci_bus_size_bridges(dev->subordinate,
> -							       &add_list);
>  				}
>  			}
>  		}
> -		__pci_bus_assign_resources(bus, &add_list, NULL);
> +		pci_assign_unassigned_bridge_resources(bus->self);
>  	}
>  
>  	acpiphp_sanitize_bus(bus);
Rafael J. Wysocki May 22, 2023, 11:52 a.m. UTC | #4
On Mon, May 22, 2023 at 1:06 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon, 24 Apr 2023 21:15:57 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> > When using ACPI PCI hotplug, hotplugging a device with
> > large BARs may fail if bridge windows programmed by
> > firmware are not large enough.
>
> Rafael,
>
> Since there was no more comments for a while,
> can you merge this patch through your tree?

I need an ACK from Bjorn on this, as the PCI maintainer, or it can go
in via the PCI tree as far as I'm concerned.

Thanks!

> > 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.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > tested in QEMU with Q35 machine on PCIE root port and also
> > with nested conventional bridge attached to root port.
> >
> > v2:
> >   * fixup subject to match expected style
> >   * drop no longer needed __pci_bus_size_bridges() to avoid
> >     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 5b1f271c6034..328d1e416014 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >                               acpiphp_native_scan_bridge(dev);
> >               }
> >       } else {
> > -             LIST_HEAD(add_list);
> >               int max, pass;
> >
> >               acpiphp_rescan_slot(slot);
> > @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >                               if (pass && dev->subordinate) {
> >                                       check_hotplug_bridge(slot, dev);
> >                                       pcibios_resource_survey_bus(dev->subordinate);
> > -                                     __pci_bus_size_bridges(dev->subordinate,
> > -                                                            &add_list);
> >                               }
> >                       }
> >               }
> > -             __pci_bus_assign_resources(bus, &add_list, NULL);
> > +             pci_assign_unassigned_bridge_resources(bus->self);
> >       }
> >
> >       acpiphp_sanitize_bus(bus);
>
Michael S. Tsirkin May 22, 2023, 7:21 p.m. UTC | #5
On Mon, May 22, 2023 at 01:52:33PM +0200, Rafael J. Wysocki wrote:
> On Mon, May 22, 2023 at 1:06 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Mon, 24 Apr 2023 21:15:57 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > > When using ACPI PCI hotplug, hotplugging a device with
> > > large BARs may fail if bridge windows programmed by
> > > firmware are not large enough.
> >
> > Rafael,
> >
> > Since there was no more comments for a while,
> > can you merge this patch through your tree?
> 
> I need an ACK from Bjorn on this, as the PCI maintainer, or it can go
> in via the PCI tree as far as I'm concerned.
> 
> Thanks!

I'd also like an ACK for:
Cc: stable@vger.kernel.org



> > > 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.
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > tested in QEMU with Q35 machine on PCIE root port and also
> > > with nested conventional bridge attached to root port.
> > >
> > > v2:
> > >   * fixup subject to match expected style
> > >   * drop no longer needed __pci_bus_size_bridges() to avoid
> > >     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> > > ---
> > >  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > > index 5b1f271c6034..328d1e416014 100644
> > > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > > @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> > >                               acpiphp_native_scan_bridge(dev);
> > >               }
> > >       } else {
> > > -             LIST_HEAD(add_list);
> > >               int max, pass;
> > >
> > >               acpiphp_rescan_slot(slot);
> > > @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> > >                               if (pass && dev->subordinate) {
> > >                                       check_hotplug_bridge(slot, dev);
> > >                                       pcibios_resource_survey_bus(dev->subordinate);
> > > -                                     __pci_bus_size_bridges(dev->subordinate,
> > > -                                                            &add_list);
> > >                               }
> > >                       }
> > >               }
> > > -             __pci_bus_assign_resources(bus, &add_list, NULL);
> > > +             pci_assign_unassigned_bridge_resources(bus->self);
> > >       }
> > >
> > >       acpiphp_sanitize_bus(bus);
> >
Bjorn Helgaas May 22, 2023, 9:51 p.m. UTC | #6
On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Tentatively applied to pci/hotplug for v6.5.

Tentative only because I'm hoping for your ack or review, Rafael (I
see you acked v1, but this one is a little different).

And I think you'd like a stable tag, Michael?  I'm happy to add one,
but wasn't sure if you wanted a separate ack for that.

> ---
> tested in QEMU with Q35 machine on PCIE root port and also
> with nested conventional bridge attached to root port.
> 
> v2:
>   * fixup subject to match expected style
>   * drop no longer needed __pci_bus_size_bridges() to avoid
>     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..328d1e416014 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				acpiphp_native_scan_bridge(dev);
>  		}
>  	} else {
> -		LIST_HEAD(add_list);
>  		int max, pass;
>  
>  		acpiphp_rescan_slot(slot);
> @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
>  				if (pass && dev->subordinate) {
>  					check_hotplug_bridge(slot, dev);
>  					pcibios_resource_survey_bus(dev->subordinate);
> -					__pci_bus_size_bridges(dev->subordinate,
> -							       &add_list);
>  				}
>  			}
>  		}
> -		__pci_bus_assign_resources(bus, &add_list, NULL);
> +		pci_assign_unassigned_bridge_resources(bus->self);
>  	}
>  
>  	acpiphp_sanitize_bus(bus);
> -- 
> 2.39.1
>
Michael S. Tsirkin May 22, 2023, 9:54 p.m. UTC | #7
On Mon, May 22, 2023 at 04:51:41PM -0500, Bjorn Helgaas wrote:
> On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Tentatively applied to pci/hotplug for v6.5.
> 
> Tentative only because I'm hoping for your ack or review, Rafael (I
> see you acked v1, but this one is a little different).
> 
> And I think you'd like a stable tag, Michael?  I'm happy to add one,
> but wasn't sure if you wanted a separate ack for that.


I'd like a stable tag, yes. Just was not sure whom best to ask.



> > ---
> > tested in QEMU with Q35 machine on PCIE root port and also
> > with nested conventional bridge attached to root port.
> > 
> > v2:
> >   * fixup subject to match expected style
> >   * drop no longer needed __pci_bus_size_bridges() to avoid
> >     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 5b1f271c6034..328d1e416014 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >  				acpiphp_native_scan_bridge(dev);
> >  		}
> >  	} else {
> > -		LIST_HEAD(add_list);
> >  		int max, pass;
> >  
> >  		acpiphp_rescan_slot(slot);
> > @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >  				if (pass && dev->subordinate) {
> >  					check_hotplug_bridge(slot, dev);
> >  					pcibios_resource_survey_bus(dev->subordinate);
> > -					__pci_bus_size_bridges(dev->subordinate,
> > -							       &add_list);
> >  				}
> >  			}
> >  		}
> > -		__pci_bus_assign_resources(bus, &add_list, NULL);
> > +		pci_assign_unassigned_bridge_resources(bus->self);
> >  	}
> >  
> >  	acpiphp_sanitize_bus(bus);
> > -- 
> > 2.39.1
> >
Rafael J. Wysocki May 22, 2023, 9:55 p.m. UTC | #8
On Mon, May 22, 2023 at 11:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Tentatively applied to pci/hotplug for v6.5.
>
> Tentative only because I'm hoping for your ack or review, Rafael (I
> see you acked v1, but this one is a little different).

This one is fine with me too, so please feel free to add

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

to it.

> And I think you'd like a stable tag, Michael?  I'm happy to add one,
> but wasn't sure if you wanted a separate ack for that.
>
> > ---
> > tested in QEMU with Q35 machine on PCIE root port and also
> > with nested conventional bridge attached to root port.
> >
> > v2:
> >   * fixup subject to match expected style
> >   * drop no longer needed __pci_bus_size_bridges() to avoid
> >     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 5b1f271c6034..328d1e416014 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >                               acpiphp_native_scan_bridge(dev);
> >               }
> >       } else {
> > -             LIST_HEAD(add_list);
> >               int max, pass;
> >
> >               acpiphp_rescan_slot(slot);
> > @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >                               if (pass && dev->subordinate) {
> >                                       check_hotplug_bridge(slot, dev);
> >                                       pcibios_resource_survey_bus(dev->subordinate);
> > -                                     __pci_bus_size_bridges(dev->subordinate,
> > -                                                            &add_list);
> >                               }
> >                       }
> >               }
> > -             __pci_bus_assign_resources(bus, &add_list, NULL);
> > +             pci_assign_unassigned_bridge_resources(bus->self);
> >       }
> >
> >       acpiphp_sanitize_bus(bus);
> > --
> > 2.39.1
> >
Bjorn Helgaas May 23, 2023, 3:42 p.m. UTC | #9
On Mon, May 22, 2023 at 04:51:41PM -0500, Bjorn Helgaas wrote:
> On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Tentatively applied to pci/hotplug for v6.5.
> 
> Tentative only because I'm hoping for your ack or review, Rafael (I
> see you acked v1, but this one is a little different).
> 
> And I think you'd like a stable tag, Michael?  I'm happy to add one,
> but wasn't sure if you wanted a separate ack for that.

Added stable tag and Rafael's ack.  All set for v6.5, thanks!

> > ---
> > tested in QEMU with Q35 machine on PCIE root port and also
> > with nested conventional bridge attached to root port.
> > 
> > v2:
> >   * fixup subject to match expected style
> >   * drop no longer needed __pci_bus_size_bridges() to avoid
> >     memory leak (Bjorn Helgaas <helgaas@kernel.org>)
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 5b1f271c6034..328d1e416014 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -498,7 +498,6 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >  				acpiphp_native_scan_bridge(dev);
> >  		}
> >  	} else {
> > -		LIST_HEAD(add_list);
> >  		int max, pass;
> >  
> >  		acpiphp_rescan_slot(slot);
> > @@ -512,12 +511,10 @@ static void enable_slot(struct acpiphp_slot *slot, bool bridge)
> >  				if (pass && dev->subordinate) {
> >  					check_hotplug_bridge(slot, dev);
> >  					pcibios_resource_survey_bus(dev->subordinate);
> > -					__pci_bus_size_bridges(dev->subordinate,
> > -							       &add_list);
> >  				}
> >  			}
> >  		}
> > -		__pci_bus_assign_resources(bus, &add_list, NULL);
> > +		pci_assign_unassigned_bridge_resources(bus->self);
> >  	}
> >  
> >  	acpiphp_sanitize_bus(bus);
> > -- 
> > 2.39.1
> >
Bjorn Helgaas May 30, 2023, 5:12 p.m. UTC | #10
On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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)

Ugh, I just noticed that we turned on PCI_COMMAND_MEMORY even though
BARs 0 and 1 haven't been assigned.  How did that happen?  It looks
like pci_enable_resources() checks for that, but there must be a hole
somewhere.

>    Unable to create vram_mapping
>    qxl: probe of 0000:01:00.0 failed with error -12
Michael S. Tsirkin May 30, 2023, 6:16 p.m. UTC | #11
On Tue, May 30, 2023 at 12:12:44PM -0500, Bjorn Helgaas wrote:
> On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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)
> 
> Ugh, I just noticed that we turned on PCI_COMMAND_MEMORY even though
> BARs 0 and 1 haven't been assigned.  How did that happen?  It looks
> like pci_enable_resources() checks for that, but there must be a hole
> somewhere.

Maybe because BAR2 was assigned? I think pci_enable_resources just
does
                if (r->flags & IORESOURCE_MEM)
                        cmd |= PCI_COMMAND_MEMORY;
in a loop so if any memory BARs are assigned then PCI_COMMAND_MEMORY
is set.

> >    Unable to create vram_mapping
> >    qxl: probe of 0000:01:00.0 failed with error -12
Bjorn Helgaas May 30, 2023, 6:55 p.m. UTC | #12
On Tue, May 30, 2023 at 02:16:36PM -0400, Michael S. Tsirkin wrote:
> On Tue, May 30, 2023 at 12:12:44PM -0500, Bjorn Helgaas wrote:
> > On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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)
> > 
> > Ugh, I just noticed that we turned on PCI_COMMAND_MEMORY even though
> > BARs 0 and 1 haven't been assigned.  How did that happen?  It looks
> > like pci_enable_resources() checks for that, but there must be a hole
> > somewhere.
> 
> Maybe because BAR2 was assigned? I think pci_enable_resources just
> does
>                 if (r->flags & IORESOURCE_MEM)
>                         cmd |= PCI_COMMAND_MEMORY;
> in a loop so if any memory BARs are assigned then PCI_COMMAND_MEMORY
> is set.

It does, but it also bails out if it finds IORESOURCE_UNSET:

  pci_enable_resources()
  {
    ...
    pci_dev_for_each_resource(dev, r, i) {
      ...
      if (r->flags & IORESOURCE_UNSET) {
        pci_err(dev, "can't enable device: BAR %d %pR not assigned\n");
        return -EINVAL;
      }
      ...
      if (r->flags & IORESOURCE_MEM)
        cmd |= PCI_COMMAND_MEMORY;
    }
    ...
  }

I expected that IORESOURCE_UNSET would still be there from
pci_assign_resource(), since we saw the "failed to assign" messages,
but there must be more going on.

Bjorn
Michael S. Tsirkin May 30, 2023, 7:06 p.m. UTC | #13
On Tue, May 30, 2023 at 01:55:12PM -0500, Bjorn Helgaas wrote:
> On Tue, May 30, 2023 at 02:16:36PM -0400, Michael S. Tsirkin wrote:
> > On Tue, May 30, 2023 at 12:12:44PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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)
> > > 
> > > Ugh, I just noticed that we turned on PCI_COMMAND_MEMORY even though
> > > BARs 0 and 1 haven't been assigned.  How did that happen?  It looks
> > > like pci_enable_resources() checks for that, but there must be a hole
> > > somewhere.
> > 
> > Maybe because BAR2 was assigned? I think pci_enable_resources just
> > does
> >                 if (r->flags & IORESOURCE_MEM)
> >                         cmd |= PCI_COMMAND_MEMORY;
> > in a loop so if any memory BARs are assigned then PCI_COMMAND_MEMORY
> > is set.
> 
> It does, but it also bails out if it finds IORESOURCE_UNSET:
> 
>   pci_enable_resources()
>   {
>     ...
>     pci_dev_for_each_resource(dev, r, i) {
>       ...
>       if (r->flags & IORESOURCE_UNSET) {
>         pci_err(dev, "can't enable device: BAR %d %pR not assigned\n");
>         return -EINVAL;
>       }
>       ...
>       if (r->flags & IORESOURCE_MEM)
>         cmd |= PCI_COMMAND_MEMORY;
>     }
>     ...
>   }
> 
> I expected that IORESOURCE_UNSET would still be there from
> pci_assign_resource(), since we saw the "failed to assign" messages,
> but there must be more going on.
> 
> Bjorn

Oh you are right. I donnu.
Igor Mammedov May 31, 2023, 3:22 p.m. UTC | #14
On Tue, 30 May 2023 13:55:12 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Tue, May 30, 2023 at 02:16:36PM -0400, Michael S. Tsirkin wrote:
> > On Tue, May 30, 2023 at 12:12:44PM -0500, Bjorn Helgaas wrote:  
> > > On Mon, Apr 24, 2023 at 09:15:57PM +0200, Igor Mammedov 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)  
> > > 
> > > Ugh, I just noticed that we turned on PCI_COMMAND_MEMORY even though
> > > BARs 0 and 1 haven't been assigned.  How did that happen?  It looks
> > > like pci_enable_resources() checks for that, but there must be a hole
> > > somewhere.  
> > 
> > Maybe because BAR2 was assigned? I think pci_enable_resources just
> > does
> >                 if (r->flags & IORESOURCE_MEM)
> >                         cmd |= PCI_COMMAND_MEMORY;
> > in a loop so if any memory BARs are assigned then PCI_COMMAND_MEMORY
> > is set.  
> 
> It does, but it also bails out if it finds IORESOURCE_UNSET:
> 
>   pci_enable_resources()
>   {
>     ...
>     pci_dev_for_each_resource(dev, r, i) {
>       ...
>       if (r->flags & IORESOURCE_UNSET) {
>         pci_err(dev, "can't enable device: BAR %d %pR not assigned\n");
>         return -EINVAL;
>       }
>       ...
>       if (r->flags & IORESOURCE_MEM)
>         cmd |= PCI_COMMAND_MEMORY;
>     }
>     ...
>   }
> 
> I expected that IORESOURCE_UNSET would still be there from
> pci_assign_resource(), since we saw the "failed to assign" messages,
> but there must be more going on.

with current acpiphp code pci_assign_resource() isn't called,
instead it goes __pci_bus_assign_resources() route.

However I an reproduce similar issue with SHPC when using
hierarchy deeper than 1 bridge (for which relocation has never worked)

qemu-kvm -monitor stdio -M q35 -cpu host -enable-kvm  -m 4G \
   -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off \
   -device pcie-root-port,id=pr1,bus=pcie.0,chassis=4 \
   -device pcie-pci-bridge,id=br1,bus=pr1

hotplug device
  (qemu) device_add qxl,addr=1,bus=br1

shpchp 0000:01:00.0: Latch close on Slot(1)
shpchp 0000:01:00.0: Button pressed on Slot(1)
shpchp 0000:01:00.0: Card present on Slot(1)
shpchp 0000:01:00.0: PCI slot #1 - powering on due to button press
pci 0000:02:01.0: [1b36:0100] type 00 class 0x038000
pci 0000:02:01.0: reg 0x10: [mem 0x00000000-0x03ffffff]
pci 0000:02:01.0: reg 0x14: [mem 0x00000000-0x03ffffff]
pci 0000:02:01.0: reg 0x18: [mem 0x00000000-0x00001fff]
pci 0000:02:01.0: reg 0x1c: [io  0x0000-0x001f]
pci 0000:02:01.0: BAR 0: no space for [mem size 0x04000000]
pci 0000:02:01.0: BAR 0: failed to assign [mem size 0x04000000]
pci 0000:02:01.0: BAR 1: no space for [mem size 0x04000000]
pci 0000:02:01.0: BAR 1: failed to assign [mem size 0x04000000]
pci 0000:02:01.0: BAR 2: assigned [mem 0xfe600000-0xfe601fff]
pci 0000:02:01.0: BAR 3: assigned [io  0xc000-0xc01f]
                  ^^^^^^^^^^^
shpchp 0000:01:00.0: PCI bridge to [bus 02]
shpchp 0000:01:00.0:   bridge window [io  0xc000-0xcfff]
shpchp 0000:01:00.0:   bridge window [mem 0xfe600000-0xfe7fffff]
shpchp 0000:01:00.0:   bridge window [mem 0xfe000000-0xfe1fffff 64bit pref]
PCI: No. 2 try to assign unassigned res
release child resource [mem 0xfe600000-0xfe601fff]
shpchp 0000:01:00.0: resource 8 [mem 0xfe600000-0xfe7fffff] released
shpchp 0000:01:00.0: PCI bridge to [bus 02]
shpchp 0000:01:00.0: BAR 8: no space for [mem size 0x0a000000]
shpchp 0000:01:00.0: BAR 8: failed to assign [mem size 0x0a000000]
pci 0000:02:01.0: BAR 0: no space for [mem size 0x04000000]
pci 0000:02:01.0: BAR 0: failed to assign [mem size 0x04000000]
pci 0000:02:01.0: BAR 1: no space for [mem size 0x04000000]
pci 0000:02:01.0: BAR 1: failed to assign [mem size 0x04000000]
pci 0000:02:01.0: BAR 2: no space for [mem size 0x00002000]
pci 0000:02:01.0: BAR 2: failed to assign [mem size 0x00002000]
shpchp 0000:01:00.0: PCI bridge to [bus 02]
shpchp 0000:01:00.0:   bridge window [io  0xc000-0xcfff]
shpchp 0000:01:00.0:   bridge window [mem 0xfe000000-0xfe1fffff 64bit pref]
qxl 0000:02:01.0: enabling device (0000 -> 0001)
                                           ^^^ IO res only
where:
  assign_requested_resources_sorted()
     ...
     if (pci_assign_resource())
        reset_resource(res);
reset wipes everything pci_assign_resource() has done for failing resources
leaving only assigned IO (on the 1st pass).
Then later pci_enable_device_flags() will build mask for available bars

        for (i = 0; i <= PCI_ROM_RESOURCE; i++)                                  
                if (dev->resource[i].flags & flags)                              
                        bars |= (1 << i);                                        
        for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++)           
                if (dev->resource[i].flags & flags)                              
                        bars |= (1 << i);

however since for failed MEM resources reset cleared flags along with everything else, 
above snippet will ignore MEM bars, leaving only assigned IO resource.
Then
   do_pci_enable_device() -> pcibios_enable_device() -> pci_enable_resources(,bars)
will happily succeed since mask tells it only to look into IO.

And well even if mask weren't excluding MEM ones, it won't help since
the resource was cleared out in assign_requested_resources_sorted().

Perhaps instead of playing with flags, we should somehow mark device
with unusable resources as disabled, and fail pci_enable_device() early.
(and also make sure its resources aren't accounted anymore on follow
up hotplug events to the bridge)
diff mbox series

Patch

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 5b1f271c6034..328d1e416014 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -498,7 +498,6 @@  static void enable_slot(struct acpiphp_slot *slot, bool bridge)
 				acpiphp_native_scan_bridge(dev);
 		}
 	} else {
-		LIST_HEAD(add_list);
 		int max, pass;
 
 		acpiphp_rescan_slot(slot);
@@ -512,12 +511,10 @@  static void enable_slot(struct acpiphp_slot *slot, bool bridge)
 				if (pass && dev->subordinate) {
 					check_hotplug_bridge(slot, dev);
 					pcibios_resource_survey_bus(dev->subordinate);
-					__pci_bus_size_bridges(dev->subordinate,
-							       &add_list);
 				}
 			}
 		}
-		__pci_bus_assign_resources(bus, &add_list, NULL);
+		pci_assign_unassigned_bridge_resources(bus->self);
 	}
 
 	acpiphp_sanitize_bus(bus);