Patchwork pci: unplug all devs of same slot once

login
register
mail settings
Submitter Amos Kong
Date May 11, 2012, 2:57 p.m.
Message ID <20120511145725.16518.77110.stgit@t>
Download mbox | patch
Permalink /patch/158531/
State New
Headers show

Comments

Amos Kong - May 11, 2012, 2:57 p.m.
The whole PCI slot should be removed once. Currently only one func
is cleaned in pci_unplug_device(), if you try to remove a single
func by monitor cmd.

Start VM with 8 multiple-function block devs, hot-removing
those block devs by 'device_del ...' would cause qemu abort.

| (qemu) device_del virti0-0-0
| (qemu) **
|ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)

Execute 'device_del $blkid' in monitor
 \_handle_user_command()
    \_qmp_device_del()
       \_qdev_unplug()
          \_pci_unplug_device()
               | //only one obj(func) is unpluged
               v //need process funcs here
   object_unparent()
    \_object_finalize_child_property()

Guest sets pci dev by ioport write (eject from acpi)
 \_kvm_handle_io()
    \_pciej_write()
      \_acpi_piix_eject_slot()
           |
           v  //all qdevs(funcs) will be free
 QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
     PCIDevice *dev = PCI_DEVICE(qdev);
     if (PCI_SLOT(dev->devfn) == slot) {
         qdev_free()

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/pci.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)
Jianjun Kong - May 13, 2012, 12:40 a.m.
On Fri, May 11, 2012 at 10:57 PM, Amos Kong <akong@redhat.com> wrote:

> The whole PCI slot should be removed once. Currently only one func
> is cleaned in pci_unplug_device(), if you try to remove a single
> func by monitor cmd.
>
> Start VM with 8 multiple-function block devs, hot-removing
> those block devs by 'device_del ...' would cause qemu abort.
>
> | (qemu) device_del virti0-0-0
> | (qemu) **
> |ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)
>
> Execute 'device_del $blkid' in monitor
>  \_handle_user_command()
>    \_qmp_device_del()
>       \_qdev_unplug()
>          \_pci_unplug_device()
>               | //only one obj(func) is unpluged
>               v //need process funcs here
>   object_unparent()
>    \_object_finalize_child_property()
>
> Guest sets pci dev by ioport write (eject from acpi)
>  \_kvm_handle_io()
>    \_pciej_write()
>      \_acpi_piix_eject_slot()
>           |
>           v  //all qdevs(funcs) will be free
>  QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
>     PCIDevice *dev = PCI_DEVICE(qdev);
>     if (PCI_SLOT(dev->devfn) == slot) {
>         qdev_free()
>


This is a regression bug, it's caused by this
commit: 57c9fafe0f759c9f1efa5451662b3627f9bb95e0
Before commit this commit, we free object in qdev_free(), as you can see in
above analysis, qdev_free() is called for all the functions.
But after applied this patch, we only release object of one function
in pci_unplug_device().


commit 57c9fafe0f759c9f1efa5451662b3627f9bb95e0
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Mon Jan 30 08:55:55 2012 -0600

    qom: move properties from qdev to object

    This is mostly code movement although not entirely.  This makes
properties part
    of the Object base class which means that we can now start using Object
in a
    meaningful way outside of qdev.

    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>



>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/pci.c |   34 ++++++++++++++++++++++++++--------
>  1 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index b706e69..960cf53 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1520,16 +1520,34 @@ static int pci_qdev_init(DeviceState *qdev)
>
>  static int pci_unplug_device(DeviceState *qdev)
>  {
> -    PCIDevice *dev = PCI_DEVICE(qdev);
> -    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> +    DeviceState *dev, *next;
> +    PCIDevice *cur;
> +    int ret, slot = PCI_SLOT(PCI_DEVICE(qdev)->devfn);
> +    BusState *bus = qdev->parent_bus;
> +
> +    ret = 0;
> +    QTAILQ_FOREACH_SAFE(dev, &bus->children, sibling, next) {
> +        cur = PCI_DEVICE(dev);
> +
> +        if (PCI_SLOT(cur->devfn) == slot) {
> +            PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(cur);
> +            if (pc->no_hotplug) {
> +                qerror_report(QERR_DEVICE_NO_HOTPLUG,
> +                              object_get_typename(OBJECT(dev)));
> +                return -1;
> +            }
>
> -    if (pc->no_hotplug) {
> -        qerror_report(QERR_DEVICE_NO_HOTPLUG,
> object_get_typename(OBJECT(dev)));
> -        return -1;
> +            object_unparent(OBJECT(cur));
> +            ret = cur->bus->hotplug(cur->bus->hotplug_qdev, cur,
> +                                    PCI_HOTPLUG_DISABLED);
> +            if (ret < 0) {
> +                qerror_report(QERR_UNDEFINED_ERROR,
> +                              object_get_typename(OBJECT(dev)));
> +                break;
> +            }
> +        }
>     }
> -    object_unparent(OBJECT(dev));
> -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> -                             PCI_HOTPLUG_DISABLED);
> +    return ret;
>  }
>
>  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool
> multifunction,
>
>
>
Michael S. Tsirkin - May 13, 2012, 10:54 a.m.
On Fri, May 11, 2012 at 10:57:25PM +0800, Amos Kong wrote:
> The whole PCI slot should be removed once. Currently only one func
> is cleaned in pci_unplug_device(), if you try to remove a single
> func by monitor cmd.
> 
> Start VM with 8 multiple-function block devs, hot-removing
> those block devs by 'device_del ...' would cause qemu abort.
> 
> | (qemu) device_del virti0-0-0
> | (qemu) **
> |ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)
> 
> Execute 'device_del $blkid' in monitor
>  \_handle_user_command()
>     \_qmp_device_del()
>        \_qdev_unplug()
>           \_pci_unplug_device()
>                | //only one obj(func) is unpluged
>                v //need process funcs here
>    object_unparent()
>     \_object_finalize_child_property()

