Julia Michael,

Can you please provide your inputs on this one and the corresponding unit
tests?

On Fri, Sep 4, 2020 at 9:40 PM Ani Sinha <ani@anisinha.ca> wrote:

> Cold plugged bridges are not hot unpluggable, even when their hotplug
>
> property (acpi-pci-hotplug-with-bridge-support) is turned off. Please see
>
> the function acpi_pcihp_pc_no_hotplug() (thanks Julia). 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. This is because we add amls like _EJ0 etc for the
>
> pci slot where the bridge is cold plugged. 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-pluggable/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:
>
> v3: commit log updates providing more accurate information as received
> from Julia.
>
> v2: cosmetic commit log updates with patch testing information.
>
> 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
>
>
>
>