Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem

[Anthony Liguori <anthony@codemonkey.ws>]

Liu Ping Fan <qemulist@gmail.com> writes:

> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Scene:
>   obja lies in objA, when objA's ref->0, it will be freed,
> but at that time obja can still be in use.
>
> The real example is:
> typedef struct PCIIDEState {
>     PCIDevice dev;
>     IDEBus bus[2]; --> create in place
>     .....
> }
>
> When without big lock protection for mmio-dispatch, we will hold
> obj's refcnt. So memory_region_init_io() will replace the third para
> "void *opaque" with "Object *obj".
> With this patch, we can protect PCIIDEState from disappearing during
> mmio-dispatch hold the IDEBus->ref.
>
> And the ref circle has been broken when calling qdev_delete_subtree().
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

I think this is solving the wrong problem.  There are many, many
dependencies a device may have on other devices.  Memory allocation
isn't the only one.

The problem is that we want to make sure that a device doesn't "go away"
while an MMIO dispatch is happening.  This is easy to solve without
touching referencing counting.

The device will hold a lock while the MMIO is being dispatched.  The
delete path simply needs to acquire that same lock.  This will ensure
that a delete operation cannot finish while MMIO is still in flight.

Regarding deleting a device, not all devices are capable of being
deleted and specifically, devices that are composed within the memory of
another device cannot be directly deleted (they can only be deleted
as part of their parent's destruction).

Regards,

Anthony Liguori

> ---
>  hw/qdev.c |    2 ++
>  hw/qdev.h |    1 +
>  2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e2339a1..b09ebbf 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -510,6 +510,8 @@ void qbus_create_inplace(BusState *bus, const char *typename,
>  {
>      object_initialize(bus, typename);
>  
> +    bus->overlap = parent;
> +    object_ref(OBJECT(bus->overlap));
>      bus->parent = parent;
>      bus->name = name ? g_strdup(name) : NULL;
>      qbus_realize(bus);
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 182cfa5..9bc5783 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -117,6 +117,7 @@ struct BusState {
>      int allow_hotplug;
>      bool qom_allocated;
>      bool glib_allocated;
> +    DeviceState *overlap;
>      int max_index;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> -- 
> 1.7.4.4