[Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[Anthony Liguori]

Right now, you need to pair up object_new with object_delete.  This is
impractical when using reference counting because we would like to ensure that
object_unref() also frees memory when needed.

The first few patches fix this problem by introducing a release callback so
that objects that need special release behavior (i.e. g_free) can do that.

Since link and child properties all hold references, in order to actually free
an object, we need to break those links.  User created devices end up as
children of a container.  But child properties cannot be removed which means
there's no obvious way to remove the reference and ultimately free the object.

We introduce the concept of "nullable child" properties to solve this.  This is
a child property that can be broken by writing NULL to the child link.  Today
we set all /peripheral* children to be nullable so that they can be deleted by
management tools.

In terms of modeling hotplug, we represent unplug by removing the object from
the parent bus.  We need to register a notifier for when this happens so that
we can also remove the parent's child property to ultimately release the object.

Putting it all together, we have:

1) qmp_device_del will issue a callback to a device.  The default callback will
   do a forced eject (which means writing NULL to the parent_bus link).

2) PCI hotplug is a bit more sophisticated in that it waits for the guest to
   do the ejection.

3) qmp_device_del will register an eject notifier such that the device gets
   completely removed.

There's a slightly change in behavior here.  A device is not automatically
destroyed based on a guest initiated eject.  A management tool must explicitly
break the parent's link to the child in order for the device to disappear
completely.  device_del behaves exactly as it does today though.

This is an RFC.  I've tested the series quite a lot (it was hard to get the
reference counting right) but not enough to apply.  I also don't think the
series is quite split right and may not bisect cleanly.

I also want to write up a document describing object life cycle since admittedly
the above is probably not that easy to follow.

I wanted to share this now though because it works and I think the concepts are
right.

[Qemu-devel] [PATCH 1/9] savevm: don't rely on paths if we can store a DeviceState object

[Anthony Liguori]

Paths break during tear down which can result in odd behavior since we do path
based lookup during unregister (which happens at tear down).

For devices, just store the DeviceState directly and use that.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 savevm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index c7fe283..bb6494b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1177,6 +1177,7 @@ typedef struct SaveStateEntry {
     CompatEntry *compat;
     int no_migrate;
     int is_ram;
+    DeviceState *dev;
 } SaveStateEntry;
 
 
