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:23 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> 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.

I have sent a V3 with info Julia sent and also rewording the commit
log to reflect the
correct information.

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