[Qemu-devel] [PATCH] qom: fix refcounting in object_property_del_child()

[Qemu-devel] [PATCH] qom: fix refcounting in object_property_del_child()

[Amos Kong]

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

object_ref() is called in object_property_add_child(),
but we don't unref it in object_property_del_child().

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

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qom/object.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index e721fc2..9da6b59 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -320,6 +320,7 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
     QTAILQ_FOREACH(prop, &obj->properties, node) {
         if (strstart(prop->type, "child<", NULL) && prop->opaque == child) {
             object_property_del(obj, prop->name, errp);
+            object_unref(child);
             break;
         }
     }

Re: [Qemu-devel] [PATCH] qom: fix refcounting in object_property_del_child()

[Paolo Bonzini]

Il 11/05/2012 04:15, Amos Kong ha scritto:
> Start VM with 8 multiple-function block devs, hot-removing
> those block devs by 'device_del ...' would cause qemu abort.
> 
> object_ref() is called in object_property_add_child(),
> but we don't unref it in object_property_del_child().
> 
> | (qemu) device_del virti0-0-0
> | (qemu) **
> | ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qom/object.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index e721fc2..9da6b59 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -320,6 +320,7 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>      QTAILQ_FOREACH(prop, &obj->properties, node) {
>          if (strstart(prop->type, "child<", NULL) && prop->opaque == child) {
>              object_property_del(obj, prop->name, errp);
> +            object_unref(child);

This should be called by object_finalize_child_property instead, can you
check why this is not the case?

Paolo

>              break;
>          }
>      }
> 

Re: [Qemu-devel] [PATCH] qom: fix refcounting in object_property_del_child()

[Amos Kong]

On 05/11/2012 02:42 PM, Paolo Bonzini wrote:
> Il 11/05/2012 04:15, Amos Kong ha scritto:
>> Start VM with 8 multiple-function block devs, hot-removing
>> those block devs by 'device_del ...' would cause qemu abort.
>>
>> object_ref() is called in object_property_add_child(),
>> but we don't unref it in object_property_del_child().
>>
>> | (qemu) device_del virti0-0-0
>> | (qemu) **
>> | ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)
>>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>>  qom/object.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index e721fc2..9da6b59 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -320,6 +320,7 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>>      QTAILQ_FOREACH(prop, &obj->properties, node) {
>>          if (strstart(prop->type, "child<", NULL) && prop->opaque == child) {
>>              object_property_del(obj, prop->name, errp);
>> +            object_unref(child);
> 
> This should be called by object_finalize_child_property instead, can you
> check why this is not the case?

Yes, original ref/unref are right.
I will post another patch to fix this issue.


NAK this patch.


> Paolo

Thanks!

-- 
			Amos.

[Qemu-devel] [PATCH] pci: unplug all devs of same slot once

[Amos Kong]

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

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,

Re: [Qemu-devel] [PATCH] pci: unplug all devs of same slot once

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4907 bytes --]

Re: [Qemu-devel] [PATCH] pci: unplug all devs of same slot once

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] [PATCH] pci: unplug all devs of same slot once

[Paolo Bonzini]

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

[Qemu-devel] [PATCH v3] pci: call object_unparent() before free_qdev()

[Amos Kong]

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)

It's a regression introduced by commit 57c9fafe

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.

free_qdev() are called for all functions in slot,
but unparent_delete() is only called for one
function.

---
aliguori has a better resolution, better to do it in 1.2

v2: fix warning: too many arguments for format
v3: move object_unparent() to acpi_piix_eject_slot()

Signed-off-by: Amos Kong <kongjianjun@gmail.com>
---
 hw/acpi_piix4.c |    1 +
 hw/pci.c        |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 585da4e..0345490 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -299,6 +299,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
+                object_unparent(OBJECT(dev));
                 qdev_free(qdev);
             }
         }
diff --git a/hw/pci.c b/hw/pci.c
index b706e69..c1ebdde 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1527,7 +1527,6 @@ static int pci_unplug_device(DeviceState *qdev)
         qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
         return -1;
     }
-    object_unparent(OBJECT(dev));
     return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
                              PCI_HOTPLUG_DISABLED);
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH v3] pci: call object_unparent() before free_qdev()