@@ -1235,6 +1236,7 @@ int register_savevm_live(DeviceState *dev,
     se->opaque = opaque;
     se->vmsd = NULL;
     se->no_migrate = 0;
+    se->dev = dev;
     /* if this is a live_savem then set is_ram */
     if (ops->save_live_setup != NULL) {
         se->is_ram = 1;
@@ -1298,7 +1300,8 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
     pstrcat(id, sizeof(id), idstr);
 
     QTAILQ_FOREACH_SAFE(se, &savevm_handlers, entry, new_se) {
-        if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
+        if ((dev && se->dev == dev) ||
+            (strcmp(se->idstr, id) == 0 && se->opaque == opaque)) {
             QTAILQ_REMOVE(&savevm_handlers, se, entry);
             if (se->compat) {
                 g_free(se->compat);
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/9] object: automatically free objects based on a release function

[Anthony Liguori]

Now object_delete() simply has the semantics of unref'ing an object and
unparenting it.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 include/qemu/object.h |    5 +++++
 qom/object.c          |   16 +++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index cc75fee..487adcd 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -242,6 +242,8 @@ struct ObjectClass
     GSList *interfaces;
 };
 
+typedef void (ObjectReleaseFunc)(Object *obj);
+
 /**
  * Object:
  *
@@ -264,6 +266,7 @@ struct Object
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
+    ObjectReleaseFunc *release;
 };
 
 /**
@@ -464,6 +467,8 @@ Object *object_new_with_type(Type type);
  */
 void object_delete(Object *obj);
 
+void object_set_release_func(Object *obj, ObjectReleaseFunc *func);
+
 /**
  * object_initialize_with_type:
  * @obj: A pointer to the memory to be used for the object.
diff --git a/qom/object.c b/qom/object.c
index e3e9242..44135c3 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -384,6 +384,20 @@ void object_finalize(void *data)
     object_property_del_all(obj);
 
     g_assert(obj->ref == 0);
+
+    if (obj->release) {
+        obj->release(obj);
+    }
+}
+
+void object_set_release_func(Object *obj, ObjectReleaseFunc *func)
+{
+    obj->release = func;
+}
+
+static void object_release_func(Object *obj)
+{
+    g_free(obj);
 }
 
 Object *object_new_with_type(Type type)
@@ -395,6 +409,7 @@ Object *object_new_with_type(Type type)
 
     obj = g_malloc(type->instance_size);
     object_initialize_with_type(obj, type);
+    object_set_release_func(obj, object_release_func);
     object_ref(obj);
 
     return obj;
@@ -412,7 +427,6 @@ void object_delete(Object *obj)
     object_unparent(obj);
     g_assert(obj->ref == 1);
     object_unref(obj);
-    g_free(obj);
 }
 
 Object *object_dynamic_cast(Object *obj, const char *typename)
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 2/9] object: automatically free objects based on a release function

[Andreas Färber]

Am 26.08.2012 17:51, schrieb Anthony Liguori:
> Now object_delete() simply has the semantics of unref'ing an object and
> unparenting it.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Acked-by: Andreas Färber <afaerber@suse.de>

/-F

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 3/9] qbus: remove glib_allocated/qom_allocated and use release hook to free memory

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pci.c    |    7 ++++++-
 hw/qdev.c   |   15 ---------------
 hw/qdev.h   |    7 -------
 hw/sysbus.c |    7 ++++++-
 4 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 4d95984..437af70 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -292,6 +292,11 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
 }
 
+static void pci_bus_release(Object *obj)
+{
+    g_free(obj);
+}
+
 PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
@@ -300,9 +305,9 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
     PCIBus *bus;
 
     bus = g_malloc0(sizeof(*bus));
-    bus->qbus.glib_allocated = true;
     pci_bus_new_inplace(bus, parent, name, address_space_mem,
                         address_space_io, devfn_min);
+    object_set_release_func(OBJECT(bus), pci_bus_release);
     return bus;
 }
 
diff --git a/hw/qdev.c b/hw/qdev.c
index b5a52ac..6b61daa 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -459,8 +459,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     BusState *bus;
 
     bus = BUS(object_new(typename));
-    bus->qom_allocated = true;
-
     bus->parent = parent;
     bus->name = name ? g_strdup(name) : NULL;
     qbus_realize(bus);
@@ -468,18 +466,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     return bus;
 }
 
-void qbus_free(BusState *bus)
-{
-    if (bus->qom_allocated) {
-        object_delete(OBJECT(bus));
-    } else {
-        object_finalize(OBJECT(bus));
-        if (bus->glib_allocated) {
-            g_free(bus);
-        }
-    }
-}
-
 static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
 {
     BusClass *bc = BUS_GET_CLASS(bus);
@@ -698,7 +684,6 @@ static void device_finalize(Object *obj)
     if (dev->state == DEV_STATE_INITIALIZED) {
         while (dev->num_child_bus) {
             bus = QLIST_FIRST(&dev->child_bus);
-            qbus_free(bus);
         }
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
diff --git a/hw/qdev.h b/hw/qdev.h
index d699194..3561e3a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -106,17 +106,12 @@ typedef struct BusChild {
 
 /**
  * BusState:
- * @qom_allocated: Indicates whether the object was allocated by QOM.
- * @glib_allocated: Indicates whether the object was initialized in-place
- * yet is expected to be freed with g_free().
  */
 struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
     int allow_hotplug;
-    bool qom_allocated;
-    bool glib_allocated;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
@@ -201,8 +196,6 @@ int qdev_walk_children(DeviceState *dev, qdev_walkerfn *devfn,
 void qdev_reset_all(DeviceState *dev);
 void qbus_reset_all_fn(void *opaque);
 
-void qbus_free(BusState *bus);
-
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
 /* This should go away once we get rid of the NULL bus hack */
diff --git a/hw/sysbus.c b/hw/sysbus.c
index 9d8b1ea..8305337 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -267,6 +267,11 @@ static TypeInfo sysbus_device_type_info = {
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
 
+static void main_system_bus_release(Object *obj)
+{
+    g_free(obj);
+}
+
 static void main_system_bus_create(void)
 {
     /* assign main_system_bus before qbus_create_inplace()
@@ -274,7 +279,7 @@ static void main_system_bus_create(void)
     main_system_bus = g_malloc0(system_bus_info.instance_size);
     qbus_create_inplace(main_system_bus, TYPE_SYSTEM_BUS, NULL,
                         "main-system-bus");
-    main_system_bus->glib_allocated = true;
+    object_set_release_func(OBJECT(main_system_bus), main_system_bus_release);
     object_property_add_child(container_get(qdev_get_machine(),
                                             "/unattached"),
                               "sysbus", OBJECT(main_system_bus), NULL);
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 3/9] qbus: remove glib_allocated/qom_allocated and use release hook to free memory

[Andreas Färber]

Am 26.08.2012 17:51, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

That's a really nice solution for cleaning this up, thanks!

Acked-by: Andreas Färber <afaerber@suse.de>

However one conceptional detail...

> ---
>  hw/pci.c    |    7 ++++++-
>  hw/qdev.c   |   15 ---------------
>  hw/qdev.h   |    7 -------
>  hw/sysbus.c |    7 ++++++-
>  4 files changed, 12 insertions(+), 24 deletions(-)
[...]
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b5a52ac..6b61daa 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
[...]
> @@ -468,18 +466,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      return bus;
>  }
>  
> -void qbus_free(BusState *bus)
> -{
> -    if (bus->qom_allocated) {
> -        object_delete(OBJECT(bus));
> -    } else {
> -        object_finalize(OBJECT(bus));
> -        if (bus->glib_allocated) {
> -            g_free(bus);
> -        }
> -    }
> -}
> -
>  static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
>  {
>      BusClass *bc = BUS_GET_CLASS(bus);
> @@ -698,7 +684,6 @@ static void device_finalize(Object *obj)
>      if (dev->state == DEV_STATE_INITIALIZED) {
>          while (dev->num_child_bus) {
>              bus = QLIST_FIRST(&dev->child_bus);
> -            qbus_free(bus);
>          }
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);

I wonder how this is gonna work: The device used to be in charge of
tearing down its bus children ... now it neither deletes nor finalizes
nor unrefs? Is the while loop even still needed?

Wouldn't the busses still have the device as parent, referencing it,
blocking device_finalize?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/9] qbus: remove glib_allocated/qom_allocated and use release hook to free memory

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 26.08.2012 17:51, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> That's a really nice solution for cleaning this up, thanks!
>
> Acked-by: Andreas Färber <afaerber@suse.de>
>
> However one conceptional detail...
>
>> ---
>>  hw/pci.c    |    7 ++++++-
>>  hw/qdev.c   |   15 ---------------
>>  hw/qdev.h   |    7 -------
>>  hw/sysbus.c |    7 ++++++-
>>  4 files changed, 12 insertions(+), 24 deletions(-)
> [...]
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index b5a52ac..6b61daa 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
> [...]
>> @@ -468,18 +466,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>>      return bus;
>>  }
>>  
>> -void qbus_free(BusState *bus)
>> -{
>> -    if (bus->qom_allocated) {
>> -        object_delete(OBJECT(bus));
>> -    } else {
>> -        object_finalize(OBJECT(bus));
>> -        if (bus->glib_allocated) {
>> -            g_free(bus);
>> -        }
>> -    }
>> -}
>> -
>>  static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
>>  {
>>      BusClass *bc = BUS_GET_CLASS(bus);
>> @@ -698,7 +684,6 @@ static void device_finalize(Object *obj)
>>      if (dev->state == DEV_STATE_INITIALIZED) {
>>          while (dev->num_child_bus) {
>>              bus = QLIST_FIRST(&dev->child_bus);
>> -            qbus_free(bus);
>>          }
>>          if (qdev_get_vmsd(dev)) {
>>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>
> I wonder how this is gonna work: The device used to be in charge of
> tearing down its bus children ... now it neither deletes nor finalizes
> nor unrefs? Is the while loop even still needed?
>
> Wouldn't the busses still have the device as parent, referencing it,
> blocking device_finalize?

This has never been right..  Just because a controller goes away, it
doesn't mean that the devices ought to go away too.

There are different types of "remove" so let's consider each.

1) Guest visible eject: if a controller is ejected, then the guest will
   obviously see everything behind it get removed too.  This is an
   emulation detail, not a QOM thing.

2) Final deletion: this only happens when all references go away.  If
   you eject a controller but there are still children that reference
   it, the controller won't go away.  You actually need to delete each
   individual disk (or whatever is behind it) in order to break the
   reference counting.

The eject notifier could walk the full bus and attempt to break the
connections but honestly, I'd much prefer that we deprecate the current
device_del interface and just do everything through QOM properties.
That would mean manually deleting all of the devices behind the bus if
that's really what you wanted to do.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/9] qbus: remove glib_allocated/qom_allocated and use release hook to free memory

[Andreas Färber]

Am 27.08.2012 16:22, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> I wonder how this is gonna work: The device used to be in charge of
>> tearing down its bus children ... now it neither deletes nor finalizes
>> nor unrefs? Is the while loop even still needed?
>>
>> Wouldn't the busses still have the device as parent, referencing it,
>> blocking device_finalize?
> 
> This has never been right..  Just because a controller goes away, it
> doesn't mean that the devices ought to go away too.
> 
> There are different types of "remove" so let's consider each.
> 
> 1) Guest visible eject: if a controller is ejected, then the guest will
>    obviously see everything behind it get removed too.  This is an
>    emulation detail, not a QOM thing.
> 
> 2) Final deletion: this only happens when all references go away.  If
>    you eject a controller but there are still children that reference
>    it, the controller won't go away.  You actually need to delete each
>    individual disk (or whatever is behind it) in order to break the
>    reference counting.
> 
> The eject notifier could walk the full bus and attempt to break the
> connections but honestly, I'd much prefer that we deprecate the current
> device_del interface and just do everything through QOM properties.
> That would mean manually deleting all of the devices behind the bus if
> that's really what you wanted to do.

I think we're talking about different scenarios here...

I was thinking

PCIHostState has-a PCIBus

(not PCIBus has-a PCIDevice) and final deletion.

In that case I would expect that it must be guaranteed that the device
that created the bus has access to the bus until it destroys it. But
IIUC the PCIHostState, once unparented from its SysBus (bad example!),
has a refcount of 1 (its PCIBus) thereby not being finalized?

I do understand your concept of refcounting matches what Java, .NET,
etc. do for objects but combined with the new QBus I feel this is
blurring the encapsulations and expected semantics of the device-centric
functions we have. To me the uninitfn means "the whole object goes away"
and is incompatible with "part of its children may stay behind if there
are still stray references to them"... we can no longer properly access
them then, only devices have canonical paths, so we'd risk piling up
garbage at runtime.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 4/9] object: remove object_finalize

[Anthony Liguori]

Callers should just use object_unref

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c             |    4 ----
 include/qemu/object.h |    9 ---------
 qom/object.c          |    2 +-
 3 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 6b61daa..fdee91f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -678,13 +678,9 @@ static void device_initfn(Object *obj)
 static void device_finalize(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
-    BusState *bus;
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     if (dev->state == DEV_STATE_INITIALIZED) {
-        while (dev->num_child_bus) {
-            bus = QLIST_FIRST(&dev->child_bus);
-        }
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
diff --git a/include/qemu/object.h b/include/qemu/object.h
index 487adcd..8bc9935 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -490,15 +490,6 @@ void object_initialize_with_type(void *data, Type type);
 void object_initialize(void *obj, const char *typename);
 
 /**
- * object_finalize:
- * @obj: The object to finalize.
- *
- * This function destroys and object without freeing the memory associated with
- * it.
- */
-void object_finalize(void *obj);
-
-/**
  * object_dynamic_cast:
  * @obj: The object to cast.
  * @typename: The @typename to cast to.
diff --git a/qom/object.c b/qom/object.c
index 44135c3..1144f79 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -375,7 +375,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
     }
 }
 
-void object_finalize(void *data)
+static void object_finalize(void *data)
 {
     Object *obj = data;
     TypeImpl *ti = obj->class->type;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 4/9] object: remove object_finalize

[Andreas Färber]

Am 26.08.2012 17:51, schrieb Anthony Liguori:
> Callers should just use object_unref
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c             |    4 ----
>  include/qemu/object.h |    9 ---------
>  qom/object.c          |    2 +-
>  3 files changed, 1 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6b61daa..fdee91f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -678,13 +678,9 @@ static void device_initfn(Object *obj)
>  static void device_finalize(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> -    BusState *bus;
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
>      if (dev->state == DEV_STATE_INITIALIZED) {
> -        while (dev->num_child_bus) {
> -            bus = QLIST_FIRST(&dev->child_bus);
> -        }
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }

This seems to answer my remark on 3/9, should've been squashed into that
one.

> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 487adcd..8bc9935 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -490,15 +490,6 @@ void object_initialize_with_type(void *data, Type type);
>  void object_initialize(void *obj, const char *typename);
>  
>  /**
> - * object_finalize:
> - * @obj: The object to finalize.
> - *
> - * This function destroys and object without freeing the memory associated with
> - * it.
> - */
> -void object_finalize(void *obj);
> -
> -/**
>   * object_dynamic_cast:
>   * @obj: The object to cast.
>   * @typename: The @typename to cast to.
> diff --git a/qom/object.c b/qom/object.c
> index 44135c3..1144f79 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -375,7 +375,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
>      }
>  }
>  
> -void object_finalize(void *data)
> +static void object_finalize(void *data)
>  {
>      Object *obj = data;
>      TypeImpl *ti = obj->class->type;

This is what I was referring to with breaking the encapsulation on 3/9:
When we have a PHB with embedded PCIDevice on its PCIBus, as
demonstrated with i440fx and prep_pci, then when doing object_delete()
on the whole thing I expect the main object's finalizer to call
object_finalize() on its embedded childs, forcing their uninit or an
assert if a programming error. Not just an unref that might or might not
finalize it.

If however finalize is called only at refcount 0 then who will unref the
self-created children? Finalize would never be called due to pending
references by its children...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/9] object: remove object_finalize

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 26.08.2012 17:51, schrieb Anthony Liguori:
>> Callers should just use object_unref
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/qdev.c             |    4 ----
>>  include/qemu/object.h |    9 ---------
>>  qom/object.c          |    2 +-
>>  3 files changed, 1 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 6b61daa..fdee91f 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -678,13 +678,9 @@ static void device_initfn(Object *obj)
>>  static void device_finalize(Object *obj)
>>  {
>>      DeviceState *dev = DEVICE(obj);
>> -    BusState *bus;
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>  
>>      if (dev->state == DEV_STATE_INITIALIZED) {
>> -        while (dev->num_child_bus) {
>> -            bus = QLIST_FIRST(&dev->child_bus);
>> -        }
>>          if (qdev_get_vmsd(dev)) {
>>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>>          }
>
> This seems to answer my remark on 3/9, should've been squashed into that
> one.
>
>> diff --git a/include/qemu/object.h b/include/qemu/object.h
>> index 487adcd..8bc9935 100644
>> --- a/include/qemu/object.h
>> +++ b/include/qemu/object.h
>> @@ -490,15 +490,6 @@ void object_initialize_with_type(void *data, Type type);
>>  void object_initialize(void *obj, const char *typename);
>>  
>>  /**
>> - * object_finalize:
>> - * @obj: The object to finalize.
>> - *
>> - * This function destroys and object without freeing the memory associated with
>> - * it.
>> - */
>> -void object_finalize(void *obj);
>> -
>> -/**
>>   * object_dynamic_cast:
>>   * @obj: The object to cast.
>>   * @typename: The @typename to cast to.
>> diff --git a/qom/object.c b/qom/object.c
>> index 44135c3..1144f79 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -375,7 +375,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
>>      }
>>  }
>>  
>> -void object_finalize(void *data)
>> +static void object_finalize(void *data)
>>  {
>>      Object *obj = data;
>>      TypeImpl *ti = obj->class->type;
>
> This is what I was referring to with breaking the encapsulation on 3/9:
> When we have a PHB with embedded PCIDevice on its PCIBus, as
> demonstrated with i440fx and prep_pci, then when doing object_delete()
> on the whole thing I expect the main object's finalizer to call
> object_finalize() on its embedded childs, forcing their uninit or an
> assert if a programming error. Not just an unref that might or might not
> finalize it.
>
> If however finalize is called only at refcount 0 then who will unref the
> self-created children? Finalize would never be called due to pending
> references by its children...

The refs are stored as properties and when properties are removed during
object destruction, those references are dropped.

That is what takes the refcount to 0.  We could add a
g_assert(child->ref == 1) before removing the property to catch
programming errors.

Regards,

Anthony Liguori

>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 5/9] object: add support for nullable child properties

[Anthony Liguori]

A nullable child can be detached from its parent by setting the property to
NULL.  This provides a mechanism for a management tool to delete certain
types of objects.

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

object: link<>: release reference on finalize, externalize getter/setter

Reported-by: Ping Fan
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 include/qemu/object.h |    9 +++++
 qom/object.c          |   78 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 77 insertions(+), 10 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 8bc9935..5bb62dd 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -907,6 +907,9 @@ Object *object_resolve_path_component(Object *parent, gchar *part);
 void object_property_add_child(Object *obj, const char *name,
                                Object *child, struct Error **errp);
 
+void object_property_add_nullable_child(Object *obj, const char *name,
+                                        Object *child, struct Error **errp);
+
 /**
  * object_property_add_link:
  * @obj: the object to add a property to
@@ -925,6 +928,12 @@ void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
                               struct Error **errp);
 
+void object_get_link_property(Object *obj, struct Visitor *v, void *opaque,
+                              const char *name, struct Error **errp);
+
+void object_set_link_property(Object *obj, struct Visitor *v, void *opaque,
+                              const char *name, struct Error **errp);
+
 /**
  * object_property_add_str:
  * @obj: the object to add a property to
diff --git a/qom/object.c b/qom/object.c
index 1144f79..61fa40f 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -744,8 +744,17 @@ char *object_property_get_str(Object *obj, const char *name,
 void object_property_set_link(Object *obj, Object *value,
                               const char *name, Error **errp)
 {
-    object_property_set_str(obj, object_get_canonical_path(value),
-                            name, errp);
+    char *str;
+
+    if (value == NULL) {
+        str = g_strdup("");
+    } else {
+        str = object_get_canonical_path(value);
+    }
+
+    object_property_set_str(obj, str, name, errp);
+
+    g_free(str);
 }
 
 Object *object_property_get_link(Object *obj, const char *name,
@@ -882,6 +891,27 @@ static void object_get_child_property(Object *obj, Visitor *v, void *opaque,
     g_free(path);
 }
 
+static void object_set_child_property(Object *obj, Visitor *v, void *opaque,
+                                      const char *name, Error **errp)
+{
+    gchar *path = NULL;
+    Error *local_err = NULL;
+
+    visit_type_str(v, &path, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (path && *path) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "Cannot set child properties to anything but empty string");
+        return;
+    }
+
+    object_property_del(obj, name, errp);
+}
+
 static void object_finalize_child_property(Object *obj, const char *name,
                                            void *opaque)
 {
@@ -890,15 +920,17 @@ static void object_finalize_child_property(Object *obj, const char *name,
     object_unref(child);
 }
 
-void object_property_add_child(Object *obj, const char *name,
-                               Object *child, Error **errp)
+static void object_property_add_child_full(Object *obj, const char *name,
+                                           Object *child, bool writable,
+                                           Error **errp)
 {
     gchar *type;
 
     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
 
     object_property_add(obj, name, type, object_get_child_property,
-                        NULL, object_finalize_child_property, child, errp);
+                        writable ? object_set_child_property : NULL,
+                        object_finalize_child_property, child, errp);
 
     object_ref(child);
     g_assert(child->parent == NULL);
@@ -907,8 +939,20 @@ void object_property_add_child(Object *obj, const char *name,
     g_free(type);
 }
 
-static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
-                                     const char *name, Error **errp)
+void object_property_add_child(Object *obj, const char *name,
+                               Object *child, Error **errp)
+{
+    object_property_add_child_full(obj, name, child, false, errp);
+}
+
+void object_property_add_nullable_child(Object *obj, const char *name,
+                                        Object *child, Error **errp)
+{
+    object_property_add_child_full(obj, name, child, true, errp);
+}
+
+void object_get_link_property(Object *obj, Visitor *v, void *opaque,
+                              const char *name, Error **errp)
 {
     Object **child = opaque;
     gchar *path;
@@ -923,8 +967,8 @@ static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
-static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
-                                     const char *name, Error **errp)
+void object_set_link_property(Object *obj, Visitor *v, void *opaque,
+                              const char *name, Error **errp)
 {
     Object **child = opaque;
     Object *old_target;
@@ -970,6 +1014,16 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+static void object_finalize_link_property(Object *obj, const char *name,
+                                           void *opaque)
+{
+    Object **child = opaque;
+
+    if (*child != NULL) {
+        object_unref(*child);
+    }
+}
+
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
                               Error **errp)
@@ -981,7 +1035,11 @@ void object_property_add_link(Object *obj, const char *name,
     object_property_add(obj, name, full_type,
                         object_get_link_property,
                         object_set_link_property,
-                        NULL, child, errp);
+                        object_finalize_link_property, child, errp);
+
+    if (*child) {
+        object_ref(*child);
+    }
 
     g_free(full_type);
 }
-- 
1.7.5.4

[Qemu-devel] [PATCH 6/9] qdev: make devices created with device_add nullable so they can be deleted

[Anthony Liguori]

A management tool can destroy these devices by writing an empty string into
the child link property.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev-monitor.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 018b386..3f08575 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -469,15 +469,20 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         return NULL;
     }
     if (qdev->id) {
-        object_property_add_child(qdev_get_peripheral(), qdev->id,
-                                  OBJECT(qdev), NULL);
+        object_property_add_nullable_child(qdev_get_peripheral(), qdev->id,
+                                           OBJECT(qdev), NULL);
     } else {
         static int anon_count;
         gchar *name = g_strdup_printf("device[%d]", anon_count++);
-        object_property_add_child(qdev_get_peripheral_anon(), name,
-                                  OBJECT(qdev), NULL);
+        object_property_add_nullable_child(qdev_get_peripheral_anon(), name,
+                                           OBJECT(qdev), NULL);
         g_free(name);
-    }        
+    }
+
+    /* Drop the allocation reference -- the container link will ensure the
+       object stays alive. */
+    object_unref(OBJECT(qdev));
+
     if (qdev_init(qdev) < 0) {
         qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
-- 
1.7.5.4

[Qemu-devel] [PATCH 7/9] qdev: add notifier for when the device loses its parent bus (eject)

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |    2 ++
 hw/qdev.h |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index fdee91f..86e1337 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -672,6 +672,8 @@ static void device_initfn(Object *obj)
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL);
+
+    notifier_list_init(&dev->eject_notifier);
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/hw/qdev.h b/hw/qdev.h
index 3561e3a..5009072 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -8,6 +8,7 @@
 #include "qapi/qapi-visit-core.h"
 #include "qemu/object.h"
 #include "error.h"
+#include "notify.h"
 
 typedef struct Property Property;
 
@@ -76,6 +77,8 @@ struct DeviceState {
     int num_child_bus;
     int instance_id_alias;
     int alias_required_for_version;
+
+    NotifierList eject_notifier;
 };
 
 #define TYPE_BUS "bus"
-- 
1.7.5.4

[Qemu-devel] [PATCH 8/9] qdev: make qdev_set_parent_bus() just set a link property

[Anthony Liguori]

Also make setting the link to NULL break the bus link

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   48 ++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 86e1337..525a0cb 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -100,8 +100,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
 
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
 {
-    dev->parent_bus = bus;
-    bus_add_child(bus, dev);
+    object_property_set_link(OBJECT(dev), OBJECT(bus), "parent_bus", NULL);
 }
 
 /* Create a new device.  This only initializes the device state structure
@@ -241,8 +240,8 @@ void qbus_reset_all_fn(void *opaque)
 /* can be used as ->unplug() callback for the simple cases */
 int qdev_simple_unplug_cb(DeviceState *dev)
 {
-    /* just zap it */
-    qdev_free(dev);
+    /* Unplug from parent bus via a forced eject */
+    qdev_set_parent_bus(dev, NULL);
     return 0;
 }
 
@@ -646,6 +645,40 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
     assert_no_error(local_err);
 }
 
+static void qdev_set_link_property(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    BusState *parent_bus = dev->parent_bus;
+
+    object_set_link_property(obj, v, opaque, name, errp);
+
+    if (parent_bus) {
+        bus_remove_child(parent_bus, dev);
+    }
+
+    if (dev->parent_bus) {
+        bus_add_child(dev->parent_bus, dev);
+    }
+
+    if (!dev->parent_bus) {
+        notifier_list_notify(&dev->eject_notifier, dev);
+    }
+}
+
+static void qdev_release_link_property(Object *obj, const char *name,
+                                       void *opaque)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    if (dev->parent_bus) {
+        bus_remove_child(dev->parent_bus, dev);
+        object_unref(OBJECT(dev->parent_bus));
+    }
+
+    dev->parent_bus = NULL;
+}
+
 static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -670,8 +703,11 @@ static void device_initfn(Object *obj)
     } while (class != object_class_by_name(TYPE_DEVICE));
     qdev_prop_set_globals(dev);
 
-    object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
-                             (Object **)&dev->parent_bus, NULL);
+    object_property_add(OBJECT(dev), "parent_bus", "link<" TYPE_BUS ">",
+                        object_get_link_property,
+                        qdev_set_link_property,
+                        qdev_release_link_property,
+                        &dev->parent_bus, NULL);
 
     notifier_list_init(&dev->eject_notifier);
 }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 8/9] qdev: make qdev_set_parent_bus() just set a link property

