Re: [Qemu-devel] [RFC PATCH 10/13] memory: MemoryRegion: replace owner field with QOM parent

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <address@hidden> wrote:
> The two are now the same.
>
> Signed-off-by: Paolo Bonzini <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  include/exec/memory.h |  1 -
>  memory.c              | 35 +++++++++++++++++++++++++----------
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ead7eaf..bcea4df 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -140,7 +140,6 @@ struct MemoryRegion {
>      const MemoryRegionOps *ops;
>      const MemoryRegionIOMMUOps *iommu_ops;
>      void *opaque;
> -    struct Object *owner;
>      MemoryRegion *container;
>      Int128 size;
>      hwaddr addr;
> diff --git a/memory.c b/memory.c
> index 04d0cd3..acbab4c 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -865,9 +865,11 @@ void memory_region_init(MemoryRegion *mr,
>                          const char *name,
>                          uint64_t size)
>  {
> -    object_initialize(mr, sizeof(*mr), TYPE_MEMORY_REGION);
> +    if (!owner) {
> +        owner = qdev_get_machine();
> +    }
>
> -    mr->owner = owner ? owner : qdev_get_machine();
> +    object_initialize(mr, sizeof(*mr), TYPE_MEMORY_REGION);
>      mr->size = int128_make64(size);
>      if (size == UINT64_MAX) {
>          mr->size = int128_2_64();
> @@ -875,7 +877,7 @@ void memory_region_init(MemoryRegion *mr,
>      mr->name = g_strdup(name);
>
>      if (name) {
> -        object_property_add_child_array(mr->owner, name, OBJECT(mr));
> +        object_property_add_child_array(owner, name, OBJECT(mr));
>          object_unref(OBJECT(mr));
>      }
>  }
> @@ -1130,24 +1132,37 @@ void memory_region_destroy(MemoryRegion *mr)
>
>  Object *memory_region_owner(MemoryRegion *mr)
>  {
> -    return mr->owner;
> +    Object *obj = OBJECT(mr);
> +    return obj->parent;

Ideally this should be functionized. object_get_parent or some such?

>  }
>
>  void memory_region_ref(MemoryRegion *mr)
>  {
> -    if (mr && mr->owner) {
> -        object_ref(mr->owner);
> +    /* MMIO callbacks most likely will access data that belongs
> +     * to the owner, hence the need to ref/unref the owner whenever
> +     * the memory region is in use.
> +     *
> +     * The memory region is a child of its owner.  As long as the
> +     * owner doesn't call unparent itself on the memory region,
> +     * ref-ing the owner will also keep the memory region alive.
> +     * Memory regions without an owner are supposed to never go away,
> +     * but we still ref/unref them for debugging purposes.

This seems like a generic problem of reffing a composite object. I.e.
If anyone one of the components are reffed, ref the composite instead.

Should there perhaps be a boolean flag in struct Object that you can
set that redirects all refs/unrefs to the QOM parent? Then you can
construct all sorts of composite objects without having to add this
particular iffery to reffage code. The stream proxy objects
(rx_data_dev and rx_control_dev) in hw/dma/xilinx_axidma.c seem like
another candidate to me for this kind of feature - considering they
are taken and used as links, but would die quite badly if their parent
was finalised out from underneath them.

I think that's all follow up though - hence the RB.

Regards,
Peter

> +     */
> +    Object *obj = OBJECT(mr);
> +    if (obj && obj->parent) {
> +        object_ref(obj->parent);
>      } else {
> -        object_ref(mr);
> +        object_ref(obj);
>      }
>  }
>
>  void memory_region_unref(MemoryRegion *mr)
>  {
> -    if (mr && mr->owner) {
> -        object_unref(mr->owner);
> +    Object *obj = OBJECT(mr);
> +    if (obj && obj->parent) {
> +        object_unref(obj->parent);
>      } else {
> -        object_unref(mr);
> +        object_unref(obj);
>      }
>  }
>
> --
> 1.8.3.1
>
>
>