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

[Ani Sinha]

On Fri, Sep 11, 2020 at 9:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Sep 11, 2020 at 09:35:42PM +0530, Ani Sinha wrote:
> > On Fri, Sep 11, 2020 at 9:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Sep 10, 2020 at 12:19:19AM +0530, Ani Sinha wrote:
> > > > On Sep 10, 2020, 00:00 +0530, Ani Sinha <ani@anisinha.ca>, wrote:
> > > >
> > > >     On Sep 9, 2020, 23:20 +0530, Julia Suvorova <jusual@redhat.com>, 
> > > > wrote:
> > > >
> > > >         On Fri, Sep 4, 2020 at 6:10 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>
> > > >
> > > >
> > > >
> > > >         Reviewed-by: Julia Suvorova <jusual@redhat.com>
> > > >
> > > >
> > > >
> > > >         BTW, aren't all bridges handled in 
> > > > build_append_pci_bus_devices()
> > > >         cold-plugged?
> > > >
> > > >
> > > >
> > > >     Yes they are.
> > > >
> > > >
> > > > Maybe as an improvement we can simply identify a bridge instead of a 
> > > > cold
> > > > plugged bridge. However let’s have that improvement as a separate patch 
> > > > on top
> > > > of this. Also let’s see what Igor thinks.
> > >
> > > Well this changes acpi tables so I don't see how this will pass
> > > the unit tests. Did you test this change?
> >
> > There were no unit tests to cover this bridge flag. I have added the
> > unit tests. It's the same one you have queued.
>
>
> Hmm. somehow the new tests fail for me ...

Maybe you need to pull this change out, make sure the tests passes and
then apply it again:

commit b26b2241baefc51fe2069b83cea9cfa654e75a43 (HEAD -> v5.1.0)
Author: Ani Sinha <ani@anisinha.ca>
Date:   Tue Sep 1 15:19:22 2020 +0530

    piix4: don't reserve hw resources when hotplug is off globally

    When acpi hotplug is turned off for both root pci bus as well as for pci
    bridges, we should not generate the related amls for DSDT table or
initialize
    related hw ports or reserve hw resources. This change makes sure all those
    operations are turned off in the case acpi pci hotplug is off globally.

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