[liu ping fan]

On Sun, Aug 26, 2012 at 11:51 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Also make setting the link to NULL break the bus link
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c |   48 ++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 86e1337..525a0cb 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -100,8 +100,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>
>  void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>  {
> -    dev->parent_bus = bus;
> -    bus_add_child(bus, dev);
> +    object_property_set_link(OBJECT(dev), OBJECT(bus), "parent_bus", NULL);
>  }
>
>  /* Create a new device.  This only initializes the device state structure
> @@ -241,8 +240,8 @@ void qbus_reset_all_fn(void *opaque)
>  /* can be used as ->unplug() callback for the simple cases */
>  int qdev_simple_unplug_cb(DeviceState *dev)
>  {
> -    /* just zap it */
> -    qdev_free(dev);
> +    /* Unplug from parent bus via a forced eject */
> +    qdev_set_parent_bus(dev, NULL);

I think it is more reliable to remove the reference property(child,
link) before object_finialize().  So when uplug-finish, we delete all
the refers:  bus->child, bus<-child by _del_property not using
_set_property.

>      return 0;
>  }
>
> @@ -646,6 +645,40 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
>      assert_no_error(local_err);
>  }
>
> +static void qdev_set_link_property(Object *obj, Visitor *v, void *opaque,
> +                                   const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    BusState *parent_bus = dev->parent_bus;
> +
> +    object_set_link_property(obj, v, opaque, name, errp);
> +
> +    if (parent_bus) {
> +        bus_remove_child(parent_bus, dev);
> +    }
> +
> +    if (dev->parent_bus) {
> +        bus_add_child(dev->parent_bus, dev);
> +    }
> +
> +    if (!dev->parent_bus) {
> +        notifier_list_notify(&dev->eject_notifier, dev);
> +    }
> +}
> +
> +static void qdev_release_link_property(Object *obj, const char *name,
> +                                       void *opaque)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    if (dev->parent_bus) {
> +        bus_remove_child(dev->parent_bus, dev);
> +        object_unref(OBJECT(dev->parent_bus));
> +    }
> +
> +    dev->parent_bus = NULL;
> +}
> +
>  static void device_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -670,8 +703,11 @@ static void device_initfn(Object *obj)
>      } while (class != object_class_by_name(TYPE_DEVICE));
>      qdev_prop_set_globals(dev);
>
> -    object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
> -                             (Object **)&dev->parent_bus, NULL);
> +    object_property_add(OBJECT(dev), "parent_bus", "link<" TYPE_BUS ">",
> +                        object_get_link_property,
> +                        qdev_set_link_property,
> +                        qdev_release_link_property,
> +                        &dev->parent_bus, NULL);
>
>      notifier_list_init(&dev->eject_notifier);
>  }
> --
> 1.7.5.4
>
>

