Re: [PATCH] pci hotplug: rescan bridge after device hotplug

[Jason Baron <jbaron@redhat.com>]

On Tue, May 22, 2012 at 09:07:54PM -0700, Yinghai Lu wrote:
> On Tue, May 22, 2012 at 8:31 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Tue, May 22, 2012 at 7:43 PM, Jason Baron <jbaron@redhat.com> wrote:
> >> On Tue, May 22, 2012 at 02:21:13PM -0700, Yinghai Lu wrote:
> >>> On Tue, May 22, 2012 at 1:11 PM, Jason Baron <jbaron@redhat.com> wrote:
> >>> > I'm tyring to support bridge hotplug and devices below it in qemu via acpi
> >>> > hotplug. Currently only 1 level or 32 slots are supported. By allowing for a
> >>> > second level, we will be able to support 32^2 devices.
> >>> >
> >>> > If I first hotplug the bridge with no devices intially below it, the hotplug
> >>> > code sets the bridge memory window to 0 and does not increase it when
> >>> > subsequent devices are added below it.
> >>> >
> >>> > Fix this, by calling pci_rescan_bus_bridge_resize(), on the bridge directly
> >>> > below the root to re-size all the birdge windows that may have changed.
> >>> >
> >>> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> >>> > ---
> >>> >  drivers/pci/hotplug/acpiphp_glue.c |    8 ++++++++
> >>> >  1 files changed, 8 insertions(+), 0 deletions(-)
> >>> >
> >>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >>> > index 806c44f..8960c1e 100644
> >>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> >>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >>> > @@ -792,6 +792,7 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >>> >  {
> >>> >        struct pci_dev *dev;
> >>> >        struct pci_bus *bus = slot->bridge->pci_bus;
> >>> > +       struct pci_bus *rescan_bus;
> >>> >        struct acpiphp_func *func;
> >>> >        int retval = 0;
> >>> >        int num, max, pass;
> >>> > @@ -821,6 +822,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >>> >                }
> >>> >        }
> >>> >
> >>> > +       /* Ensure we rescan/setup a bridge for new devs hanging off of it */
> >>> > +       rescan_bus = bus;
> >>> > +       while (rescan_bus->parent && rescan_bus->parent->self)
> >>> > +               rescan_bus = rescan_bus->parent;
> >>> > +       if (rescan_bus->self)
> >>> > +               pci_rescan_bus_bridge_resize(rescan_bus->self);
> >>> > +
> >>>
> >>> No, you can not do that.  some parents bus could have other devices
> >>> and driver could be loaded for those devices.
> >>> so can not release resources that are used by those devices to resize
> >>> parent bridges.
> >>>
> >>
> >> hmmm...this patch also does what I want:
> >>
> >>
> >> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> >> index 806c44f..be63c72 100644
> >> --- a/drivers/pci/hotplug/acpiphp_glue.c
> >> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> >> @@ -821,6 +821,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> >>                }
> >>        }
> >>
> >> +       /* Ensure we rescan/setup a bridge for new devs hanging off of it */
> >> +       if (bus->self)
> >> +               pci_assign_unassigned_bridge_resources(bus->self);
> >> +
> >>        list_for_each_entry(func, &slot->funcs, sibling)
> >>                acpiphp_bus_add(func);
> >>
> >>
> >> There appears to be a precedant for something very similar in:
> >> drivers/pci/hotplug/pciehp_pci.c:pciehp_configure_device(), where there
> >> is a call to 'pci_assign_unassigned_bridge_resources(), when a new
> >> device is added...
> >
> > the bus could have other devices under that bridge already.
> >
> > so pci_assign_unassigned_bridge_resources() will still try to release
> > the bridge resource and
> > those devices resources.
> >
> > but pciehp we could do that, because it will only have one device
> > under that pcie root port or downstream port.
> >
> > so you can make qemu to support pcie and pciehp.
> >
> > for hot add one hotplug bridge support, we have
> >     bus->self->is_hotplug_bridge to set, so it will have allocate
> > minimum resource.
> >
> > so you also could try to use quirk etc to get that bit set.
> > please check
> > drivers/pci/quirks.c::
> > /* Allow manual resource allocation for PCI hotplug bridges
> >  * via pci=hpmemsize=nnM and pci=hpiosize=nnM parameters. For
> >  * some PCI-PCI hotplug bridges, like PLX 6254 (former HINT HB6),
> >  * kernel fails to allocate resources when hotplug device is
> >  * inserted and PCI bus is rescanned.
> >  */
> > static void __devinit quirk_hotplug_bridge(struct pci_dev *dev)
> > {
> >        dev->is_hotplug_bridge = 1;
> > }
> >
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_HINT, 0x0020, quirk_hotplug_bridge);
> >
> > but still would prefer you to make qemu to support pciehp.
> 
> another solution could be:
> 
> in qemu acpi dsdt, you could set bridge size for new added bridge.
> 
> current pbus_size_mem() will not shrink the old bridge resource size.
> 
> Thanks
> 
> Yinghai Lu

ok, I also tried hard-wiring the bridge io/mem base and limit registers on the
qemu side. That seems to work without any guest-side hotplug code changes. And
would seem to be more flexible than putting the limits in acpi.

Thanks,

-Jason