* [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
@ 2020-09-04 16:10 Ani Sinha
2020-09-07 13:17 ` Ani Sinha
2020-09-09 17:50 ` Julia Suvorova
0 siblings, 2 replies; 12+ messages in thread
From: Ani Sinha @ 2020-09-04 16:10 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, jusual, Paolo Bonzini,
Ani Sinha, Igor Mammedov, Richard Henderson
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-04 16:10 [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges Ani Sinha
@ 2020-09-07 13:17 ` Ani Sinha
2020-09-09 12:14 ` Ani Sinha
2020-09-09 17:50 ` Julia Suvorova
1 sibling, 1 reply; 12+ messages in thread
From: Ani Sinha @ 2020-09-07 13:17 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, jusual, Paolo Bonzini,
Igor Mammedov, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]
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
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 4344 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-07 13:17 ` Ani Sinha
@ 2020-09-09 12:14 ` Ani Sinha
0 siblings, 0 replies; 12+ messages in thread
From: Ani Sinha @ 2020-09-09 12:14 UTC (permalink / raw)
To: QEMU Developers
Cc: Eduardo Habkost, Michael S. Tsirkin, Julia Suvorova,
Paolo Bonzini, Igor Mammedov, Richard Henderson
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
>>
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-04 16:10 [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges Ani Sinha
2020-09-07 13:17 ` Ani Sinha
@ 2020-09-09 17:50 ` Julia Suvorova
2020-09-09 18:30 ` Ani Sinha
1 sibling, 1 reply; 12+ messages in thread
From: Julia Suvorova @ 2020-09-09 17:50 UTC (permalink / raw)
To: Ani Sinha
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Developers,
Paolo Bonzini, Igor Mammedov, Richard Henderson
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-09 17:50 ` Julia Suvorova
@ 2020-09-09 18:30 ` Ani Sinha
2020-09-09 18:49 ` Ani Sinha
0 siblings, 1 reply; 12+ messages in thread
From: Ani Sinha @ 2020-09-09 18:30 UTC (permalink / raw)
To: Julia Suvorova
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Developers,
Paolo Bonzini, Igor Mammedov, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]
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
> >
>
[-- Attachment #2: Type: text/html, Size: 29191 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-09 18:30 ` Ani Sinha
@ 2020-09-09 18:49 ` Ani Sinha
2020-09-11 15:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Ani Sinha @ 2020-09-09 18:49 UTC (permalink / raw)
To: Julia Suvorova
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Developers,
Paolo Bonzini, Igor Mammedov, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 3763 bytes --]
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
> > >
> >
[-- Attachment #2: Type: text/html, Size: 42146 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-09 18:49 ` Ani Sinha
@ 2020-09-11 15:52 ` Michael S. Tsirkin
2020-09-11 16:05 ` Ani Sinha
0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2020-09-11 15:52 UTC (permalink / raw)
To: Ani Sinha
Cc: Eduardo Habkost, Julia Suvorova, QEMU Developers, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
>
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-11 15:52 ` Michael S. Tsirkin
@ 2020-09-11 16:05 ` Ani Sinha
2020-09-11 16:08 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Ani Sinha @ 2020-09-11 16:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eduardo Habkost, Julia Suvorova, QEMU Developers, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
> >
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-11 16:05 ` Ani Sinha
@ 2020-09-11 16:08 ` Michael S. Tsirkin
2020-09-11 16:15 ` Ani Sinha
2020-09-11 18:10 ` Ani Sinha
0 siblings, 2 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2020-09-11 16:08 UTC (permalink / raw)
To: Ani Sinha
Cc: Eduardo Habkost, Julia Suvorova, QEMU Developers, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
> > >
> > >
> > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-11 16:08 ` Michael S. Tsirkin
@ 2020-09-11 16:15 ` Ani Sinha
2020-09-11 16:17 ` Ani Sinha
2020-09-11 18:10 ` Ani Sinha
1 sibling, 1 reply; 12+ messages in thread
From: Ani Sinha @ 2020-09-11 16:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eduardo Habkost, Julia Suvorova, QEMU Developers, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-11 16:15 ` Ani Sinha
@ 2020-09-11 16:17 ` Ani Sinha
0 siblings, 0 replies; 12+ messages in thread
From: Ani Sinha @ 2020-09-11 16:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eduardo Habkost, Julia Suvorova, QEMU Developers, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges
2020-09-11 16:08 ` Michael S. Tsirkin
2020-09-11 16:15 ` Ani Sinha
@ 2020-09-11 18:10 ` Ani Sinha
1 sibling, 0 replies; 12+ messages in thread
From: Ani Sinha @ 2020-09-11 18:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eduardo Habkost, Julia Suvorova, QEMU Developers, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-11 18:12 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 16:10 [PATCH v3] i440fx/acpi: do not add hotplug related amls for cold plugged bridges Ani Sinha
2020-09-07 13:17 ` Ani Sinha
2020-09-09 12:14 ` Ani Sinha
2020-09-09 17:50 ` Julia Suvorova
2020-09-09 18:30 ` Ani Sinha
2020-09-09 18:49 ` Ani Sinha
2020-09-11 15:52 ` Michael S. Tsirkin
2020-09-11 16:05 ` Ani Sinha
2020-09-11 16:08 ` Michael S. Tsirkin
2020-09-11 16:15 ` Ani Sinha
2020-09-11 16:17 ` Ani Sinha
2020-09-11 18:10 ` Ani Sinha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.