Re: [Qemu-devel] [PATCH 8/9] qdev: make qdev_set_parent_bus() just set a link property

[Anthony Liguori]

liu ping fan <qemulist@gmail.com> writes:

> On Sun, Aug 26, 2012 at 11:51 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Also make setting the link to NULL break the bus link
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/qdev.c |   48 ++++++++++++++++++++++++++++++++++++++++++------
>>  1 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 86e1337..525a0cb 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -100,8 +100,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>
>>  void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>>  {
>> -    dev->parent_bus = bus;
>> -    bus_add_child(bus, dev);
>> +    object_property_set_link(OBJECT(dev), OBJECT(bus), "parent_bus", NULL);
>>  }
>>
>>  /* Create a new device.  This only initializes the device state structure
>> @@ -241,8 +240,8 @@ void qbus_reset_all_fn(void *opaque)
>>  /* can be used as ->unplug() callback for the simple cases */
>>  int qdev_simple_unplug_cb(DeviceState *dev)
>>  {
>> -    /* just zap it */
>> -    qdev_free(dev);
>> +    /* Unplug from parent bus via a forced eject */
>> +    qdev_set_parent_bus(dev, NULL);
>
> I think it is more reliable to remove the reference property(child,
> link) before object_finialize().  So when uplug-finish, we delete all
> the refers:  bus->child, bus<-child by _del_property not using
> _set_property.

object_finalize is called when ref=0.  You cannot remove refs in
finalize because by definition, ref=0.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 9/9] hotplug: refactor hotplug to leverage new QOM functions