[Michael S. Tsirkin]

On Sun, May 20, 2012 at 05:57:45PM +0800, Amos Kong wrote:
> 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)
> 
> It's a regression introduced by commit 57c9fafe
> 
> 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.
> 
> free_qdev() are called for all functions in slot,
> but unparent_delete() is only called for one
> function.
> 
> ---
> aliguori has a better resolution, better to do it in 1.2
> 
> v2: fix warning: too many arguments for format
> v3: move object_unparent() to acpi_piix_eject_slot()
> 
> Signed-off-by: Amos Kong <kongjianjun@gmail.com>

commit is mangled up a bit.  It should be:

subject: xxxx

commit log

Signed-off-by: XXXX

--- 

Versioning info

diff


No need to repost just we that. But we also need to update other pci
hotplug users: hw//shpc.c hw//pcie.c
Not sure about pci-hotplug.c (calls qdev_free on error
handling) - add a virtio blk function with wrong drive
parameter using pci_add and see. Anything else?

> ---
>  hw/acpi_piix4.c |    1 +
>  hw/pci.c        |    1 -
>  2 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 585da4e..0345490 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -299,6 +299,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
>              if (pc->no_hotplug) {
>                  slot_free = false;
>              } else {
> +                object_unparent(OBJECT(dev));
>                  qdev_free(qdev);
>              }
>          }
> diff --git a/hw/pci.c b/hw/pci.c
> index b706e69..c1ebdde 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1527,7 +1527,6 @@ static int pci_unplug_device(DeviceState *qdev)
>          qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
>          return -1;
>      }
> -    object_unparent(OBJECT(dev));
>      return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
>                               PCI_HOTPLUG_DISABLED);
>  }
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH v3] pci: call object_unparent() before free_qdev()

[Jason Baron]

On Sun, May 20, 2012 at 05:57:45PM +0800, Amos Kong wrote:
> 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)
> 
> It's a regression introduced by commit 57c9fafe
> 

I found a similar assertion where the parent reference isn't cleared,
doing:

(qemu) device_add pci-bridge

in the monitor. I posted for patches for it under:

Subject: [Qemu-devel] [PATCH 0/2] pci: hotplug bridge fixes

It's still an issue with the current tree.

Thanks,

-Jason

Re: [Qemu-devel] [PATCH v3] pci: call object_unparent() before free_qdev()

[Michael S. Tsirkin]

On Mon, Jun 04, 2012 at 04:15:56PM -0400, Jason Baron wrote:
> On Sun, May 20, 2012 at 05:57:45PM +0800, Amos Kong wrote:
> > 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)
> > 
> > It's a regression introduced by commit 57c9fafe
> > 
> 
> I found a similar assertion where the parent reference isn't cleared,
> doing:
> 
> (qemu) device_add pci-bridge
> 
> in the monitor. I posted for patches for it under:
> 
> Subject: [Qemu-devel] [PATCH 0/2] pci: hotplug bridge fixes
> 
> It's still an issue with the current tree.
> 
> Thanks,
> 
> -Jason

I pushed your patches on my tree pci branch, care to test
there?

Re: [Qemu-devel] [PATCH v3] pci: call object_unparent() before free_qdev()

[Jason Baron]

On Tue, Jun 05, 2012 at 12:52:02AM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 04, 2012 at 04:15:56PM -0400, Jason Baron wrote:
> > On Sun, May 20, 2012 at 05:57:45PM +0800, Amos Kong wrote:
> > > 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)
> > > 
> > > It's a regression introduced by commit 57c9fafe
> > > 
> > 
> > I found a similar assertion where the parent reference isn't cleared,
> > doing:
> > 
> > (qemu) device_add pci-bridge
> > 
> > in the monitor. I posted for patches for it under:
> > 
> > Subject: [Qemu-devel] [PATCH 0/2] pci: hotplug bridge fixes
> > 
> > It's still an issue with the current tree.
> > 
> > Thanks,
> > 
> > -Jason
> 
> I pushed your patches on my tree pci branch, care to test
> there?
> 

Yes, fixes are confirmed.

Test case is quite simple:

(qemu) device_add pci_bridge

causes segfault without the 2 patches applied.

Thanks,

-Jason