| Message ID | 20200904124404.9894-1-ani@anisinha.ca |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i440fx/acpi: disable hotplug of cold plugged bridges regarldless of hotplug switch | expand |
On Sep 4, 2020, 18:59 +0530, Philippe Mathieu-Daudé <philmd@redhat.com>, wrote: Hi Ani, On 9/4/20 2:44 PM, Ani Sinha wrote: Cold plugged bridges should not be hot unpluggable, even when their hotplug property (acpi-pci-hotplug-with-bridge-support) is turned off. 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. When devices are attached to the bridge, the bridge is ultimately not hot-unpluggable. 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-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. Can you add a test reproducer? Looking at tests/qtest/virtio-rng-test.c it shouldn't be that hard / time consuming. I can add a unit test. If you look at the disassembled DSDT table, you can easily see that it’s adding the EJ0 method for the slot in which the bridge is attached. <div dir="ltr"> <div> <div name="messageReplySection"> <div dir="auto">On Sep 4, 2020, 18:59 +0530, Philippe Mathieu-Daudé <<a href="mailto:philmd@redhat.com" target="_blank">philmd@redhat.com</a>>, wrote:</div> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">Hi Ani,</blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><br></blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">On 9/4/20 2:44 PM, Ani Sinha wrote:</blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">Cold plugged bridges should not be hot unpluggable, even when their hotplug</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">property (acpi-pci-hotplug-with-bridge-support) is turned off. However, with</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">the current implementaton, windows would try to hot-unplug a pci bridge when</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">it's hotplug switch is off. This is regardless of whether there are devices</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">attached to the bridge. When devices are attached to the bridge, the bridge</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">is ultimately not hot-unpluggable. We have a demo video here:</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><a href="https://youtu.be/pME2sjyQweo" target="_blank">https://youtu.be/pME2sjyQweo</a></blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><br></blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">In this fix, we identify a cold plugged bridge and for cold plugged bridges,</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">we do not add the appropriate amls and acpi methods that are used by the OS</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">to identify a hot-unpluggable pci device. After this change, Windows does not</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">show an option to eject the PCI bridge. A demo video is here:</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><a href="https://youtu.be/kbgej5B9Hgs" target="_blank">https://youtu.be/kbgej5B9Hgs</a></blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><br></blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">While at it, I have also updated a stale comment.</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><br></blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">This change is tested with a Windows 2012R2 guest image and Windows 2019 server</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">guest image running on Ubuntu 18.04 host. This change is based off of upstream</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid"> <blockquote style="border-left-color:rgb(230,126,34);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">qemu master branch tag v5.1.0.</blockquote> </blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><br></blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">Can you add a test reproducer?</blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px"><br></blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">Looking at tests/qtest/virtio-rng-test.c it shouldn't be that</blockquote> <blockquote style="border-left-color:rgb(26,188,156);margin:0px;padding-left:10px;border-left-width:thin;border-left-style:solid;padding-bottom:5px;padding-top:5px">hard / time consuming.</blockquote> <div dir="auto"><br> I can add a unit test. If you look at the disassembled DSDT table, you can easily see that it’s adding the EJ0 method for the slot in which the bridge is attached.<br></div> </div> </div> </div>
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 should not be hot unpluggable, even when their hotplug property (acpi-pci-hotplug-with-bridge-support) is turned off. 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. When devices are attached to the bridge, the bridge is ultimately not hot-unpluggable. 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-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: v2: cosmetic commit log update. v1: initial patch.