[Anthony Liguori]

1) DeviceState::unplug requests for an eject to happen
   - the default implementation is a forced eject

2) A bus can eject a device by setting the parent_bus to NULL
   - this detaches the device from the bus
   - this does *not* cause the device to disappear

3) The current implementation on unplug also registers an eject notifier
   - the eject notifier will detach the device the parent.  This will cause the
     device to disappear

4) A purely guest initiated unplug will not delete a device but will cause the
   device to appear detached from the guests PoV.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/acpi_piix4.c   |    3 ++-
 hw/pci.c          |   10 +++++++++-
 hw/pcie.c         |    2 +-
 hw/qdev.c         |   22 ++++++++++++++++++++++
 hw/qdev.h         |    2 ++
 hw/shpc.c         |    2 +-
 hw/xen_platform.c |    2 +-
 7 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 72d6e5c..eac53b3 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -305,7 +305,8 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
-                qdev_free(qdev);
+                /* Force eject of device */
+                qdev_set_parent_bus(qdev, NULL);
             }
         }
     }
diff --git a/hw/pci.c b/hw/pci.c
index 437af70..cc555c2 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -46,6 +46,14 @@ static char *pcibus_get_dev_path(DeviceState *dev);
 static char *pcibus_get_fw_dev_path(DeviceState *dev);
 static int pcibus_reset(BusState *qbus);
 
