Re: [Qemu-devel] [RFC PATCH 09/13] memory: MemoryRegion: QOMify

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <address@hidden> wrote:
> From: Peter Crosthwaite <address@hidden>
>
> QOMify memory regions as an Object. The former init() and destroy()
> routines become instance_init() and instance_finalize() resp.
>
> memory_region_init() is re-implemented to be:
> object_initialize() + set fields
>
> memory_region_destroy() is re-implemented to call unparent().
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> [Add newly-created MR as child, unparent on destruction. - Paolo]
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  exec.c                |  4 +--
>  include/exec/memory.h |  6 ++++
>  memory.c              | 82 
> ++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index c3fbbb3..687f627 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -878,7 +878,7 @@ static void phys_section_destroy(MemoryRegion *mr)
>
>      if (mr->subpage) {
>          subpage_t *subpage = container_of(mr, subpage_t, iomem);
> -        memory_region_destroy(&subpage->iomem);
> +        object_unref(OBJECT(&subpage->iomem));
>          g_free(subpage);
>      }
>  }
> @@ -1765,7 +1765,7 @@ static subpage_t *subpage_init(AddressSpace *as, hwaddr 
> base)
>      mmio->as = as;
>      mmio->base = base;
>      memory_region_init_io(&mmio->iomem, NULL, &subpage_ops, mmio,
> -                          "subpage", TARGET_PAGE_SIZE);
> +                          NULL, TARGET_PAGE_SIZE);
>      mmio->iomem.subpage = true;
>  #if defined(DEBUG_SUBPAGE)
>      printf("%s: %p base " TARGET_FMT_plx " len %08x\n", __func__,
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 549ae73..ead7eaf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -31,10 +31,15 @@
>  #include "qemu/queue.h"
>  #include "qemu/int128.h"
>  #include "qemu/notify.h"
> +#include "qom/object.h"
>
>  #define MAX_PHYS_ADDR_SPACE_BITS 62
>  #define MAX_PHYS_ADDR            (((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - 
> 1)
>
> +#define TYPE_MEMORY_REGION "qemu:memory-region"
> +#define MEMORY_REGION(obj) \
> +        OBJECT_CHECK(MemoryRegion, (obj), TYPE_MEMORY_REGION)
> +
>  typedef struct MemoryRegionOps MemoryRegionOps;
>  typedef struct MemoryRegionMmio MemoryRegionMmio;
>
> @@ -130,6 +135,7 @@ typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>
>  struct MemoryRegion {
> +    Object parent_obj;
>      /* All fields are private - violators will be prosecuted */
>      const MemoryRegionOps *ops;
>      const MemoryRegionIOMMUOps *iommu_ops;
> diff --git a/memory.c b/memory.c
> index 5a60622..04d0cd3 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -841,40 +841,55 @@ static void 
> memory_region_destructor_rom_device(MemoryRegion *mr)
>      qemu_ram_free(mr->ram_addr & TARGET_PAGE_MASK);
>  }
>
> +static void object_property_add_child_array(Object *owner,
> +                                            const char *name,
> +                                            Object *child)
> +{
> +    int i;
> +    for (i = 0; ; i++) {
> +        char *full_name = g_strdup_printf("%s[%d]", name, i);
> +        Error *local_err = NULL;
> +
> +        object_property_add_child(owner, full_name, child, &local_err);
> +        if (!local_err) {
> +            break;
> +        }
> +
> +        error_free(local_err);
> +    }
> +}

I think this concept is generally useful and belongs in QOM core.

