[PATCH v2] i440fx/acpi: disable hotplug of cold plugged bridges regarldless of hotplug switch

[PATCH v2] i440fx/acpi: disable hotplug of cold plugged bridges regarldless of hotplug switch

[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 and Windows 2019 server
guest image running on Ubuntu 18.04 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(-)

Changelog:
v2: cosmetic commit log update.
v1: initial patch.

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 v2] i440fx/acpi: disable hotplug of cold plugged bridges regarldless of hotplug switch

[Philippe Mathieu-Daudé]

Hi Ani,

On 9/4/20 2:44 PM, Ani Sinha wrote:
> 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 and Windows 2019 server
> guest image running on Ubuntu 18.04 host. This change is based off of upstream
> qemu master branch tag v5.1.0.

Can you add a test reproducer?

Looking at tests/qtest/virtio-rng-test.c it shouldn't be that
hard / time consuming.

Thanks!

Phil.

> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/i386/acpi-build.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Changelog:
> v2: cosmetic commit log update.
> v1: initial patch.
> 
> 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;
> 

Re: [PATCH v2] i440fx/acpi: disable hotplug of cold plugged bridges regarldless of hotplug switch

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

 On Sep 4, 2020, 18:59 +0530, Philippe Mathieu-Daudé <philmd@redhat.com>,
wrote:

Hi Ani,


On 9/4/20 2:44 PM, Ani Sinha wrote:

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 and Windows 2019
server

guest image running on Ubuntu 18.04 host. This change is based off of
upstream

qemu master branch tag v5.1.0.


Can you add a test reproducer?


Looking at tests/qtest/virtio-rng-test.c it shouldn't be that

hard / time consuming.


I can add a unit test. If you look at the disassembled DSDT table, you can
easily see that it’s adding the EJ0 method for the slot in which the bridge
is attached.

[-- Attachment #2: Type: text/html, Size: 9433 bytes --]

Re: [PATCH v2] i440fx/acpi: disable hotplug of cold plugged bridges regarldless of hotplug switch

[Ani Sinha]

On Fri, Sep 4, 2020 at 7:09 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Sep 4, 2020, 18:59 +0530, Philippe Mathieu-Daudé <philmd@redhat.com>, wrote:
>
> Hi Ani,
>
>
> On 9/4/20 2:44 PM, Ani Sinha wrote:
>
> 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 and Windows 2019 server
>
> guest image running on Ubuntu 18.04 host. This change is based off of upstream
>
> qemu master branch tag v5.1.0.
>
>
> Can you add a test reproducer?
>
>
> Looking at tests/qtest/virtio-rng-test.c it shouldn't be that
>
> hard / time consuming.

I think this test is hotplugging a bridge through the qmp command.
This test does not apply here as we are talking about cold plugged
bridges and making sure cold plugged bridges are not ejectable.
If you have a test infra to test such a case, please let me know.

>
> I can add a unit test. If you look at the disassembled DSDT table, you can easily see that it’s adding the EJ0 method for the slot in which the bridge is attached.

The unit test will capture this change perfectly. It will compare the
generated AML against expected AML and fail if they don't match.