+static void pcibus_remove_child(BusState *bus, DeviceState *dev)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    PCIBus *pci_bus = PCI_BUS(bus);
+
+    pci_bus->devices[pci_dev->devfn] = NULL;
+}
+
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
@@ -65,6 +73,7 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
     k->get_dev_path = pcibus_get_dev_path;
     k->get_fw_dev_path = pcibus_get_fw_dev_path;
     k->reset = pcibus_reset;
+    k->remove_child = pcibus_remove_child;
 }
 
 static const TypeInfo pci_bus_info = {
@@ -833,7 +842,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 static void do_pci_unregister_device(PCIDevice *pci_dev)
 {
     qemu_free_irqs(pci_dev->irq);
-    pci_dev->bus->devices[pci_dev->devfn] = NULL;
     pci_config_free(pci_dev);
 }
 
diff --git a/hw/pcie.c b/hw/pcie.c
index 7c92f19..d10ffea 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -235,7 +235,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
                                    PCI_EXP_SLTSTA_PDS);
         pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
     } else {
-        qdev_free(&pci_dev->qdev);
+        qdev_set_parent_bus(DEVICE(pci_dev), NULL);
         pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
                                      PCI_EXP_SLTSTA_PDS);
         pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
diff --git a/hw/qdev.c b/hw/qdev.c
index 525a0cb..be41f00 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -62,6 +62,7 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 
 static void bus_remove_child(BusState *bus, DeviceState *child)
 {
+    BusClass *bc = BUS_GET_CLASS(bus);
     BusChild *kid;
 
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
@@ -71,6 +72,11 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
             snprintf(name, sizeof(name), "child[%d]", kid->index);
             QTAILQ_REMOVE(&bus->children, kid, sibling);
             object_property_del(OBJECT(bus), name, NULL);
+
+            if (bc->remove_child) {
+                bc->remove_child(bus, kid->child);
+            }
+
             g_free(kid);
             return;
         }
@@ -192,9 +198,20 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
+static void qdev_finish_unplug(Notifier *notifier, void *data)
+{
+    DeviceState *dev = DEVICE(data);
+
+    /* unparent the object -- this should release the last reference to the
+       child*/
+    object_unparent(OBJECT(dev));
+    g_free(notifier);
+}
+
 void qdev_unplug(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    Notifier *notifier;
 
     if (!dev->parent_bus->allow_hotplug) {
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
@@ -204,6 +221,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     qdev_hot_removed = true;
 
+    notifier = g_malloc0(sizeof(*notifier));
+    notifier->notify = qdev_finish_unplug;
+
+    notifier_list_add(&dev->eject_notifier, notifier);
+
     if (dc->unplug(dev) < 0) {
         error_set(errp, QERR_UNDEFINED_ERROR);
         return;
diff --git a/hw/qdev.h b/hw/qdev.h
index 5009072..7ae8d5d 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -99,6 +99,8 @@ struct BusClass {
      */
     char *(*get_fw_dev_path)(DeviceState *dev);
     int (*reset)(BusState *bus);
+
+    void (*remove_child)(BusState *bus, DeviceState *dev);
 };
 
 typedef struct BusChild {
diff --git a/hw/shpc.c b/hw/shpc.c
index a5baf24..ce507e7 100644
--- a/hw/shpc.c
+++ b/hw/shpc.c
@@ -253,7 +253,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
          ++devfn) {
         PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
         if (affected_dev) {
-            qdev_free(&affected_dev->qdev);
+            qdev_set_parent_bus(DEVICE(affected_dev), NULL);
         }
     }
 }
diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index 0d6c2ff..5c5ecf8 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -87,7 +87,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
 {
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_NETWORK_ETHERNET) {
-        qdev_free(&d->qdev);
+        qdev_set_parent_bus(dev, NULL);
     }
 }
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 9/9] hotplug: refactor hotplug to leverage new QOM functions

[liu ping fan]