> +
> +
>  void memory_region_init(MemoryRegion *mr,
>                          Object *owner,
>                          const char *name,
>                          uint64_t size)
>  {
> -    mr->ops = &unassigned_mem_ops;
> -    mr->opaque = NULL;
> +    object_initialize(mr, sizeof(*mr), TYPE_MEMORY_REGION);
> +
>      mr->owner = owner ? owner : qdev_get_machine();
> -    mr->iommu_ops = NULL;
> -    mr->parent = NULL;
>      mr->size = int128_make64(size);
>      if (size == UINT64_MAX) {
>          mr->size = int128_2_64();
>      }
> -    mr->addr = 0;
> -    mr->subpage = false;
> +    mr->name = g_strdup(name);
> +
> +    if (name) {

Should a stock "anonymous" or such name be used if !name?

> +        object_property_add_child_array(mr->owner, name, OBJECT(mr));

So this seems to be a solution to the problem I'm describing here
taking the name munging approach:

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00364.html

In the bulk of instances however, the name will be unique and this
will just add a lot of "[0]" to visible QOM names etc. I think this
can be solved however by patching object_property_add_child_array to
alias foo[0] to foo.

Regards,
Peter

> +        object_unref(OBJECT(mr));
> +    }
> +}
> +
> +static void memory_region_initfn(Object *obj)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +
> +    mr->ops = &unassigned_mem_ops;
>      mr->enabled = true;
> -    mr->terminates = false;
> -    mr->ram = false;
>      mr->romd_mode = true;
> -    mr->readonly = false;
> -    mr->rom_device = false;
>      mr->destructor = memory_region_destructor_none;
> -    mr->priority = 0;
> -    mr->may_overlap = false;
> -    mr->alias = NULL;
>      QTAILQ_INIT(&mr->subregions);
> -    memset(&mr->subregions_link, 0, sizeof mr->subregions_link);
>      QTAILQ_INIT(&mr->coalesced);
> -    mr->name = g_strdup(name);
> -    mr->dirty_log_mask = 0;
> -    mr->ioeventfd_nb = 0;
> -    mr->ioeventfds = NULL;
> -    mr->flush_coalesced_mmio = false;
>  }
>
>  static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
> @@ -1095,8 +1110,10 @@ void memory_region_init_reservation(MemoryRegion *mr,
>      memory_region_init_io(mr, owner, &unassigned_mem_ops, mr, name, size);
>  }
>
> -void memory_region_destroy(MemoryRegion *mr)
> +static void memory_region_finalize(Object *obj)
>  {
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +
>      assert(QTAILQ_EMPTY(&mr->subregions));
>      assert(memory_region_transaction_depth == 0);
>      mr->destructor(mr);
> @@ -1105,6 +1122,12 @@ void memory_region_destroy(MemoryRegion *mr)
>      g_free(mr->ioeventfds);
>  }
>
> +void memory_region_destroy(MemoryRegion *mr)
> +{
> +    object_unparent(OBJECT(mr));
> +}
> +
> +
>  Object *memory_region_owner(MemoryRegion *mr)
>  {
>      return mr->owner;
> @@ -1114,6 +1137,8 @@ void memory_region_ref(MemoryRegion *mr)
>  {
>      if (mr && mr->owner) {
>          object_ref(mr->owner);
> +    } else {
> +        object_ref(mr);
>      }
>  }
>
> @@ -1121,6 +1146,8 @@ void memory_region_unref(MemoryRegion *mr)
>  {
>      if (mr && mr->owner) {
>          object_unref(mr->owner);
> +    } else {
> +        object_unref(mr);
>      }
>  }
>
> @@ -1904,3 +1931,18 @@ void mtree_info(fprintf_function mon_printf, void *f)
>          g_free(ml);
>      }
>  }
> +
> +static const TypeInfo memory_region_info = {
> +    .parent             = TYPE_OBJECT,
> +    .name               = TYPE_MEMORY_REGION,
> +    .instance_size      = sizeof(MemoryRegion),
> +    .instance_init      = memory_region_initfn,
> +    .instance_finalize  = memory_region_finalize,
> +};
> +
> +static void memory_register_types(void)
> +{
> +    type_register_static(&memory_region_info);
> +}
> +
> +type_init(memory_register_types)
> --
> 1.8.3.1
>
>
>