[Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Paolo Bonzini]

This series is an approach to MemoryRegion QOMification that is
complementary to Peter's posted patches.  Instead of focusing on
properties, it places attention on modeling the lifetime of memory
regions correctly.  MemoryRegions are added to the QOM tree as
children of their owner device, and analogously to what was done
for devices, memory_region_destroy becomes a simple forwarder for
object_unparent.  Still, I am including Peter's refactorings and
also read-only access to region properties.

In fact, with this change memory_region_destroy becomes mostly
obsolete, because when the owner dies the children memory regions
will be unparented themselves and then (as they lose the last ref)
finalized.

In other words, this effectively achieves the same as
http://lists.gnu.org/archive/html/qemu-devel/2013-09/msg00477.html
except it does this by way of removing code rather than adding it!
Indeed I have a follow-up that drops all but 7 calls to
memory_region_destroy.

I have already applied patches 5 and 6 to the memory branch.


Paolo Bonzini (5):
  qom: move unparenting to the child property's release callback
  qom: delete properties before calling instance_finalize
  memory: MemoryRegion: use /machine as default owner
  memory: MemoryRegion: rename parent to container
  memory: MemoryRegion: replace owner field with QOM parent

Peter Crosthwaite (8):
  qom: object: Ignore refs/unrefs of NULL
  qom: object: remove parent pointer when unparenting
  memory: MemoryRegion: factor out subregion add functionality
  memory: MemoryRegion: factor out memory region re-adder
  memory: MemoryRegion: QOMify
  memory: MemoryRegion: Add container and addr props
  memory: MemoryRegion: Add may-overlap and priority props
  memory: MemoryRegion: Add size property

 exec.c                |   4 +-
 include/exec/memory.h |  29 +++---
 memory.c              | 255 ++++++++++++++++++++++++++++++++++++++++----------
 qom/object.c          |  29 +++---
 4 files changed, 235 insertions(+), 82 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 01/13] qom: object: Ignore refs/unrefs of NULL

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Just do nothing if passed NULL for a ref or unref. This avoids
call sites that manage a combination of NULL or non-NULL pointers
having to add iffery around every ref and unref.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index e146ae5..3650a5b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -709,11 +709,17 @@ GSList *object_class_get_list(const char *implements_type,
 
 void object_ref(Object *obj)
 {
+     if (!obj) {
+         return;
+     }
      atomic_inc(&obj->ref);
 }
 
 void object_unref(Object *obj)
 {
+    if (!obj) {
+        return;
+    }
     g_assert(obj->ref > 0);
 
     /* parent always holds a reference to its children */
@@ -1132,13 +1138,9 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    if (new_target) {
-        object_ref(new_target);
-    }
+    object_ref(new_target);
     *child = new_target;
-    if (old_target != NULL) {
-        object_unref(old_target);
-    }
+    object_unref(old_target);
 }
 
 static Object *object_resolve_link_property(Object *parent, void *opaque, const gchar *part)
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 02/13] qom: object: remove parent pointer when unparenting

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Certain parts of the QOM framework test this pointer to determine if
an object is parented. Nuke it when the object is unparented to allow
for reuse of an object after unparenting.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qom/object.c b/qom/object.c
index 3650a5b..01087e5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -396,6 +396,7 @@ void object_unparent(Object *obj)
     }
     if (obj->parent) {
         object_property_del_child(obj->parent, obj, NULL);
+        obj->parent = NULL;
     }
     object_unref(obj);
 }
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 03/13] qom: move unparenting to the child property's release callback

[Paolo Bonzini]

This ensures that the unparent callback is called automatically
when the parent object is finalized.

Note that there's no need to keep a reference neither in
object_unparent nor in object_finalize_child_property.  The
reference held by the child property itself will do.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 01087e5..69a95d6 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -386,19 +386,9 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
 
 void object_unparent(Object *obj)
 {
-    if (!obj->parent) {
-        return;
-    }
-
-    object_ref(obj);
-    if (obj->class->unparent) {
-        (obj->class->unparent)(obj);
-    }
     if (obj->parent) {
         object_property_del_child(obj->parent, obj, NULL);
-        obj->parent = NULL;
     }
-    object_unref(obj);
 }
 
 static void object_deinit(Object *obj, TypeImpl *type)