This is the bug IMO. PCI device delete request
through monitor simply notifies guest. It should not unparent
the object or do anything else.

> Guest sets pci dev by ioport write (eject from acpi)
>  \_kvm_handle_io()
>     \_pciej_write()
>       \_acpi_piix_eject_slot()
>            |
>            v  //all qdevs(funcs) will be free
>  QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
>      PCIDevice *dev = PCI_DEVICE(qdev);
>      if (PCI_SLOT(dev->devfn) == slot) {
>          qdev_free()
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---

This was done as part of 57c9fafe0f759c9f1efa5451662b3627f9bb95e0.
Should we just call object_unparent before qdev_free?
Anthony?
Paolo Bonzini - May 14, 2012, 7:55 a.m.
Il 13/05/2012 12:54, Michael S. Tsirkin ha scritto:
> This was done as part of 57c9fafe0f759c9f1efa5451662b3627f9bb95e0.
> Should we just call object_unparent before qdev_free?
> Anthony?

Or just call object_unparent in object_delete?

Paolo

Patch

diff --git a/hw/pci.c b/hw/pci.c
index b706e69..960cf53 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1520,16 +1520,34 @@  static int pci_qdev_init(DeviceState *qdev)
 
 static int pci_unplug_device(DeviceState *qdev)
 {
-    PCIDevice *dev = PCI_DEVICE(qdev);
-    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+    DeviceState *dev, *next;
+    PCIDevice *cur;
+    int ret, slot = PCI_SLOT(PCI_DEVICE(qdev)->devfn);
+    BusState *bus = qdev->parent_bus;
+
+    ret = 0;
+    QTAILQ_FOREACH_SAFE(dev, &bus->children, sibling, next) {
+        cur = PCI_DEVICE(dev);
+
+        if (PCI_SLOT(cur->devfn) == slot) {
+            PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(cur);
+            if (pc->no_hotplug) {
+                qerror_report(QERR_DEVICE_NO_HOTPLUG,
+                              object_get_typename(OBJECT(dev)));
+                return -1;
+            }
 
-    if (pc->no_hotplug) {
-        qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
-        return -1;
+            object_unparent(OBJECT(cur));
+            ret = cur->bus->hotplug(cur->bus->hotplug_qdev, cur,
+                                    PCI_HOTPLUG_DISABLED);
+            if (ret < 0) {
+                qerror_report(QERR_UNDEFINED_ERROR,
+                              object_get_typename(OBJECT(dev)));
+                break;
+            }
+        }
     }
-    object_unparent(OBJECT(dev));
-    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
-                             PCI_HOTPLUG_DISABLED);
+    return ret;
 }
 
 PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,