On Sun, Aug 26, 2012 at 11:51 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> 1) DeviceState::unplug requests for an eject to happen
>    - the default implementation is a forced eject
>
> 2) A bus can eject a device by setting the parent_bus to NULL
>    - this detaches the device from the bus
>    - this does *not* cause the device to disappear
>
> 3) The current implementation on unplug also registers an eject notifier
>    - the eject notifier will detach the device the parent.  This will cause the
>      device to disappear
>
> 4) A purely guest initiated unplug will not delete a device but will cause the
>    device to appear detached from the guests PoV.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/acpi_piix4.c   |    3 ++-
>  hw/pci.c          |   10 +++++++++-
>  hw/pcie.c         |    2 +-
>  hw/qdev.c         |   22 ++++++++++++++++++++++
>  hw/qdev.h         |    2 ++
>  hw/shpc.c         |    2 +-
>  hw/xen_platform.c |    2 +-
>  7 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 72d6e5c..eac53b3 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -305,7 +305,8 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
>              if (pc->no_hotplug) {
>                  slot_free = false;
>              } else {
> -                qdev_free(qdev);
> +                /* Force eject of device */
> +                qdev_set_parent_bus(qdev, NULL);

Do we need to wait for guest's ACKs for all of this device's children.
Then we can change this node's topology in the device tree.
I think, we can color the current device as unplug_state=ACK, and then
decide whether to detached it or not.
Each unplug ack from guest, will first check 1st.whether current node
can be release or not.  2nd. if can released, then go bottom-up
through the device tree to check whether the upper device can be
released or not.
If the down device(devB) removal cause the up device(devA) becomes a
leaf, then we can remove devA.
A leaf device is defined as : has no BusState kids OR all of its
BusState kids are empty.

This method can avoid sudden remove.
>              }
>          }
>      }
> diff --git a/hw/pci.c b/hw/pci.c
> index 437af70..cc555c2 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -46,6 +46,14 @@ static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static int pcibus_reset(BusState *qbus);
>
> +static void pcibus_remove_child(BusState *bus, DeviceState *dev)
> +{
> +    PCIDevice *pci_dev = PCI_DEVICE(dev);
> +    PCIBus *pci_bus = PCI_BUS(bus);
> +
> +    pci_bus->devices[pci_dev->devfn] = NULL;
> +}
> +
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> @@ -65,6 +73,7 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
>      k->get_dev_path = pcibus_get_dev_path;
>      k->get_fw_dev_path = pcibus_get_fw_dev_path;
>      k->reset = pcibus_reset;
> +    k->remove_child = pcibus_remove_child;
>  }
>
>  static const TypeInfo pci_bus_info = {
> @@ -833,7 +842,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>  static void do_pci_unregister_device(PCIDevice *pci_dev)
>  {
>      qemu_free_irqs(pci_dev->irq);
> -    pci_dev->bus->devices[pci_dev->devfn] = NULL;
>      pci_config_free(pci_dev);
>  }
>
> diff --git a/hw/pcie.c b/hw/pcie.c
> index 7c92f19..d10ffea 100644
> --- a/hw/pcie.c
> +++ b/hw/pcie.c
> @@ -235,7 +235,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
>                                     PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
>      } else {
> -        qdev_free(&pci_dev->qdev);
> +        qdev_set_parent_bus(DEVICE(pci_dev), NULL);
>          pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
>                                       PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 525a0cb..be41f00 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -62,6 +62,7 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>
>  static void bus_remove_child(BusState *bus, DeviceState *child)
>  {
> +    BusClass *bc = BUS_GET_CLASS(bus);
>      BusChild *kid;
>
>      QTAILQ_FOREACH(kid, &bus->children, sibling) {
> @@ -71,6 +72,11 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>              snprintf(name, sizeof(name), "child[%d]", kid->index);
>              QTAILQ_REMOVE(&bus->children, kid, sibling);
>              object_property_del(OBJECT(bus), name, NULL);
> +
> +            if (bc->remove_child) {
> +                bc->remove_child(bus, kid->child);
> +            }
> +
>              g_free(kid);
>              return;
>          }
> @@ -192,9 +198,20 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>
> +static void qdev_finish_unplug(Notifier *notifier, void *data)
> +{
> +    DeviceState *dev = DEVICE(data);
> +
> +    /* unparent the object -- this should release the last reference to the
> +       child*/
> +    object_unparent(OBJECT(dev));
> +    g_free(notifier);
> +}
> +
>  void qdev_unplug(DeviceState *dev, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    Notifier *notifier;
>
>      if (!dev->parent_bus->allow_hotplug) {
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> @@ -204,6 +221,11 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>
>      qdev_hot_removed = true;
>
> +    notifier = g_malloc0(sizeof(*notifier));
> +    notifier->notify = qdev_finish_unplug;
> +
> +    notifier_list_add(&dev->eject_notifier, notifier);
> +
>      if (dc->unplug(dev) < 0) {
>          error_set(errp, QERR_UNDEFINED_ERROR);
>          return;
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 5009072..7ae8d5d 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -99,6 +99,8 @@ struct BusClass {
>       */
>      char *(*get_fw_dev_path)(DeviceState *dev);
>      int (*reset)(BusState *bus);
> +
> +    void (*remove_child)(BusState *bus, DeviceState *dev);
>  };
>
>  typedef struct BusChild {
> diff --git a/hw/shpc.c b/hw/shpc.c
> index a5baf24..ce507e7 100644
> --- a/hw/shpc.c
> +++ b/hw/shpc.c
> @@ -253,7 +253,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
>           ++devfn) {
>          PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
>          if (affected_dev) {
> -            qdev_free(&affected_dev->qdev);
> +            qdev_set_parent_bus(DEVICE(affected_dev), NULL);
>          }
>      }
>  }
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index 0d6c2ff..5c5ecf8 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -87,7 +87,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
>  {
>      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
>              PCI_CLASS_NETWORK_ETHERNET) {
> -        qdev_free(&d->qdev);
> +        qdev_set_parent_bus(dev, NULL);
>      }
>  }
>
> --
> 1.7.5.4
>
>

Re: [Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[liu ping fan]

On Sun, Aug 26, 2012 at 11:51 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Right now, you need to pair up object_new with object_delete.  This is
> impractical when using reference counting because we would like to ensure that
> object_unref() also frees memory when needed.
>
> The first few patches fix this problem by introducing a release callback so
> that objects that need special release behavior (i.e. g_free) can do that.
>
> Since link and child properties all hold references, in order to actually free
> an object, we need to break those links.  User created devices end up as
> children of a container.  But child properties cannot be removed which means
> there's no obvious way to remove the reference and ultimately free the object.
>
Why? Since we call _add_child() in qdev_device_add(), why can not we
call object_property_del_child() for qmp_device_del(). Could you
explain it more detail?

> We introduce the concept of "nullable child" properties to solve this.  This is
> a child property that can be broken by writing NULL to the child link.  Today
> we set all /peripheral* children to be nullable so that they can be deleted by
> management tools.
>
> In terms of modeling hotplug, we represent unplug by removing the object from
> the parent bus.  We need to register a notifier for when this happens so that
> we can also remove the parent's child property to ultimately release the object.
>
> Putting it all together, we have:
>
> 1) qmp_device_del will issue a callback to a device.  The default callback will
>    do a forced eject (which means writing NULL to the parent_bus link).
>
> 2) PCI hotplug is a bit more sophisticated in that it waits for the guest to
>    do the ejection.
>
> 3) qmp_device_del will register an eject notifier such that the device gets
>    completely removed.
>
> There's a slightly change in behavior here.  A device is not automatically
> destroyed based on a guest initiated eject.  A management tool must explicitly
> break the parent's link to the child in order for the device to disappear
> completely.  device_del behaves exactly as it does today though.
>
> This is an RFC.  I've tested the series quite a lot (it was hard to get the
> reference counting right) but not enough to apply.  I also don't think the
> series is quite split right and may not bisect cleanly.
>
> I also want to write up a document describing object life cycle since admittedly
> the above is probably not that easy to follow.
>
> I wanted to share this now though because it works and I think the concepts are
> right.
>
>

