| Message ID | BF9E6F48-E047-4D1B-BEF1-A58024DE0C6E@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hot unplug disabling on pci-pci bridge | expand |
On Tue, 24 Mar 2020 10:06:00 +0000 Ani Sinha <ani.sinha@nutanix.com> wrote: > Hi All : > > I have been playing with Qemu trying to disable hot-unplug capability for conventional PCI. I have discussed this briefly on IRC and the plan is to have an option on the pci-pci bridge that would disable SHPC and ACPI hotplug capability for all the slots on that bus. I am _not_ trying to implement a per slot capability for conventional PCI as was previously posted for PCIE slots in this patch : https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg01833.html (we do not need this atm). > > I am following the conversations which happened few weeks back here: > > https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00985.html question is do you need to disable only unplug side both (plug+unplug) operations (like we did with PCIE)? > > To that end, I have been experimenting with Qemu using the patch I attach below. I have attached the virtio balloon driver with bus 1 which is attached to the pci bridge. Following is the libvirt 4.5 xml snippet which I am using: > > <controller type='pci' index='0' model='pci-root'> > <alias name='pci.0'/> > </controller> > <controller type='pci' index='1' model='pci-bridge'> > <model name='pci-bridge'/> > <target chassisNr='1'/> > <alias name='pci.1'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> > </controller> > <memballoon model='virtio'> > <stats period='30'/> > <alias name='balloon0'/> > <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> > </memballoon> > > > I am using a windows guest and from the guest I can see that the balloon driver is indeed attached to the pci bridge (see attached screenshot). > I still find Windows giving me an option to hot eject the pci balloon driver. > > So what am I doing wrong here? > > [cid:F2407B5B-BBB1-4A0C-91C4-975692E3BDE1] > > > > The Qemu patch I am experimenting with (which is currently a hack) is attached below. It is based off Qemu 2.12 and not the latest mainline. > > --- > hw/pci-bridge/pci_bridge_dev.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index b2d861d..e706d49 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -58,7 +58,7 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp) > > pci_bridge_initfn(dev, TYPE_PCI_BUS); > > - if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) { > + if (0) {//bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) { > dev->config[PCI_INTERRUPT_PIN] = 0x1; > memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", > shpc_bar_size(dev)); > @@ -161,7 +161,7 @@ static Property pci_bridge_dev_properties[] = { > DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi, > ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags, > - PCI_BRIDGE_DEV_F_SHPC_REQ, true), > + PCI_BRIDGE_DEV_F_SHPC_REQ, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -181,7 +181,7 @@ static const VMStateDescription pci_bridge_dev_vmstate = { > VMSTATE_END_OF_LIST() > } > }; > - > +#if 0 > static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > @@ -208,12 +208,12 @@ static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > } > shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp); > } > - > +#endif > static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > + //HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > > k->realize = pci_bridge_dev_realize; > k->exit = pci_bridge_dev_exitfn; > @@ -227,8 +227,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) > dc->props = pci_bridge_dev_properties; > dc->vmsd = &pci_bridge_dev_vmstate; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > - hc->plug = pci_bridge_dev_hotplug_cb; > - hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb; > + //hc->plug = pci_bridge_dev_hotplug_cb; > + //hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb; > } > > static const TypeInfo pci_bridge_dev_info = { > @@ -238,7 +238,7 @@ static const TypeInfo pci_bridge_dev_info = { > .class_init = pci_bridge_dev_class_init, > .instance_finalize = pci_bridge_dev_instance_finalize, > .interfaces = (InterfaceInfo[]) { > - { TYPE_HOTPLUG_HANDLER }, > + //{ TYPE_HOTPLUG_HANDLER }, > { INTERFACE_CONVENTIONAL_PCI_DEVICE }, > { } > } > -- > 1.9.4 >
On Tue, 24 Mar 2020 10:06:00 +0000 Ani Sinha <ani.sinha@nutanix.com> wrote: [...] > > To that end, I have been experimenting with Qemu using the patch I attach below. I have attached the virtio balloon driver with bus 1 which is attached to the pci bridge. Following is the libvirt 4.5 xml snippet which I am using: > > <controller type='pci' index='0' model='pci-root'> > <alias name='pci.0'/> > </controller> > <controller type='pci' index='1' model='pci-bridge'> > <model name='pci-bridge'/> > <target chassisNr='1'/> > <alias name='pci.1'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> > </controller> > <memballoon model='virtio'> > <stats period='30'/> > <alias name='balloon0'/> > <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> > </memballoon> > > > I am using a windows guest and from the guest I can see that the balloon driver is indeed attached to the pci bridge (see attached screenshot). > I still find Windows giving me an option to hot eject the pci balloon driver. > > So what am I doing wrong here? you probably have APCI hotplug for bridges enabled (which applies to all cold-plugged bridges). You can disable it by setting following property -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off then it should fall back to SHPC which are are tying to disable > [cid:F2407B5B-BBB1-4A0C-91C4-975692E3BDE1] > > > > The Qemu patch I am experimenting with (which is currently a hack) is attached below. It is based off Qemu 2.12 and not the latest mainline. > > --- > hw/pci-bridge/pci_bridge_dev.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index b2d861d..e706d49 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -58,7 +58,7 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp) > > pci_bridge_initfn(dev, TYPE_PCI_BUS); > > - if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) { > + if (0) {//bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) { > dev->config[PCI_INTERRUPT_PIN] = 0x1; > memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", > shpc_bar_size(dev)); > @@ -161,7 +161,7 @@ static Property pci_bridge_dev_properties[] = { > DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi, > ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags, > - PCI_BRIDGE_DEV_F_SHPC_REQ, true), > + PCI_BRIDGE_DEV_F_SHPC_REQ, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -181,7 +181,7 @@ static const VMStateDescription pci_bridge_dev_vmstate = { > VMSTATE_END_OF_LIST() > } > }; > - > +#if 0 > static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > @@ -208,12 +208,12 @@ static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > } > shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp); > } > - > +#endif > static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > + //HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); > > k->realize = pci_bridge_dev_realize; > k->exit = pci_bridge_dev_exitfn; > @@ -227,8 +227,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) > dc->props = pci_bridge_dev_properties; > dc->vmsd = &pci_bridge_dev_vmstate; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > - hc->plug = pci_bridge_dev_hotplug_cb; > - hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb; > + //hc->plug = pci_bridge_dev_hotplug_cb; > + //hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb; > } > > static const TypeInfo pci_bridge_dev_info = { > @@ -238,7 +238,7 @@ static const TypeInfo pci_bridge_dev_info = { > .class_init = pci_bridge_dev_class_init, > .instance_finalize = pci_bridge_dev_instance_finalize, > .interfaces = (InterfaceInfo[]) { > - { TYPE_HOTPLUG_HANDLER }, > + //{ TYPE_HOTPLUG_HANDLER }, > { INTERFACE_CONVENTIONAL_PCI_DEVICE }, > { } > } > -- > 1.9.4 >
> On Mar 24, 2020, at 4:38 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > question is do you need to disable only unplug side both > (plug+unplug) operations (like we did with PCIE)? I need to disable the unplug side only. Ani
On Mar 24, 2020, at 4:47 PM, Igor Mammedov <imammedo@redhat.com<mailto:imammedo@redhat.com>> wrote:
you probably have APCI hotplug for bridges enabled (which applies to
all cold-plugged bridges).
You can disable it by setting following property
-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off
then it should fall back to SHPC which are are tying to disable
Ok I patched to change the default :
---
hw/acpi/piix4.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index d706360..db888cb 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -675,7 +675,7 @@ static Property piix4_pm_properties[] = {
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 1),
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
- use_acpi_pci_hotplug, true),
+ use_acpi_pci_hotplug, false),
DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
acpi_memory_hotplug.is_enabled, true),
DEFINE_PROP_END_OF_LIST(),
--
1.9.4
This seems to work but now Windows is giving an option to eject pci bridges. Clicking it does not do anything. I wonder if we can disable that as well. AFAIK hot unplugging bridges would not take effect until ACPI tables are updated and the OS kernel re-reads the updated tables. See https://bugzilla.redhat.com/show_bug.cgi?id=1339875
Ani
[cid:809BEDB4-EEF6-4197-A083-4AF2E9999CEB]
On Tue, 24 Mar 2020 11:49:53 +0000 Ani Sinha <ani.sinha@nutanix.com> wrote: > > On Mar 24, 2020, at 4:38 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > > > question is do you need to disable only unplug side both > > (plug+unplug) operations (like we did with PCIE)? > > I need to disable the unplug side only. I don't know about SHPC (i.e you probably can only have both or none) For ACPI hotplug it should be possible to hack current impl., one would need to remove _EJ0->PCEJ methods. That should hide ability to eject device in Windows but IO ports will still be there so malicious guest will still be able to eject device. But I'd emulate real hardware behavior where one would have plug+unplug or none or disable acpi pci hotplug globally for VM. > > Ani > >
On Tue, 24 Mar 2020 13:29:10 +0000 Ani Sinha <ani.sinha@nutanix.com> wrote: > On Mar 24, 2020, at 4:47 PM, Igor Mammedov <imammedo@redhat.com<mailto:imammedo@redhat.com>> wrote: > > you probably have APCI hotplug for bridges enabled (which applies to > all cold-plugged bridges). > You can disable it by setting following property > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off > then it should fall back to SHPC which are are tying to disable > > Ok I patched to change the default : > > --- > hw/acpi/piix4.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index d706360..db888cb 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -675,7 +675,7 @@ static Property piix4_pm_properties[] = { > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 1), > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2), > DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState, > - use_acpi_pci_hotplug, true), > + use_acpi_pci_hotplug, false), > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, > acpi_memory_hotplug.is_enabled, true), > DEFINE_PROP_END_OF_LIST(), > -- > 1.9.4 > > This seems to work but now Windows is giving an option to eject pci bridges. Clicking it does not do anything. I wonder if we can disable that as well. AFAIK hot unplugging bridges would not take effect until ACPI tables are updated and the OS kernel re-reads the updated tables. See https://bugzilla.redhat.com/show_bug.cgi?id=1339875 if you are interested in hacking ACPI PCI hotplug more, look at how build_append_pci_bus_devices() works > > Ani > [cid:809BEDB4-EEF6-4197-A083-4AF2E9999CEB]
> On Mar 24, 2020, at 7:47 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > if you are interested in hacking ACPI PCI hotplug more, look at how build_append_pci_bus_devices() works Cool. Will keep digging while stuck at home :-) A
> On Mar 24, 2020, at 7:39 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > disable acpi pci hotplug globally for VM This sounds like a good idea for our needs. A
Hi Igor: > On Mar 24, 2020, at 7:47 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > if you are interested in hacking ACPI PCI hotplug more, look at how build_append_pci_bus_devices() works I am trying to disable hot unplug of a device on PCI-PCI bridge without removing the capability to hot plug. Will it be enough to disable the following lines of code? + method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); + aml_append(method, + aml_call2("PCEJ", aml_name("BSEL"), + aml_name("_SUN")) + ); + aml_append(dev, method); I have sent a patch. Please provide comments. https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg02773.html Thanks Ani
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index b2d861d..e706d49 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -58,7 +58,7 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp) pci_bridge_initfn(dev, TYPE_PCI_BUS); - if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) { + if (0) {//bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) { dev->config[PCI_INTERRUPT_PIN] = 0x1; memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev)); @@ -161,7 +161,7 @@ static Property pci_bridge_dev_properties[] = { DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi, ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags, - PCI_BRIDGE_DEV_F_SHPC_REQ, true), + PCI_BRIDGE_DEV_F_SHPC_REQ, false), DEFINE_PROP_END_OF_LIST(), }; @@ -181,7 +181,7 @@ static const VMStateDescription pci_bridge_dev_vmstate = { VMSTATE_END_OF_LIST() } }; - +#if 0 static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { @@ -208,12 +208,12 @@ static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev, } shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp); } - +#endif static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); + //HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); k->realize = pci_bridge_dev_realize; k->exit = pci_bridge_dev_exitfn; @@ -227,8 +227,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) dc->props = pci_bridge_dev_properties; dc->vmsd = &pci_bridge_dev_vmstate; set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); - hc->plug = pci_bridge_dev_hotplug_cb; - hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb; + //hc->plug = pci_bridge_dev_hotplug_cb; + //hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb; } static const TypeInfo pci_bridge_dev_info = { @@ -238,7 +238,7 @@ static const TypeInfo pci_bridge_dev_info = { .class_init = pci_bridge_dev_class_init, .instance_finalize = pci_bridge_dev_instance_finalize, .interfaces = (InterfaceInfo[]) { - { TYPE_HOTPLUG_HANDLER }, + //{ TYPE_HOTPLUG_HANDLER }, { INTERFACE_CONVENTIONAL_PCI_DEVICE }, { } }
Hi All : I have been playing with Qemu trying to disable hot-unplug capability for conventional PCI. I have discussed this briefly on IRC and the plan is to have an option on the pci-pci bridge that would disable SHPC and ACPI hotplug capability for all the slots on that bus. I am _not_ trying to implement a per slot capability for conventional PCI as was previously posted for PCIE slots in this patch : https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg01833.html (we do not need this atm). I am following the conversations which happened few weeks back here: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00985.html To that end, I have been experimenting with Qemu using the patch I attach below. I have attached the virtio balloon driver with bus 1 which is attached to the pci bridge. Following is the libvirt 4.5 xml snippet which I am using: <controller type='pci' index='0' model='pci-root'> <alias name='pci.0'/> </controller> <controller type='pci' index='1' model='pci-bridge'> <model name='pci-bridge'/> <target chassisNr='1'/> <alias name='pci.1'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </controller> <memballoon model='virtio'> <stats period='30'/> <alias name='balloon0'/> <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> </memballoon> I am using a windows guest and from the guest I can see that the balloon driver is indeed attached to the pci bridge (see attached screenshot). I still find Windows giving me an option to hot eject the pci balloon driver. So what am I doing wrong here? [cid:F2407B5B-BBB1-4A0C-91C4-975692E3BDE1] The Qemu patch I am experimenting with (which is currently a hack) is attached below. It is based off Qemu 2.12 and not the latest mainline. --- hw/pci-bridge/pci_bridge_dev.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) -- 1.9.4