i440fx/acpi: don't hot-unplug cold plugged bridges when their hotplug switch is off

i440fx/acpi: don't hot-unplug cold plugged bridges when their hotplug switch is off

[Ani Sinha]

I have seen that when hotplug for pci bridge devices are turned off and devices are attached
behind the bridge, I still see Windows trying to hot remove the pci bridge and failing. Today
looking at the AML disassembly, I realized that we are addding slot enumeration and EJ0
methods even for the slot where the bridge is attached. 

Please see the video: https://youtu.be/pME2sjyQweo

This does not seem right. A cold-plugged bridge should not be hot removable under any
conditions. Hence, this patch tries to address this. After the change, as the demo video
shows, Windows no longer shows the bridge device as hot removable:  https://youtu.be/kbgej5B9Hgs
under the same test settings.

Requesting comments for this fix.

--Ani
 

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

[Ani Sinha]

Cold plugged bridges should not be hot unpluggable, even when their hotplug property
(acpi-pci-hotplug-with-bridge-support) is turned off. 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

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

[Julia Suvorova]

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().

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
>

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

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