Re: [Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[Andreas Färber]

Am 27.08.2012 09:22, schrieb liu ping fan:
> On Sun, Aug 26, 2012 at 11:51 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Right now, you need to pair up object_new with object_delete.  This is
>> impractical when using reference counting because we would like to ensure that
>> object_unref() also frees memory when needed.
>>
>> The first few patches fix this problem by introducing a release callback so
>> that objects that need special release behavior (i.e. g_free) can do that.
>>
>> Since link and child properties all hold references, in order to actually free
>> an object, we need to break those links.  User created devices end up as
>> children of a container.  But child properties cannot be removed which means
>> there's no obvious way to remove the reference and ultimately free the object.
>>
> Why? Since we call _add_child() in qdev_device_add(), why can not we
> call object_property_del_child() for qmp_device_del(). Could you
> explain it more detail?

Seconded. If we hot-unplug a device, we should surely remove its child<>
property from /machine/unassigned or parent bus or whatever.
Why is it that child<> properties cannot be removed?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[Paolo Bonzini]

Il 27/08/2012 13:46, Andreas Färber ha scritto:
>>> >>
>>> >> Since link and child properties all hold references, in order to actually free
>>> >> an object, we need to break those links.  User created devices end up as
>>> >> children of a container.  But child properties cannot be removed which means
>>> >> there's no obvious way to remove the reference and ultimately free the object.
>>> >>
>> > Why? Since we call _add_child() in qdev_device_add(), why can not we
>> > call object_property_del_child() for qmp_device_del(). Could you
>> > explain it more detail?
> Seconded. If we hot-unplug a device, we should surely remove its child<>
> property from /machine/unassigned or parent bus or whatever.

Sure, as soon as the device is ejected by the guest.  But until that
point we need to keep the device in the QOM tree so that: 1) it has a
canonical path; 2) it can be examined; 3) it keeps children alive.

> Why is it that child<> properties cannot be removed?

Yeah, I didn't quite understand the difference between unparenting and
setting the child property to NULL.

Paolo

Re: [Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 27/08/2012 13:46, Andreas Färber ha scritto:
>>>> >>
>>>> >> Since link and child properties all hold references, in order to actually free
>>>> >> an object, we need to break those links.  User created devices end up as
>>>> >> children of a container.  But child properties cannot be removed which means
>>>> >> there's no obvious way to remove the reference and ultimately free the object.
>>>> >>
>>> > Why? Since we call _add_child() in qdev_device_add(), why can not we
>>> > call object_property_del_child() for qmp_device_del(). Could you
>>> > explain it more detail?
>> Seconded. If we hot-unplug a device, we should surely remove its child<>
>> property from /machine/unassigned or parent bus or whatever.

That's exactly what is happening in this series.  qmp_device_del adds an
ejection notifier that unparents the device to remove the last reference count.

>
> Sure, as soon as the device is ejected by the guest.  But until that
> point we need to keep the device in the QOM tree so that: 1) it has a
> canonical path; 2) it can be examined; 3) it keeps children alive.
>
>> Why is it that child<> properties cannot be removed?
>
> Yeah, I didn't quite understand the difference between unparenting and
> setting the child property to NULL.

They are exactly the same thing.  Setting the child property to NULL is
unparenting.

Unparenting is essentially deleting.  This series makes it such that
there is a white list of devices that are capable of being deleted.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [RFC PATCH 0/9] qom: improve reference counting and hotplug

[Vasilis Liaskovitis]

Hi,

On Sun, Aug 26, 2012 at 10:51:29AM -0500, Anthony Liguori wrote:
> Right now, you need to pair up object_new with object_delete.  This is
> impractical when using reference counting because we would like to ensure that
> object_unref() also frees memory when needed.
> 
> The first few patches fix this problem by introducing a release callback so
> that objects that need special release behavior (i.e. g_free) can do that.
> 
> Since link and child properties all hold references, in order to actually free
> an object, we need to break those links.  User created devices end up as
> children of a container.  But child properties cannot be removed which means
> there's no obvious way to remove the reference and ultimately free the object.
> 
> We introduce the concept of "nullable child" properties to solve this.  This is
> a child property that can be broken by writing NULL to the child link.  Today
> we set all /peripheral* children to be nullable so that they can be deleted by
> management tools.
> 
> In terms of modeling hotplug, we represent unplug by removing the object from
> the parent bus.  We need to register a notifier for when this happens so that
> we can also remove the parent's child property to ultimately release the object.
> 
> Putting it all together, we have:
> 
> 1) qmp_device_del will issue a callback to a device.  The default callback will
>    do a forced eject (which means writing NULL to the parent_bus link).
> 
> 2) PCI hotplug is a bit more sophisticated in that it waits for the guest to
>    do the ejection.
> 
> 3) qmp_device_del will register an eject notifier such that the device gets
>    completely removed.
> 
> There's a slightly change in behavior here.  A device is not automatically
> destroyed based on a guest initiated eject.  A management tool must explicitly
> break the parent's link to the child in order for the device to disappear
> completely.  device_del behaves exactly as it does today though.

is there a plan to respin this series, or has it been dropped? Afaict it hasn't
landed upstream. I am reworking an acpi memory hotplug RFC patchset, and noticed this
patchset changes guest-initiated eject semantics. 

Currently a successfull guest-initiated eject in my mem-hotplug patchset destroys the
qdev device. But the semantics of this patchset would require a new behaviour.

thanks,

- Vasilis