Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges

[Ani Sinha]

Gentle reminder to kindly review this patch :-)

@Julia Suvorova @Michael S. Tsirkin

On Mon, Sep 7, 2020 at 6:47 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> 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
>>
>>
>>