Re: [PATCH v1] i440fx/acpi: don't hot-unplug cold plugged bridges when their hotplug switch is off

[Ani Sinha]

On Fri, Sep 4, 2020 at 9:18 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> On Fri, Sep 4, 2020 at 10:25 AM Ani Sinha <ani@anisinha.ca> wrote:
> >
> > Cold plugged bridges should not be hot unpluggable, even when their hotplug 
> > property
> > (acpi-pci-hotplug-with-bridge-support) is turned off.
>
> Unplugging a cold-plugged bridge is impossible already, see
> acpi_pcihp_pc_no_hotplug().

Ah cool. So all the more reason we should fix this because guest OSes
should not think that the device is hot-unpluggable.

>
> Best regards, Julia Suvorova.
>
> > However, with the current
> > implementaton, windows would try to hot-unplug a pci bridge when it's 
> > hotplug switch
> > is off. This is regardless of whether there are devices attached to the 
> > bridge. When
> > devices are attached to the bridge, the bridge is ultimately not 
> > hot-unpluggable. We
> > have a demo video here: https://youtu.be/pME2sjyQweo
> >
> > In this fix, we identify a cold plugged bridge and for cold plugged 
> > bridges, we do not
> > add the appropriate amls and acpi methods that are used by the OS to 
> > identify a hot-
> > unpluggable pci device. After this change, Windows does not show an option 
> > to eject the
> > PCI bridge. A demo video is here:  https://youtu.be/kbgej5B9Hgs
> >
> > While at it, I have also updated a stale comment.
> >
> > This change is tested with a Windows 2012R2 guest image running on Ubuntu 
> > host. This
> > change is based off of upstream qemu master branch tag v5.1.0.
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/i386/acpi-build.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b7bcbbbb2a..90b863f4ec 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -359,6 +359,7 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >          int slot = PCI_SLOT(i);
> >          bool hotplug_enabled_dev;
> >          bool bridge_in_acpi;
> > +        bool cold_plugged_bridge;
> >
> >          if (!pdev) {
> >              if (bsel) { /* add hotplug slots for non present devices */
> > @@ -380,15 +381,14 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >          pc = PCI_DEVICE_GET_CLASS(pdev);
> >          dc = DEVICE_GET_CLASS(pdev);
> >
> > -        /* When hotplug for bridges is enabled, bridges are
> > -         * described in ACPI separately (see build_pci_bus_end).
> > -         * In this case they aren't themselves hot-pluggable.
> > +        /*
> > +         * Cold plugged bridges aren't themselves hot-pluggable.
> >           * Hotplugged bridges *are* hot-pluggable.
> >           */
> > -        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
> > -            !DEVICE(pdev)->hotplugged;
> > +        cold_plugged_bridge = pc->is_bridge && !DEVICE(pdev)->hotplugged;
> > +        bridge_in_acpi =  cold_plugged_bridge && pcihp_bridge_en;
> >
> > -        hotplug_enabled_dev = bsel && dc->hotpluggable && !bridge_in_acpi;
> > +        hotplug_enabled_dev = bsel && dc->hotpluggable && 
> > !cold_plugged_bridge;
> >
> >          if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> >              continue;
> > --
> > 2.17.1
> >
>