| Message ID | 20200904161000.12115-1-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges | expand |
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 > > > >
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 >> >> >>
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? > --- > 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 >
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. > > > --- > > 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 > > >
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. > > > > > --- > > > 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 > > > > >
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? > > > > --- > > 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 > > > > >
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. > > > > > > > > > > --- > > > > 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 > > > > > > > > > > >
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 ... > > > > > > > > > > > > > > > > --- > > > > > > 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 > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > > > >
On Fri, Sep 11, 2020 at 9:45 PM Ani Sinha <ani@anisinha.ca> wrote: > > 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> For this patch, I have not sent any updated unit tests. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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 ... Ok I was working off v5.1.0 tag. Did not realize. I rebased all my patches to the latest master and reworked the unit tests. I have sent you the entire patch set as exists in my workspace. It passes unit tests. > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > >
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;
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.