@@ -1017,6 +1007,10 @@ static void object_finalize_child_property(Object *obj, const char *name,
 {
     Object *child = opaque;
 
+    if (child->class->unparent) {
+        (child->class->unparent)(child);
+    }
+    child->parent = NULL;
     object_unref(child);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 03/13] qom: move unparenting to the child property's release callback

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This ensures that the unparent callback is called automatically
> when the parent object is finalized.
>
> Note that there's no need to keep a reference neither in
> object_unparent nor in object_finalize_child_property.  The
> reference held by the child property itself will do.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 01087e5..69a95d6 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -386,19 +386,9 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>
>  void object_unparent(Object *obj)
>  {
> -    if (!obj->parent) {
> -        return;
> -    }
> -
> -    object_ref(obj);
> -    if (obj->class->unparent) {
> -        (obj->class->unparent)(obj);
> -    }

This changes this publicly visible API significantly, in that unparent
from the childs point-of-view is now invisible in cases where you are
doing a standalone object_unparent() call.

I'm not sure why parent finalisation is the only occasion on which a
genuine and complete unparenting is allowed? I think this API should
remain complete should a QOM user wish to re-arrange child
relationships seperate from any finalisations.

>      if (obj->parent) {
>          object_property_del_child(obj->parent, obj, NULL);
> -        obj->parent = NULL;
>      }
> -    object_unref(obj);
>  }
>
>  static void object_deinit(Object *obj, TypeImpl *type)
> @@ -1017,6 +1007,10 @@ static void object_finalize_child_property(Object *obj, const char *name,
>  {
>      Object *child = opaque;
>
> +    if (child->class->unparent) {
> +        (child->class->unparent)(child);
> +    }
> +    child->parent = NULL;

Can you just leave object_unparent() mostly as-is and call it from
here? It's already guarded against repeated calls should there be
users calling it manually before parent finalisations.

Regards,
Peter

>      object_unref(child);
>  }
>
> --
> 1.8.3.1
>
>
>

Re: [Qemu-devel] [RFC PATCH 03/13] qom: move unparenting to the child property's release callback

[Paolo Bonzini]

Il 11/06/2014 15:35, Peter Crosthwaite ha scritto:
> On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> This ensures that the unparent callback is called automatically
>> when the parent object is finalized.
>>
>> Note that there's no need to keep a reference neither in
>> object_unparent nor in object_finalize_child_property.  The
>> reference held by the child property itself will do.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  qom/object.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 01087e5..69a95d6 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -386,19 +386,9 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>>
>>  void object_unparent(Object *obj)
>>  {
>> -    if (!obj->parent) {
>> -        return;
>> -    }
>> -
>> -    object_ref(obj);
>> -    if (obj->class->unparent) {
>> -        (obj->class->unparent)(obj);
>> -    }
>
> This changes this publicly visible API significantly, in that unparent
> from the childs point-of-view is now invisible in cases where you are
> doing a standalone object_unparent() call.
>
> I'm not sure why parent finalisation is the only occasion on which a
> genuine and complete unparenting is allowed? I think this API should
> remain complete should a QOM user wish to re-arrange child
> relationships seperate from any finalisations.

No, this is moved to object_finalize_child_property, i.e. the release 
callback of child<> properties.  object_property_del_child calls it.

The change is that, before, the unparent callback was only invoked if 
you explicitly called object_unparent.  Now it's always called as part 
of finalize, even if you unparent the outer object but not the inner 
ones.  I find it more intuitive.

Note that this is not yet needed in these 13 patches.

Paolo

>>      if (obj->parent) {
>>          object_property_del_child(obj->parent, obj, NULL);
>> -        obj->parent = NULL;
>>      }
>> -    object_unref(obj);
>>  }
>>
>>  static void object_deinit(Object *obj, TypeImpl *type)
>> @@ -1017,6 +1007,10 @@ static void object_finalize_child_property(Object *obj, const char *name,
>>  {
>>      Object *child = opaque;
>>
>> +    if (child->class->unparent) {
>> +        (child->class->unparent)(child);
>> +    }
>> +    child->parent = NULL;
>
> Can you just leave object_unparent() mostly as-is and call it from
> here? It's already guarded against repeated calls should there be
> users calling it manually before parent finalisations.
>
> Regards,
> Peter
>
>>      object_unref(child);
>>  }
>>
>> --
>> 1.8.3.1
>>
>>
>>

Re: [Qemu-devel] [RFC PATCH 03/13] qom: move unparenting to the child property's release callback

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 12:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/06/2014 15:35, Peter Crosthwaite ha scritto:
>
>> On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com>
>> wrote:
>>>
>>> This ensures that the unparent callback is called automatically
>>> when the parent object is finalized.
>>>
>>> Note that there's no need to keep a reference neither in
>>> object_unparent nor in object_finalize_child_property.  The
>>> reference held by the child property itself will do.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  qom/object.c | 14 ++++----------
>>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 01087e5..69a95d6 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -386,19 +386,9 @@ static void object_property_del_child(Object *obj,
>>> Object *child, Error **errp)
>>>
>>>  void object_unparent(Object *obj)
>>>  {
>>> -    if (!obj->parent) {
>>> -        return;
>>> -    }
>>> -
>>> -    object_ref(obj);
>>> -    if (obj->class->unparent) {
>>> -        (obj->class->unparent)(obj);
>>> -    }
>>
>>
>> This changes this publicly visible API significantly, in that unparent
>> from the childs point-of-view is now invisible in cases where you are
>> doing a standalone object_unparent() call.
>>
>> I'm not sure why parent finalisation is the only occasion on which a
>> genuine and complete unparenting is allowed? I think this API should
>> remain complete should a QOM user wish to re-arrange child
>> relationships seperate from any finalisations.
>
>
> No, this is moved to object_finalize_child_property, i.e. the release
> callback of child<> properties.  object_property_del_child calls it.
>

Ok it's adding up for me now.

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

> The change is that, before, the unparent callback was only invoked if you
> explicitly called object_unparent.  Now it's always called as part of
> finalize, even if you unparent the outer object but not the inner ones.  I
> find it more intuitive.
>
> Note that this is not yet needed in these 13 patches.
>
> Paolo
>
>
>>>      if (obj->parent) {
>>>          object_property_del_child(obj->parent, obj, NULL);
>>> -        obj->parent = NULL;
>>>      }
>>> -    object_unref(obj);
>>>  }
>>>
>>>  static void object_deinit(Object *obj, TypeImpl *type)
>>> @@ -1017,6 +1007,10 @@ static void object_finalize_child_property(Object
>>> *obj, const char *name,
>>>  {
>>>      Object *child = opaque;
>>>
>>> +    if (child->class->unparent) {
>>> +        (child->class->unparent)(child);
>>> +    }
>>> +    child->parent = NULL;
>>
>>
>> Can you just leave object_unparent() mostly as-is and call it from
>> here? It's already guarded against repeated calls should there be
>> users calling it manually before parent finalisations.
>>
>> Regards,
>> Peter
>>
>>>      object_unref(child);
>>>  }
>>>
>>> --
>>> 1.8.3.1
>>>
>>>
>>>
>
>

[Qemu-devel] [RFC PATCH 04/13] qom: delete properties before calling instance_finalize

[Paolo Bonzini]

This ensures that the children's unparent callback will still
have a usable parent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 69a95d6..780aaa8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -407,8 +407,8 @@ static void object_finalize(void *data)
     Object *obj = data;
     TypeImpl *ti = obj->class->type;
 
-    object_deinit(obj, ti);
     object_property_del_all(obj);
+    object_deinit(obj, ti);
 
     g_assert(obj->ref == 0);
     if (obj->free) {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 04/13] qom: delete properties before calling instance_finalize

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This ensures that the children's unparent callback will still
> have a usable parent.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 69a95d6..780aaa8 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -407,8 +407,8 @@ static void object_finalize(void *data)
>      Object *obj = data;
>      TypeImpl *ti = obj->class->type;
>
> -    object_deinit(obj, ti);
>      object_property_del_all(obj);
> +    object_deinit(obj, ti);

Does this means you lose access to your own properties before calling
your own finalisation? I guess its a non-issue if all clients just use
private fields directly, but it seems weird you cant use your own
getter at the end because it's dissappeared on you already.

Regards,
Peter

>
>      g_assert(obj->ref == 0);
>      if (obj->free) {
> --
> 1.8.3.1
>
>
>

Re: [Qemu-devel] [RFC PATCH 04/13] qom: delete properties before calling instance_finalize

[Paolo Bonzini]

Il 11/06/2014 15:42, Peter Crosthwaite ha scritto:
>> >
>> > -    object_deinit(obj, ti);
>> >      object_property_del_all(obj);
>> > +    object_deinit(obj, ti);
> Does this means you lose access to your own properties before calling
> your own finalisation? I guess its a non-issue if all clients just use
> private fields directly, but it seems weird you cant use your own
> getter at the end because it's dissappeared on you already.

It is just as weird that a property's finalizer cannot do its work 
because it needs something that instance_finalize has already freed. :)

This is important with the previous patch, that calls arbitrary code of 
children objects at finalization time.

Paolo

Re: [Qemu-devel] [RFC PATCH 04/13] qom: delete properties before calling instance_finalize

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 12:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/06/2014 15:42, Peter Crosthwaite ha scritto:
>
>>> >
>>> > -    object_deinit(obj, ti);
>>> >      object_property_del_all(obj);
>>> > +    object_deinit(obj, ti);
>>
>> Does this means you lose access to your own properties before calling
>> your own finalisation? I guess its a non-issue if all clients just use
>> private fields directly, but it seems weird you cant use your own
>> getter at the end because it's dissappeared on you already.
>
>
> It is just as weird that a property's finalizer cannot do its work because
> it needs something that instance_finalize has already freed. :)
>
> This is important with the previous patch, that calls arbitrary code of
> children objects at finalization time.
>

Ok fair enough.

Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> Paolo
>

[Qemu-devel] [RFC PATCH 05/13] memory: MemoryRegion: factor out subregion add functionality

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Split off the core looping code that actually adds subregions into
it's own fn. This prepares support for Memory Region qomification
where setting the MR address or parent via QOM will back onto this more
minimal function.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[Rename new function. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/memory.c b/memory.c
index 3811bd1..15f63e8 100644
--- a/memory.c
+++ b/memory.c
@@ -1423,18 +1423,15 @@ void memory_region_del_eventfd(MemoryRegion *mr,
     memory_region_transaction_commit();
 }
 
-static void memory_region_add_subregion_common(MemoryRegion *mr,
-                                               hwaddr offset,
-                                               MemoryRegion *subregion)
+static void memory_region_update_parent_subregions(MemoryRegion *subregion)
 {
+    hwaddr offset = subregion->addr;
+    MemoryRegion *mr = subregion->parent;
     MemoryRegion *other;
 
     memory_region_transaction_begin();
 
-    assert(!subregion->parent);
     memory_region_ref(subregion);
-    subregion->parent = mr;
-    subregion->addr = offset;
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->may_overlap || other->may_overlap) {
             continue;
@@ -1468,6 +1465,15 @@ done:
     memory_region_transaction_commit();
 }
 
+static void memory_region_add_subregion_common(MemoryRegion *mr,
+                                               hwaddr offset,
+                                               MemoryRegion *subregion)
+{
+    assert(!subregion->parent);
+    subregion->parent = mr;
+    subregion->addr = offset;
+    memory_region_update_parent_subregions(subregion);
+}
 
 void memory_region_add_subregion(MemoryRegion *mr,
                                  hwaddr offset,
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 06/13] memory: MemoryRegion: factor out memory region re-adder

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

memory_region_set_address is mostly just a function that deletes and
re-adds a memory region. Factor this generic functionality out into a
re-usable function. This prepares support for further QOMification
of MemoryRegion.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/memory.c b/memory.c
index 15f63e8..06a4af7 100644
--- a/memory.c
+++ b/memory.c
@@ -1517,21 +1517,27 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
     memory_region_transaction_commit();
 }
 
-void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
+static void memory_region_readd_subregion(MemoryRegion *mr)
 {
     MemoryRegion *parent = mr->parent;
 
-    if (addr == mr->addr || !parent) {
-        mr->addr = addr;
-        return;
+    if (parent) {
+        memory_region_transaction_begin();
+        memory_region_ref(mr);
+        memory_region_del_subregion(parent, mr);
+        mr->parent = parent;
+        memory_region_update_parent_subregions(mr);
+        memory_region_unref(mr);
+        memory_region_transaction_commit();
     }
+}
 
-    memory_region_transaction_begin();
-    memory_region_ref(mr);
-    memory_region_del_subregion(parent, mr);
-    memory_region_add_subregion_common(parent, addr, mr);
-    memory_region_unref(mr);
-    memory_region_transaction_commit();
+void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
+{
+    if (addr != mr->addr) {
+        mr->addr = addr;
+        memory_region_readd_subregion(mr);
+    }
 }
 
 void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset)
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 07/13] memory: MemoryRegion: use /machine as default owner

[Paolo Bonzini]

This will be used (after QOMification) as the QOM parent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index 06a4af7..b40055c 100644
--- a/memory.c
+++ b/memory.c
@@ -848,7 +848,7 @@ void memory_region_init(MemoryRegion *mr,
 {
     mr->ops = &unassigned_mem_ops;
     mr->opaque = NULL;
-    mr->owner = owner;
+    mr->owner = owner ? owner : qdev_get_machine();
     mr->iommu_ops = NULL;
     mr->parent = NULL;
     mr->size = int128_make64(size);
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 07/13] memory: MemoryRegion: use /machine as default owner

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This will be used (after QOMification) as the QOM parent.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/memory.c b/memory.c
> index 06a4af7..b40055c 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -848,7 +848,7 @@ void memory_region_init(MemoryRegion *mr,
>  {
>      mr->ops = &unassigned_mem_ops;
>      mr->opaque = NULL;
> -    mr->owner = owner;
> +    mr->owner = owner ? owner : qdev_get_machine();
>      mr->iommu_ops = NULL;
>      mr->parent = NULL;
>      mr->size = int128_make64(size);
> --
> 1.8.3.1
>
>
>

[Qemu-devel] [RFC PATCH 08/13] memory: MemoryRegion: rename parent to container

[Paolo Bonzini]

Avoid confusion with the QOM parent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 20 ++++++++++----------
 memory.c              | 38 +++++++++++++++++++-------------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 1d55ad9..549ae73 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -135,7 +135,7 @@ struct MemoryRegion {
     const MemoryRegionIOMMUOps *iommu_ops;
     void *opaque;
     struct Object *owner;
-    MemoryRegion *parent;
+    MemoryRegion *container;
     Int128 size;
     hwaddr addr;
     void (*destructor)(MemoryRegion *mr);
@@ -815,11 +815,11 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled);
 /*
  * memory_region_set_address: dynamically update the address of a region
  *
- * Dynamically updates the address of a region, relative to its parent.
+ * Dynamically updates the address of a region, relative to its container.
  * May be used on regions are currently part of a memory hierarchy.
  *
  * @mr: the region to be updated
- * @addr: new address, relative to parent region
+ * @addr: new address, relative to container region
  */
 void memory_region_set_address(MemoryRegion *mr, hwaddr addr);
 
@@ -836,16 +836,16 @@ void memory_region_set_alias_offset(MemoryRegion *mr,
                                     hwaddr offset);
 
 /**
- * memory_region_present: checks if an address relative to a @parent
- * translates into #MemoryRegion within @parent
+ * memory_region_present: checks if an address relative to a @container
+ * translates into #MemoryRegion within @container
  *
- * Answer whether a #MemoryRegion within @parent covers the address
+ * Answer whether a #MemoryRegion within @container covers the address
  * @addr.
  *
- * @parent: a #MemoryRegion within which @addr is a relative address
- * @addr: the area within @parent to be searched
+ * @container: a #MemoryRegion within which @addr is a relative address
+ * @addr: the area within @container to be searched
  */
-bool memory_region_present(MemoryRegion *parent, hwaddr addr);
+bool memory_region_present(MemoryRegion *container, hwaddr addr);
 
 /**
  * memory_region_find: translate an address/size relative to a
@@ -866,7 +866,7 @@ bool memory_region_present(MemoryRegion *parent, hwaddr addr);
  * Similarly, the .@offset_within_address_space is relative to the
  * address space that contains both regions, the passed and the
  * returned one.  However, in the special case where the @mr argument
- * has no parent (and thus is the root of the address space), the
+ * has no container (and thus is the root of the address space), the
  * following will hold:
  *    .@offset_within_address_space >= @addr
  *    .@offset_within_address_space + .@size <= @addr + @size
diff --git a/memory.c b/memory.c
index b40055c..5a60622 100644
--- a/memory.c
+++ b/memory.c
@@ -485,8 +485,8 @@ static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
 {
     AddressSpace *as;
 
-    while (mr->parent) {
-        mr = mr->parent;
+    while (mr->container) {
+        mr = mr->container;
     }
     QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
         if (mr == as->root) {
@@ -1423,10 +1423,10 @@ void memory_region_del_eventfd(MemoryRegion *mr,
     memory_region_transaction_commit();
 }
 
-static void memory_region_update_parent_subregions(MemoryRegion *subregion)
+static void memory_region_update_container_subregions(MemoryRegion *subregion)
 {
     hwaddr offset = subregion->addr;
-    MemoryRegion *mr = subregion->parent;
+    MemoryRegion *mr = subregion->container;
     MemoryRegion *other;
 
     memory_region_transaction_begin();
@@ -1469,10 +1469,10 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
                                                hwaddr offset,
                                                MemoryRegion *subregion)
 {
-    assert(!subregion->parent);
-    subregion->parent = mr;
+    assert(!subregion->container);
+    subregion->container = mr;
     subregion->addr = offset;
-    memory_region_update_parent_subregions(subregion);
+    memory_region_update_container_subregions(subregion);
 }
 
 void memory_region_add_subregion(MemoryRegion *mr,
@@ -1498,8 +1498,8 @@ void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion)
 {
     memory_region_transaction_begin();
-    assert(subregion->parent == mr);
-    subregion->parent = NULL;
+    assert(subregion->container == mr);
+    subregion->container = NULL;
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
     memory_region_unref(subregion);
     memory_region_update_pending |= mr->enabled && subregion->enabled;
@@ -1519,14 +1519,14 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
 
 static void memory_region_readd_subregion(MemoryRegion *mr)
 {
-    MemoryRegion *parent = mr->parent;
+    MemoryRegion *container = mr->container;
 
-    if (parent) {
+    if (container) {
         memory_region_transaction_begin();
         memory_region_ref(mr);
-        memory_region_del_subregion(parent, mr);
-        mr->parent = parent;
-        memory_region_update_parent_subregions(mr);
+        memory_region_del_subregion(container, mr);
+        mr->container = container;
+        memory_region_update_container_subregions(mr);
         memory_region_unref(mr);
         memory_region_transaction_commit();
     }
@@ -1578,10 +1578,10 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr)
                    sizeof(FlatRange), cmp_flatrange_addr);
 }
 
-bool memory_region_present(MemoryRegion *parent, hwaddr addr)
+bool memory_region_present(MemoryRegion *container, hwaddr addr)
 {
-    MemoryRegion *mr = memory_region_find(parent, addr, 1).mr;
-    if (!mr || (mr == parent)) {
+    MemoryRegion *mr = memory_region_find(container, addr, 1).mr;
+    if (!mr || (mr == container)) {
         return false;
     }
     memory_region_unref(mr);
@@ -1599,8 +1599,8 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
     FlatRange *fr;
 
     addr += mr->addr;
-    for (root = mr; root->parent; ) {
-        root = root->parent;
+    for (root = mr; root->container; ) {
+        root = root->container;
         addr += root->addr;
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 08/13] memory: MemoryRegion: rename parent to container

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Avoid confusion with the QOM parent.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  include/exec/memory.h | 20 ++++++++++----------
>  memory.c              | 38 +++++++++++++++++++-------------------
>  2 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 1d55ad9..549ae73 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -135,7 +135,7 @@ struct MemoryRegion {
>      const MemoryRegionIOMMUOps *iommu_ops;
>      void *opaque;
>      struct Object *owner;
> -    MemoryRegion *parent;
> +    MemoryRegion *container;
>      Int128 size;
>      hwaddr addr;
>      void (*destructor)(MemoryRegion *mr);
> @@ -815,11 +815,11 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled);
>  /*
>   * memory_region_set_address: dynamically update the address of a region
>   *
> - * Dynamically updates the address of a region, relative to its parent.
> + * Dynamically updates the address of a region, relative to its container.
>   * May be used on regions are currently part of a memory hierarchy.
>   *
>   * @mr: the region to be updated
> - * @addr: new address, relative to parent region
> + * @addr: new address, relative to container region
>   */
>  void memory_region_set_address(MemoryRegion *mr, hwaddr addr);
>
> @@ -836,16 +836,16 @@ void memory_region_set_alias_offset(MemoryRegion *mr,
>                                      hwaddr offset);
>
>  /**
> - * memory_region_present: checks if an address relative to a @parent
> - * translates into #MemoryRegion within @parent
> + * memory_region_present: checks if an address relative to a @container
> + * translates into #MemoryRegion within @container
>   *
> - * Answer whether a #MemoryRegion within @parent covers the address
> + * Answer whether a #MemoryRegion within @container covers the address
>   * @addr.
>   *
> - * @parent: a #MemoryRegion within which @addr is a relative address
> - * @addr: the area within @parent to be searched
> + * @container: a #MemoryRegion within which @addr is a relative address
> + * @addr: the area within @container to be searched
>   */
> -bool memory_region_present(MemoryRegion *parent, hwaddr addr);
> +bool memory_region_present(MemoryRegion *container, hwaddr addr);
>
>  /**
>   * memory_region_find: translate an address/size relative to a
> @@ -866,7 +866,7 @@ bool memory_region_present(MemoryRegion *parent, hwaddr addr);
>   * Similarly, the .@offset_within_address_space is relative to the
>   * address space that contains both regions, the passed and the
>   * returned one.  However, in the special case where the @mr argument
> - * has no parent (and thus is the root of the address space), the
> + * has no container (and thus is the root of the address space), the
>   * following will hold:
>   *    .@offset_within_address_space >= @addr
>   *    .@offset_within_address_space + .@size <= @addr + @size
> diff --git a/memory.c b/memory.c
> index b40055c..5a60622 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -485,8 +485,8 @@ static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
>  {
>      AddressSpace *as;
>
> -    while (mr->parent) {
> -        mr = mr->parent;
> +    while (mr->container) {
> +        mr = mr->container;
>      }
>      QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>          if (mr == as->root) {
> @@ -1423,10 +1423,10 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>      memory_region_transaction_commit();
>  }
>
> -static void memory_region_update_parent_subregions(MemoryRegion *subregion)
> +static void memory_region_update_container_subregions(MemoryRegion *subregion)
>  {
>      hwaddr offset = subregion->addr;
> -    MemoryRegion *mr = subregion->parent;
> +    MemoryRegion *mr = subregion->container;
>      MemoryRegion *other;
>
>      memory_region_transaction_begin();
> @@ -1469,10 +1469,10 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
>                                                 hwaddr offset,
>                                                 MemoryRegion *subregion)
>  {
> -    assert(!subregion->parent);
> -    subregion->parent = mr;
> +    assert(!subregion->container);
> +    subregion->container = mr;
>      subregion->addr = offset;
> -    memory_region_update_parent_subregions(subregion);
> +    memory_region_update_container_subregions(subregion);
>  }
>
>  void memory_region_add_subregion(MemoryRegion *mr,
> @@ -1498,8 +1498,8 @@ void memory_region_del_subregion(MemoryRegion *mr,
>                                   MemoryRegion *subregion)
>  {
>      memory_region_transaction_begin();
> -    assert(subregion->parent == mr);
> -    subregion->parent = NULL;
> +    assert(subregion->container == mr);
> +    subregion->container = NULL;
>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>      memory_region_unref(subregion);
>      memory_region_update_pending |= mr->enabled && subregion->enabled;
> @@ -1519,14 +1519,14 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
>
>  static void memory_region_readd_subregion(MemoryRegion *mr)
>  {
> -    MemoryRegion *parent = mr->parent;
> +    MemoryRegion *container = mr->container;
>
> -    if (parent) {
> +    if (container) {
>          memory_region_transaction_begin();
>          memory_region_ref(mr);
> -        memory_region_del_subregion(parent, mr);
> -        mr->parent = parent;
> -        memory_region_update_parent_subregions(mr);
> +        memory_region_del_subregion(container, mr);
> +        mr->container = container;
> +        memory_region_update_container_subregions(mr);
>          memory_region_unref(mr);
>          memory_region_transaction_commit();
>      }
> @@ -1578,10 +1578,10 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr)
>                     sizeof(FlatRange), cmp_flatrange_addr);
>  }
>
> -bool memory_region_present(MemoryRegion *parent, hwaddr addr)
> +bool memory_region_present(MemoryRegion *container, hwaddr addr)
>  {
> -    MemoryRegion *mr = memory_region_find(parent, addr, 1).mr;
> -    if (!mr || (mr == parent)) {
> +    MemoryRegion *mr = memory_region_find(container, addr, 1).mr;
> +    if (!mr || (mr == container)) {
>          return false;
>      }
>      memory_region_unref(mr);
> @@ -1599,8 +1599,8 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
>      FlatRange *fr;
>
>      addr += mr->addr;
> -    for (root = mr; root->parent; ) {
> -        root = root->parent;
> +    for (root = mr; root->container; ) {
> +        root = root->container;
>          addr += root->addr;
>      }
>
> --
> 1.8.3.1
>
>
>

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

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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 <peter.crosthwaite@xilinx.com>
[Add newly-created MR as child, unparent on destruction. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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);
+    }
+}
+        
+
 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) {
+        object_property_add_child_array(mr->owner, name, OBJECT(mr));
+        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

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

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> 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 <peter.crosthwaite@xilinx.com>
> [Add newly-created MR as child, unparent on destruction. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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
>
>
>

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

[Paolo Bonzini]

Il 11/06/2014 15:12, Peter Crosthwaite ha scritto:
>> > +    if (name) {
> Should a stock "anonymous" or such name be used if !name?
>
>> > +        object_property_add_child_array(mr->owner, name, OBJECT(mr));

The !name case is used by "transient" memory regions for which we don't 
want the overhead of QOM (right now, this is the subpage memory regions 
used by exec.c).  These are removed with object_unref, not object_unparent.

Because of the default owner now being /machine, the problem becomes 
much more pervasive.

I tried a few machines, and even perfectly qdevified ones fail.  For 
example ARM has multiple gic_cpu memory regions.  And everything that 
has two instances of something that is not qdev-ified will fail horribly 
(this includes PC, whose 8257 DMA controllers are not qdev-ified).

If you do not create all memory region as children you cannot know how 
to destroy the region: will an unref be enough (memory_region_destroy in 
your series) or is unparent also necessary?

In the end, I believe the conceptual cleanliness of destroy==unparent, 
and the removal of 200-odd memory_region_destroy calls, are worth the 
extra ugly [0]'s.

Paolo

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

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 12:15 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/06/2014 15:12, Peter Crosthwaite ha scritto:
>
>>> > +    if (name) {
>>
>> Should a stock "anonymous" or such name be used if !name?
>>
>>> > +        object_property_add_child_array(mr->owner, name, OBJECT(mr));
>
>
> The !name case is used by "transient" memory regions for which we don't want
> the overhead of QOM (right now, this is the subpage memory regions used by
> exec.c).  These are removed with object_unref, not object_unparent.
>
> Because of the default owner now being /machine, the problem becomes much
> more pervasive.
>
> I tried a few machines, and even perfectly qdevified ones fail.  For example
> ARM has multiple gic_cpu memory regions.  And everything that has two
> instances of something that is not qdev-ified will fail horribly (this
> includes PC, whose 8257 DMA controllers are not qdev-ified).
>
> If you do not create all memory region as children you cannot know how to
> destroy the region: will an unref be enough (memory_region_destroy in your
> series) or is unparent also necessary?
>
> In the end, I believe the conceptual cleanliness of destroy==unparent, and
> the removal of 200-odd memory_region_destroy calls, are worth the extra ugly
> [0]'s.
>

I think you get to keep the uniqueness though under my latest
proposal? No tree-wide collision resolution needed. When
object_property_add_child_array sucessfully sets something to [0] (i
== 0 iteration), just create an alias without [] at the same time. The
naming system will behave identically to this patch as-is in all the
collision cases (i >= 1).

Regards,
Peter

> Paolo
>

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

[Paolo Bonzini]

Il 11/06/2014 16:32, Peter Crosthwaite ha scritto:
>> > In the end, I believe the conceptual cleanliness of destroy==unparent, and
>> > the removal of 200-odd memory_region_destroy calls, are worth the extra ugly
>> > [0]'s.
>> >
> I think you get to keep the uniqueness though under my latest
> proposal? No tree-wide collision resolution needed. When
> object_property_add_child_array sucessfully sets something to [0] (i
> == 0 iteration), just create an alias without [] at the same time. The
> naming system will behave identically to this patch as-is in all the
> collision cases (i >= 1).

Yes, but then you get duplicates, and some of them are wrong (the ones 
without [0] if there is also a [1]).

I think the solution could be to special case names ending with "[*]" 
(perhaps straight in object_property_add_child!), and then audit 
manually at all the 400+ init calls.

For now I'd rather keep it simple.

Paolo

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

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 12:48 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/06/2014 16:32, Peter Crosthwaite ha scritto:
>
>>> > In the end, I believe the conceptual cleanliness of destroy==unparent,
>>> > and
>>> > the removal of 200-odd memory_region_destroy calls, are worth the extra
>>> > ugly
>>> > [0]'s.
>>> >
>>
>> I think you get to keep the uniqueness though under my latest
>> proposal? No tree-wide collision resolution needed. When
>> object_property_add_child_array sucessfully sets something to [0] (i
>> == 0 iteration), just create an alias without [] at the same time. The
>> naming system will behave identically to this patch as-is in all the
>> collision cases (i >= 1).
>
>
> Yes, but then you get duplicates, and some of them are wrong (the ones
> without [0] if there is also a [1]).
>
> I think the solution could be to special case names ending with "[*]"
> (perhaps straight in object_property_add_child!), and then audit manually at
> all the 400+ init calls.
>
> For now I'd rather keep it simple.
>

Fair enough. I guess we can add that alias later (and when aliases
themselves are a little more baked).

Regards,
Peter

> Paolo
>

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

[Paolo Bonzini]

The two are now the same.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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;
 }
 
 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.
+     */
+    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

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 <pbonzini@redhat.com> wrote:
> The two are now the same.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

[Qemu-devel] [RFC PATCH 11/13] memory: MemoryRegion: Add container and addr props

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Expose the already existing .parent and .addr fields as QOM properties.
.parent (i.e. the field describing the memory region that contains this
one in Memory hierachy) is renamed "container". This is to avoid
confusion with the QOM parent.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[Remove setters.  Do not unref parent on releasing the property. Clean
 up error propagation. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/memory.c b/memory.c
index acbab4c..13a9fe8 100644
--- a/memory.c
+++ b/memory.c
@@ -16,6 +16,7 @@
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 #include "exec/ioport.h"
+#include "qapi/visitor.h"
 #include "qemu/bitops.h"
 #include "qom/object.h"
 #include "trace.h"
@@ -882,6 +883,38 @@ void memory_region_init(MemoryRegion *mr,
     }
 }
 
+static void memory_region_get_addr(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    uint64_t value = mr->addr;
+
+    visit_type_uint64(v, &value, name, errp);
+}
+
+static void memory_region_get_container(Object *obj, Visitor *v, void *opaque,
+                                        const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    gchar *path = (gchar *)"";
+
+    if (mr->container) {
+        path = object_get_canonical_path(OBJECT(mr->container));
+    }
+    visit_type_str(v, &path, name, errp);
+    if (mr->container) {
+        g_free(path);
+    }
+}
+
+static Object *memory_region_resolve_container(Object *obj, void *opaque,
+                                               const char *part)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+
+    return OBJECT(mr->container);
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -892,6 +925,18 @@ static void memory_region_initfn(Object *obj)
     mr->destructor = memory_region_destructor_none;
     QTAILQ_INIT(&mr->subregions);
     QTAILQ_INIT(&mr->coalesced);
+
+    object_property_add_full(OBJECT(mr), "container",
+                             "link<" TYPE_MEMORY_REGION ">",
+                             memory_region_get_container,
+                             NULL, /* memory_region_set_container */
+                             memory_region_resolve_container,
+                             NULL, NULL, &error_abort);
+
+    object_property_add(OBJECT(mr), "addr", "uint64",
+                        memory_region_get_addr,
+                        NULL, /* memory_region_set_addr */
+                        NULL, NULL, &error_abort);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 12/13] memory: MemoryRegion: Add may-overlap and priority props

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

QOM propertyify the .may-overlap and .priority fields.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[Remove setters. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h |  2 +-
 memory.c              | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index bcea4df..c526e13 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -156,7 +156,7 @@ struct MemoryRegion {
     bool flush_coalesced_mmio;
     MemoryRegion *alias;
     hwaddr alias_offset;
-    int priority;
+    int32_t priority;
     bool may_overlap;
     QTAILQ_HEAD(subregions, MemoryRegion) subregions;
     QTAILQ_ENTRY(MemoryRegion) subregions_link;
diff --git a/memory.c b/memory.c
index 13a9fe8..7d3266a 100644
--- a/memory.c
+++ b/memory.c
@@ -915,6 +915,22 @@ static Object *memory_region_resolve_container(Object *obj, void *opaque,
     return OBJECT(mr->container);
 }
 
+static void memory_region_get_priority(Object *obj, Visitor *v, void *opaque,
+                                       const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    int32_t value = mr->priority;
+
+    visit_type_int32(v, &value, name, errp);
+}
+
+static bool memory_region_get_may_overlap(Object *obj, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+
+    return mr->may_overlap;
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -937,6 +953,14 @@ static void memory_region_initfn(Object *obj)
                         memory_region_get_addr,
                         NULL, /* memory_region_set_addr */
                         NULL, NULL, &error_abort);
+    object_property_add(OBJECT(mr), "priority", "uint32",
+                        memory_region_get_priority,
+                        NULL, /* memory_region_set_priority */
+                        NULL, NULL, &error_abort);
+    object_property_add_bool(OBJECT(mr), "may-overlap",
+                             memory_region_get_may_overlap,
+                             NULL, /* memory_region_set_may_overlap */
+                             &error_abort);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 13/13] memory: MemoryRegion: Add size property

[Paolo Bonzini]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[Remove setter. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/memory.c b/memory.c
index 7d3266a..3d6188b 100644
--- a/memory.c
+++ b/memory.c
@@ -931,6 +931,15 @@ static bool memory_region_get_may_overlap(Object *obj, Error **errp)
     return mr->may_overlap;
 }
 
+static void memory_region_get_size(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    uint64_t value = int128_get64(mr->size);
+
+    visit_type_uint64(v, &value, name, errp);
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -961,6 +970,10 @@ static void memory_region_initfn(Object *obj)
                              memory_region_get_may_overlap,
                              NULL, /* memory_region_set_may_overlap */
                              &error_abort);
+    object_property_add(OBJECT(mr), "size", "uint64",
+                        memory_region_get_size,
+                        NULL, /* memory_region_set_size, */
+                        NULL, NULL, &error_abort);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Peter Crosthwaite]

On Wed, Jun 11, 2014 at 10:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This series is an approach to MemoryRegion QOMification that is
> complementary to Peter's posted patches.  Instead of focusing on
> properties, it places attention on modeling the lifetime of memory
> regions correctly.  MemoryRegions are added to the QOM tree as
> children of their owner device, and analogously to what was done
> for devices, memory_region_destroy becomes a simple forwarder for
> object_unparent.  Still, I am including Peter's refactorings and
> also read-only access to region properties.
>
> In fact, with this change memory_region_destroy becomes mostly
> obsolete, because when the owner dies the children memory regions
> will be unparented themselves and then (as they lose the last ref)
> finalized.
>
> In other words, this effectively achieves the same as
> http://lists.gnu.org/archive/html/qemu-devel/2013-09/msg00477.html
> except it does this by way of removing code rather than adding it!
> Indeed I have a follow-up that drops all but 7 calls to
> memory_region_destroy.
>
> I have already applied patches 5 and 6 to the memory branch.
>
>

This looks good to me, the only change I see worth making pre-merge is
moving object_property_add_child_array to the QOM core. All other
review discussions have been shelved or closed (as they are additive
suggestions mostly).

Regards,
Peter

> Paolo Bonzini (5):
>   qom: move unparenting to the child property's release callback
>   qom: delete properties before calling instance_finalize
>   memory: MemoryRegion: use /machine as default owner
>   memory: MemoryRegion: rename parent to container
>   memory: MemoryRegion: replace owner field with QOM parent
>
> Peter Crosthwaite (8):
>   qom: object: Ignore refs/unrefs of NULL
>   qom: object: remove parent pointer when unparenting
>   memory: MemoryRegion: factor out subregion add functionality
>   memory: MemoryRegion: factor out memory region re-adder
>   memory: MemoryRegion: QOMify
>   memory: MemoryRegion: Add container and addr props
>   memory: MemoryRegion: Add may-overlap and priority props
>   memory: MemoryRegion: Add size property
>
>  exec.c                |   4 +-
>  include/exec/memory.h |  29 +++---
>  memory.c              | 255 ++++++++++++++++++++++++++++++++++++++++----------
>  qom/object.c          |  29 +++---
>  4 files changed, 235 insertions(+), 82 deletions(-)
>
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Paolo Bonzini]

Il 17/06/2014 14:27, Peter Crosthwaite ha scritto:
> This looks good to me, the only change I see worth making pre-merge is
> moving object_property_add_child_array to the QOM core. All other
> review discussions have been shelved or closed (as they are additive
> suggestions mostly).

What do you think of the idea of special-casing properties that end with 
"[*]" directly in object_property_add{,_full}?  This would have to wait 
for 2.2, but these patches can go in 2.1 as well.

I'll submit the series as soon as I get acks for the preliminary patches:

libqtest;      http://article.gmane.org/gmane.comp.emulators.qemu/279836
prop->resolve: http://article.gmane.org/gmane.comp.emulators.qemu/279594

Paolo

Re: [Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Peter Crosthwaite]

On Tue, Jun 17, 2014 at 10:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/06/2014 14:27, Peter Crosthwaite ha scritto:
>
>> This looks good to me, the only change I see worth making pre-merge is
>> moving object_property_add_child_array to the QOM core. All other
>> review discussions have been shelved or closed (as they are additive
>> suggestions mostly).
>
>
> What do you think of the idea of special-casing properties that end with
> "[*]" directly in object_property_add{,_full}?  This would have to wait for
> 2.2, but these patches can go in 2.1 as well.
>

Yes, that will be nice, as I assume it solves this arrayification
problem for other types of props such as ints and links etc?

Regards,
Peter

> I'll submit the series as soon as I get acks for the preliminary patches:
>
> libqtest;      http://article.gmane.org/gmane.comp.emulators.qemu/279836
> prop->resolve: http://article.gmane.org/gmane.comp.emulators.qemu/279594
>
> Paolo
>

Re: [Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Paolo Bonzini]

Il 17/06/2014 14:51, Peter Crosthwaite ha scritto:
> > What do you think of the idea of special-casing properties that end with
> > "[*]" directly in object_property_add{,_full}?  This would have to wait for
> > 2.2, but these patches can go in 2.1 as well.
>
> Yes, that will be nice, as I assume it solves this arrayification
> problem for other types of props such as ints and links etc?

Yes, exactly.

Paolo

Re: [Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Peter Crosthwaite]

On Tue, Jun 17, 2014 at 10:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/06/2014 14:27, Peter Crosthwaite ha scritto:
>
>> This looks good to me, the only change I see worth making pre-merge is
>> moving object_property_add_child_array to the QOM core. All other
>> review discussions have been shelved or closed (as they are additive
>> suggestions mostly).
>
>
> What do you think of the idea of special-casing properties that end with
> "[*]" directly in object_property_add{,_full}?  This would have to wait for
> 2.2, but these patches can go in 2.1 as well.
>
> I'll submit the series as soon as I get acks for the preliminary patches:
>
> libqtest;      http://article.gmane.org/gmane.comp.emulators.qemu/279836
> prop->resolve: http://article.gmane.org/gmane.comp.emulators.qemu/279594
>

Hi Paolo,

Where we at with this? Is it possible to get this through in some form
inside 2.1 window and should I respin it? I think we are largely in
agreement on this series.

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [RFC PATCH 00/13] "Light" memory region QOMification

[Paolo Bonzini]

Il 25/06/2014 11:45, Peter Crosthwaite ha scritto:
>
> Where we at with this? Is it possible to get this through in some form
> inside 2.1 window and should I respin it? I think we are largely in
> agreement on this series.

I'm waiting for Andreas to review Stefan's object_property_add_alias 
series, because we agreed that it should go in first.

Paolo