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 mention below. I have attached the virtio balloon driver with bus 1 which is attached to the pci bridge.

   <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:87E6C01C-8B8C-4E24-AC4B-C0D746C2F605]

The patch I am experimenting with (which is currently a hack) is attached below. It is based off libvirt 4.5 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, Mar 24, 2020 at 07:24:39AM +0000, Ani Sinha 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
> 
> To that end, I have been experimenting with Qemu using the patch I mention below. I have attached the virtio balloon driver with bus 1 which is attached to the pci bridge.
> 
>    <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:87E6C01C-8B8C-4E24-AC4B-C0D746C2F605]
> 
> The patch I am experimenting with (which is currently a hack) is
> attached below. It is based off libvirt 4.5 and not the latest mainline.

This is a patch against QEMU actually.  I doubt many on the libvirt mailing
list are familiar with the PCI / ACPI needs around hotplug, so it is probably
best to re-send this patch to the QEMU mailing list and ask questions there.

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



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

> On Mar 24, 2020, at 2:53 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> This is a patch against QEMU actually. 

Oops! Yes sorry, not sure what I was thinking. Yes this is Qemu. I will resend the email in Qemu-devel mailing list.

ani