[Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Peter Crosthwaite]

Hi Paolo, Andreas,

This patch series QOMifies Memory regions. This is the Memory API
specific subset of patches forming part of the Memory/GPIO/Sysbus
QOMification.

I think Paolo already has P1 enqeued. Including for ease of review.
some QOM patches in P2-4 that cut down on later boilerplate. TBH I can
live without them, if they not liked but they make life better IMO.

For fuller context please see:

http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03265.html

and

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

Changed since v3:
Add Paolo's QOM resolver patch
Add resolver for MemoryRegion container property.
Changed since v2:
Use object unref to finalize MR (Paolo review)
Fixed priority prop getter
Changed prioirty to signed type in QOM getters/setterts
Changed since v1:
Split into subset series.
Converted container link into low level link.
Misc finer tweaks and patch re-orderings.


Paolo Bonzini (1):
  qom: add a generic mechanism to resolve paths

Peter Crosthwaite (9):
  memory: Simplify mr_add_subregion() if-else
  qom: object: Ignore refs/unrefs of NULL
  qom: Publish object_resolve_link
  memory: Coreify 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

 include/exec/memory.h |   8 +-
 include/qom/object.h  |  65 +++++++++++
 memory.c              | 318 +++++++++++++++++++++++++++++++++++++++++++-------
 qom/object.c          |  84 +++++++------
 4 files changed, 395 insertions(+), 80 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH memory v4 01/10] memory: Simplify mr_add_subregion() if-else

[Peter Crosthwaite]

This if else is not needed. The previous call to memory_region_add
(whether _overlap or not) will always set priority and may_overlap
to desired values. And its not possible to get here without having
called memory_region_add_subregion due to the null guard on parent.
So we can just directly call memory_region_add_subregion_common.

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

 memory.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/memory.c b/memory.c
index 678661e..a427b75 100644
--- a/memory.c
+++ b/memory.c
@@ -1501,8 +1501,6 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
 void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
 {
     MemoryRegion *parent = mr->parent;
-    int priority = mr->priority;
-    bool may_overlap = mr->may_overlap;
 
     if (addr == mr->addr || !parent) {
         mr->addr = addr;
@@ -1512,11 +1510,7 @@ void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
     memory_region_transaction_begin();
     memory_region_ref(mr);
     memory_region_del_subregion(parent, mr);
-    if (may_overlap) {
-        memory_region_add_subregion_overlap(parent, addr, mr, priority);
-    } else {
-        memory_region_add_subregion(parent, addr, mr);
-    }
+    memory_region_add_subregion_common(parent, addr, mr);
     memory_region_unref(mr);
     memory_region_transaction_commit();
 }
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 02/10] qom: add a generic mechanism to resolve paths

[Peter Crosthwaite]

From: Paolo Bonzini <pbonzini@redhat.com>

It may be desirable to have custom link<> properties that do more
than just store an object.  Even the addition of a "check"
function is not enough if setting the link has side effects
or if a non-standard reference counting is preferrable.

Avoid the assumption that the opaque field of a link<> is a
LinkProperty struct, by adding a generic "resolve" callback
to ObjectProperty.

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Tested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[ PC Changes:
  * Fixed 80 char lines
]
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 include/qom/object.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c         | 57 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 87 insertions(+), 19 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index a641dcd..f8ab845 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -304,6 +304,23 @@ typedef void (ObjectPropertyAccessor)(Object *obj,
                                       Error **errp);
 
 /**
+ * ObjectPropertyResolve:
+ * @obj: the object that owns the property
+ * @opaque: the opaque registered with the property
+ * @part: the name of the property
+ *
+ * If @path is the path that led to @obj, the function should
+ * return the Object corresponding to "@path/@part".  If #NULL
+ * is returned, "@path/@part" is not a valid object path.
+ *
+ * The returned object can also be used as a starting point
+ * to resolve a relative path starting with "@part".
+ */
+typedef Object *(ObjectPropertyResolve)(Object *obj,
+                                        void *opaque,
+                                        const char *part);
+
+/**
  * ObjectPropertyRelease:
  * @obj: the object that owns the property
  * @name: the name of the property
@@ -321,6 +338,7 @@ typedef struct ObjectProperty
     gchar *type;
     ObjectPropertyAccessor *get;
     ObjectPropertyAccessor *set;
+    ObjectPropertyResolve *resolve;
     ObjectPropertyRelease *release;
     void *opaque;
 
@@ -769,6 +787,37 @@ void object_ref(Object *obj);
 void object_unref(Object *obj);
 
 /**
+ * object_property_add_full:
+ * @obj: the object to add a property to
+ * @name: the name of the property.  This can contain any character except for
+ *  a forward slash.  In general, you should use hyphens '-' instead of
+ *  underscores '_' when naming properties.
+ * @type: the type name of the property.  This namespace is pretty loosely
+ *   defined.  Sub namespaces are constructed by using a prefix and then
+ *   to angle brackets.  For instance, the type 'virtio-net-pci' in the
+ *   'link' namespace would be 'link<virtio-net-pci>'.
+ * @get: The getter to be called to read a property.  If this is NULL, then
+ *   the property cannot be read.
+ * @set: the setter to be called to write a property.  If this is NULL,
+ *   then the property cannot be written.
+ * @resolve: called when the property name is used as part of an object
+ *   path.  This is meant for cases when you want to have custom link
+ *   properties.  If it is NULL, the property name cannot be used as part
+ *   of a valid object path.
+ * @release: called when the property is removed from the object.  This is
+ *   meant to allow a property to free its opaque upon object
+ *   destruction.  This may be NULL.
+ * @opaque: an opaque pointer to pass to the callbacks for the property
+ * @errp: returns an error if this function fails
+ */
+void object_property_add_full(Object *obj, const char *name, const char *type,
+                         ObjectPropertyAccessor *get,
+                         ObjectPropertyAccessor *set,
+                         ObjectPropertyResolve *resolve,
+                         ObjectPropertyRelease *release,
+                         void *opaque, Error **errp);
+
+/**
  * object_property_add:
  * @obj: the object to add a property to
  * @name: the name of the property.  This can contain any character except for
diff --git a/qom/object.c b/qom/object.c
index e42b254..956d1ab 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -355,11 +355,6 @@ static inline bool object_property_is_child(ObjectProperty *prop)
     return strstart(prop->type, "child<", NULL);
 }
 
-static inline bool object_property_is_link(ObjectProperty *prop)
-{
-    return strstart(prop->type, "link<", NULL);
-}
-
 static void object_property_del_all(Object *obj)
 {
     while (!QTAILQ_EMPTY(&obj->properties)) {
@@ -727,9 +722,10 @@ void object_unref(Object *obj)
     }
 }
 
-void object_property_add(Object *obj, const char *name, const char *type,
+void object_property_add_full(Object *obj, const char *name, const char *type,
                          ObjectPropertyAccessor *get,
                          ObjectPropertyAccessor *set,
+                         ObjectPropertyResolve *resolve,
                          ObjectPropertyRelease *release,
                          void *opaque, Error **errp)
 {
@@ -751,12 +747,23 @@ void object_property_add(Object *obj, const char *name, const char *type,
 
     prop->get = get;
     prop->set = set;
+    prop->resolve = resolve;
     prop->release = release;
     prop->opaque = opaque;
 
     QTAILQ_INSERT_TAIL(&obj->properties, prop, node);
 }
 
+void object_property_add(Object *obj, const char *name, const char *type,
+                         ObjectPropertyAccessor *get,
+                         ObjectPropertyAccessor *set,
+                         ObjectPropertyRelease *release,
+                         void *opaque, Error **errp)
+{
+    object_property_add_full(obj, name, type, get, set, NULL, release,
+                             opaque, errp);
+}
+
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp)
 {
@@ -993,6 +1000,12 @@ static void object_get_child_property(Object *obj, Visitor *v, void *opaque,
     g_free(path);
 }
 
+static Object *object_resolve_child_property(Object *parent, void *opaque,
+                                             const gchar *part)
+{
+    return opaque;
+}
+
 static void object_finalize_child_property(Object *obj, const char *name,
                                            void *opaque)
 {
@@ -1009,8 +1022,9 @@ void object_property_add_child(Object *obj, const char *name,
 
     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, &local_err);
+    object_property_add_full(obj, name, type, object_get_child_property, NULL,
+                             object_resolve_child_property,
+                             object_finalize_child_property, child, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
@@ -1128,6 +1142,13 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+static Object *object_resolve_link_property(Object *parent, void *opaque,
+                                            const gchar *part)
+{
+    LinkProperty *lprop = opaque;
+    return *lprop->child;
+}
+
 static void object_release_link_property(Object *obj, const char *name,
                                          void *opaque)
 {
@@ -1156,12 +1177,13 @@ void object_property_add_link(Object *obj, const char *name,
 
     full_type = g_strdup_printf("link<%s>", type);
 
-    object_property_add(obj, name, full_type,
-                        object_get_link_property,
-                        check ? object_set_link_property : NULL,
-                        object_release_link_property,
-                        prop,
-                        &local_err);
+    object_property_add_full(obj, name, full_type,
+                             object_get_link_property,
+                             check ? object_set_link_property : NULL,
+                             object_resolve_link_property,
+                             object_release_link_property,
+                             prop,
+                             &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         g_free(prop);
@@ -1225,11 +1247,8 @@ Object *object_resolve_path_component(Object *parent, const gchar *part)
         return NULL;
     }
 
-    if (object_property_is_link(prop)) {
-        LinkProperty *lprop = prop->opaque;
-        return *lprop->child;
-    } else if (object_property_is_child(prop)) {
-        return prop->opaque;
+    if (prop->resolve) {
+        return prop->resolve(parent, prop->opaque, part);
     } else {
         return NULL;
     }
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 03/10] qom: object: Ignore refs/unrefs of NULL

[Peter Crosthwaite]

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

 qom/object.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 956d1ab..6813aba 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 */
@@ -1133,13 +1139,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,
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 04/10] qom: Publish object_resolve_link

[Peter Crosthwaite]

The lower level API object_resolve_path is already published to the
world as part of the QOM API. Add object_resolve link as well. This
allows QOM clients to roll their own link property setters without
having to fallback to the less safe object_resolve_path.

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

 include/qom/object.h | 16 ++++++++++++++++
 qom/object.c         | 13 ++-----------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index f8ab845..029acd8 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1097,6 +1097,22 @@ Object *object_resolve_path_type(const char *path, const char *typename,
 Object *object_resolve_path_component(Object *parent, const gchar *part);
 
 /**
+ * object_resolve_link:
+ * @obj: The object containing the link property
+ * @name: Name of the link property
+ * @path: the path to resolve
+ * @errp: Error object to populate in case of error
+ *
+ * Lookup an object and ensure its type matches a link property type.  This
+ * is similar to object_resolve_path() except type verification against the
+ * link property is performed.
+ *
+ * Returns: The matched object or NULL on path lookup failures.
+ */
+Object *object_resolve_link(Object *obj, const char *name,
+                            const char *path, Error **errp);
+
+/**
  * object_property_add_child:
  * @obj: the object to add a property to
  * @name: the name of the property
diff --git a/qom/object.c b/qom/object.c
index 6813aba..185d54b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1072,17 +1072,8 @@ static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
-/*
- * object_resolve_link:
- *
- * Lookup an object and ensure its type matches the link property type.  This
- * is similar to object_resolve_path() except type verification against the
- * link property is performed.
- *
- * Returns: The matched object or NULL on path lookup failures.
- */
-static Object *object_resolve_link(Object *obj, const char *name,
-                                   const char *path, Error **errp)
+Object *object_resolve_link(Object *obj, const char *name,
+                            const char *path, Error **errp)
 {
     const char *type;
     gchar *target_type;
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 05/10] memory: Coreify subregion add functionality

[Peter Crosthwaite]

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

 memory.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/memory.c b/memory.c
index a427b75..f9b9745 100644
--- a/memory.c
+++ b/memory.c
@@ -1410,18 +1410,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 do_memory_region_add_subregion_common(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;
@@ -1455,6 +1452,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;
+    do_memory_region_add_subregion_common(subregion);
+}
 
 void memory_region_add_subregion(MemoryRegion *mr,
                                  hwaddr offset,
-- 
2.0.0

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

[Peter Crosthwaite]

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

 memory.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/memory.c b/memory.c
index f9b9745..7135299 100644
--- a/memory.c
+++ b/memory.c
@@ -828,6 +828,23 @@ static void memory_region_destructor_rom_device(MemoryRegion *mr)
     qemu_ram_free(mr->ram_addr & TARGET_PAGE_MASK);
 }
 
+static void do_memory_region_add_subregion_common(MemoryRegion *subregion);
+
+static void memory_region_readd_subregion(MemoryRegion *mr)
+{
+    MemoryRegion *parent = mr->parent;
+
+    if (parent) {
+        memory_region_transaction_begin();
+        memory_region_ref(mr);
+        memory_region_del_subregion(parent, mr);
+        mr->parent = parent;
+        do_memory_region_add_subregion_common(mr);
+        memory_region_unref(mr);
+        memory_region_transaction_commit();
+    }
+}
+
 void memory_region_init(MemoryRegion *mr,
                         Object *owner,
                         const char *name,
@@ -1506,19 +1523,10 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
 
 void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
 {
-    MemoryRegion *parent = mr->parent;
-
-    if (addr == mr->addr || !parent) {
+    if (addr != mr->addr) {
         mr->addr = addr;
-        return;
+        memory_region_readd_subregion(mr);
     }
-
-    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_alias_offset(MemoryRegion *mr, hwaddr offset)
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 07/10] memory: MemoryRegion: QOMify

[Peter Crosthwaite]

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 finalize().

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v3: Removed un-needed memset(0) (PMM review)
changed since v2:
use object_unref() to Destroy MRs properly (Paolo Review)
Drop stale FIXME

 include/exec/memory.h |  6 ++++++
 memory.c              | 54 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 1d55ad9..371c066 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 7135299..aa193df 100644
--- a/memory.c
+++ b/memory.c
@@ -850,35 +850,26 @@ void memory_region_init(MemoryRegion *mr,
                         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;
-    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);
+}
+
+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,
@@ -1099,8 +1090,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);
@@ -1109,6 +1102,12 @@ void memory_region_destroy(MemoryRegion *mr)
     g_free(mr->ioeventfds);
 }
 
+void memory_region_destroy(MemoryRegion *mr)
+{
+    object_unref(OBJECT(mr));
+}
+
+
 Object *memory_region_owner(MemoryRegion *mr)
 {
     return mr->owner;
@@ -1893,3 +1892,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)
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 08/10] memory: MemoryRegion: Add container and addr props

[Peter Crosthwaite]

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 owner field, which is much more akin to an actual QOM
parent.

Setting the .parent ("container") will cause the memory subregion adding
to happen.  Nullifying or changing the .parent will delete or relocate
(to a different container) the subregion resp.

Setting or changing the address will relocate the memory region.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v3:
Added resolver for container property (Paolo)
changed since v2:
Removed un-needed strdup
changed since v1:
Converted container to low level link property and added subregion setup.

 memory.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/memory.c b/memory.c
index aa193df..fb1d859 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"
@@ -860,6 +861,107 @@ void memory_region_init(MemoryRegion *mr,
     mr->name = g_strdup(name);
 }
 
+static void memory_region_get_addr(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    uint64_t value = mr->addr;
+
+    visit_type_uint64(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void memory_region_set_addr(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    uint64_t value;
+
+    visit_type_uint64(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    memory_region_set_address(mr, value);
+}
+
+static void memory_region_set_container(Object *obj, Visitor *v, void *opaque,
+                                        const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    MemoryRegion *old_parent = mr->parent;
+    MemoryRegion *new_parent = NULL;
+    char *path = NULL;
+
+    visit_type_str(v, &path, name, &local_err);
+
+    if (!local_err && strcmp(path, "") != 0) {
+        new_parent = MEMORY_REGION(object_resolve_link(obj, name, path,
+                                   &local_err));
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    object_ref(OBJECT(new_parent));
+
+    memory_region_transaction_begin();
+    memory_region_ref(mr);
+    if (old_parent) {
+        memory_region_del_subregion(old_parent, mr);
+    }
+    mr->parent = new_parent;
+    if (new_parent) {
+        do_memory_region_add_subregion_common(mr);
+    }
+    memory_region_unref(mr);
+    memory_region_transaction_commit();
+
+    object_unref(OBJECT(old_parent));
+}
+
+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->parent) {
+        path = object_get_canonical_path(OBJECT(mr->parent));
+    }
+    visit_type_str(v, &path, name, errp);
+    if (mr->parent) {
+        g_free(path);
+    }
+}
+
+static void memory_region_release_container(Object *obj, const char *name,
+                                            void *opaque)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+
+    if (mr->parent) {
+        memory_region_del_subregion(mr->parent, mr);
+        object_unref(OBJECT(mr->parent));
+    }
+}
+
+static Object *memory_region_resolve_container(Object *obj, void *opaque,
+                                               const char *part)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+
+    return OBJECT(mr->parent);
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -870,6 +972,19 @@ 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,
+                             memory_region_set_container,
+                             memory_region_resolve_container,
+                             memory_region_release_container,
+                             NULL, &error_abort);
+
+    object_property_add(OBJECT(mr), "addr", "uint64",
+                        memory_region_get_addr,
+                        memory_region_set_addr,
+                        NULL, NULL, &error_abort);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 09/10] memory: MemoryRegion: Add may-overlap and priority props

[Peter Crosthwaite]

QOM propertyify the .may-overlap and .priority fields. The setters
will re-add the memory as a subregion if needed (i.e. the values change
when the memory region is already contained).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v2:
Fixed priority getter
Support signed values in priority set/get
changed since v1:
Converted priority to signed type

 include/exec/memory.h |  2 +-
 memory.c              | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 371c066..117c0d3 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -157,7 +157,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 fb1d859..e062108 100644
--- a/memory.c
+++ b/memory.c
@@ -962,6 +962,55 @@ static Object *memory_region_resolve_container(Object *obj, void *opaque,
     return OBJECT(mr->parent);
 }
 
+static void memory_region_get_priority(Object *obj, Visitor *v, void *opaque,
+                                       const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    int32_t value = mr->priority;
+
+    visit_type_int32(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void memory_region_set_priority(Object *obj, Visitor *v, void *opaque,
+                                       const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    int32_t value;
+
+    visit_type_int32(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (mr->priority != value) {
+        mr->priority = value;
+        memory_region_readd_subregion(mr);
+    }
+}
+
+static bool memory_region_get_may_overlap(Object *obj, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+
+    return mr->may_overlap;
+}
+
+static void memory_region_set_may_overlap(Object *obj, bool value, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+
+    if (mr->may_overlap != value) {
+        mr->may_overlap = value;
+        memory_region_readd_subregion(mr);
+    }
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -985,6 +1034,14 @@ static void memory_region_initfn(Object *obj)
                         memory_region_get_addr,
                         memory_region_set_addr,
                         NULL, NULL, &error_abort);
+    object_property_add(OBJECT(mr), "priority", "uint32",
+                        memory_region_get_priority,
+                        memory_region_set_priority,
+                        NULL, NULL, &error_abort);
+    object_property_add_bool(OBJECT(mr), "may-overlap",
+                             memory_region_get_may_overlap,
+                             memory_region_set_may_overlap,
+                             &error_abort);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
-- 
2.0.0

[Qemu-devel] [PATCH memory v4 10/10] memory: MemoryRegion: Add size property

[Peter Crosthwaite]

To allow devices to dynamically resize the device. The motivation is
to allow devices with variable size to init their memory_region
without size early and then correctly populate size at realize() time.

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

 memory.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/memory.c b/memory.c
index e062108..64f2d36 100644
--- a/memory.c
+++ b/memory.c
@@ -1011,6 +1011,40 @@ static void memory_region_set_may_overlap(Object *obj, bool value, Error **errp)
     }
 }
 
+static void memory_region_get_size(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    uint64_t value = int128_get64(mr->size);
+
+    visit_type_uint64(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void memory_region_set_size(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    uint64_t value;
+    Int128 v128;
+
+    visit_type_uint64(v, &value, name, &local_err);
+    v128 = int128_make64(value);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (!int128_eq(v128, mr->size)) {
+        mr->size = v128;
+        memory_region_readd_subregion(mr);
+    }
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -1042,6 +1076,10 @@ static void memory_region_initfn(Object *obj)
                              memory_region_get_may_overlap,
                              memory_region_set_may_overlap,
                              &error_abort);
+    object_property_add(OBJECT(mr), "size", "uint64",
+                        memory_region_get_size,
+                        memory_region_set_size,
+                        NULL, NULL, &error_abort);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
-- 
2.0.0

Re: [Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Paolo Bonzini]

Il 06/06/2014 08:11, Peter Crosthwaite ha scritto:
> Hi Paolo, Andreas,
>
> This patch series QOMifies Memory regions. This is the Memory API
> specific subset of patches forming part of the Memory/GPIO/Sysbus
> QOMification.
>
> I think Paolo already has P1 enqeued. Including for ease of review.
> some QOM patches in P2-4 that cut down on later boilerplate. TBH I can
> live without them, if they not liked but they make life better IMO.
>
> For fuller context please see:
>
> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03265.html
>
> and
>
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00359.html

I'm okay with the gist of the series.  I still have to understand how 
exactly the reference counts work, but the idea is fine.

However, I'd rather have the concept bake for a bit by starting with 
read-only properties.  I think there are some concepts in the 
"container" property that can be generalized.

Would you be okay with that?

Paolo

Re: [Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Peter Crosthwaite]

On Fri, Jun 6, 2014 at 7:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/06/2014 08:11, Peter Crosthwaite ha scritto:
>
>> Hi Paolo, Andreas,
>>
>> This patch series QOMifies Memory regions. This is the Memory API
>> specific subset of patches forming part of the Memory/GPIO/Sysbus
>> QOMification.
>>
>> I think Paolo already has P1 enqeued. Including for ease of review.
>> some QOM patches in P2-4 that cut down on later boilerplate. TBH I can
>> live without them, if they not liked but they make life better IMO.
>>
>> For fuller context please see:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03265.html
>>
>> and
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00359.html
>
>
> I'm okay with the gist of the series.  I still have to understand how
> exactly the reference counts work, but the idea is fine.
>
> However, I'd rather have the concept bake for a bit by starting with
> read-only properties.

The just a matter of pulling our the setters? I guess it can be done
but I don't fully understand the motivation.

  I think there are some concepts in the "container"
> property that can be generalized.
>

Can you elaborate this a little more?

Regards,
Peter

> Would you be okay with that?
>
> Paolo
>

Re: [Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Paolo Bonzini]

Il 06/06/2014 15:36, Peter Crosthwaite ha scritto:
>> However, I'd rather have the concept bake for a bit by starting with
>> read-only properties.
>
> The just a matter of pulling our the setters?

Yes.  You can leave in all the preparatory refactoring.

> I guess it can be done
> but I don't fully understand the motivation.

I'm not sure what the interactions should be between the setters and, 
for example, the "realized" property.  In addition, some memory regions 
(for example PCI BARs) are meant to be moved around by the guest, not 
the host.

By comparison, with getters only not many things can go wrong.

>>  I think there are some concepts in the "container"
>> property that can be generalized.
>
> Can you elaborate this a little more?

Basically the container property acts as a kind of "backdoor" to express 
many:1 relations (which are a bit ugly to express with the current QOM 
property model).  If object X is on the 1 side and object Y is on the 
"many" side, you then write the path to Y to a property of X.

There are obvious similarity between the container/address/visible 
properties of a MemoryRegion are very similar to bus/addr/realized that 
we have for -device.  I think we should try and understand better what 
this similarity means, and whether this approach to many:1 relations 
could become a more central part of QOM.

Paolo

Re: [Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Peter Crosthwaite]

On Sat, Jun 7, 2014 at 12:52 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/06/2014 15:36, Peter Crosthwaite ha scritto:
>
>>> However, I'd rather have the concept bake for a bit by starting with
>>> read-only properties.
>>
>>
>> The just a matter of pulling our the setters?
>
>
> Yes.  You can leave in all the preparatory refactoring.
>
>
>> I guess it can be done
>> but I don't fully understand the motivation.
>
>
> I'm not sure what the interactions should be between the setters and, for
> example, the "realized" property.

Realized of what? MR being just an OBJECT has no realized itself but I
suppose you could think about some correlation to the encapsulating
device (if one even exist - it wont for RAM).

In addition, some memory regions (for
> example PCI BARs) are meant to be moved around by the guest, not the host.
>

So the intention is the free-for-all nature of the non-DEVICE
properties solves this. Some users of MRs are dynamic and can change
setup at run time. That's ok. It leave two options:

1: Properties may be changed at any-time and run-time code (like
PCI-BAR control code) sets the QOM properties.
2: We preserve the current non-QOM API's (e.g.
memory_region_add_subregion) for run-time changers.

If we go with option #2 I guess we can lock down the setters post
realize. A sane policy might be:

1: If the ->owner is a TYPE_DEVICE, setting is OK until the owner is realized.
2: If the owner is not a device, no setting.

Although point 2 makes me sad because it means board code cant use QOM
to wire up RAM.

> By comparison, with getters only not many things can go wrong.
>

True. It's worth nothing however that the setters in the patch do not
change the underlying MR datastructures in any way and all existing
APIs are preserved without QOMification. My reasoning for not
QOMifying the regular MR usage API is pretty much your "let it bake
for a bit". Generally speaking, the setters are yet-to-be-used by
anyone except my follow up RFC work.

>
>>>  I think there are some concepts in the "container"
>>> property that can be generalized.
>>
>>
>> Can you elaborate this a little more?
>
>
> Basically the container property acts as a kind of "backdoor" to express
> many:1 relations (which are a bit ugly to express with the current QOM
> property model).  If object X is on the 1 side and object Y is on the "many"
> side, you then write the path to Y to a property of X.
>

I still maintain that this is valid. The notion that the address space
containers have awareness of the contained but the contained are
oblivious is something of an arbitrary QEMU implementation choice IMO.
It's perfectly valid in real hardware for a bus to be oblivious to
what's on it and the devices attach to it anonymously. The bus then
posts transaction to all attached devices and whoever responds
responds. That's the intended semantic of the 1 linking to the many.
Its the many that is "in the dark".

The 1 -> many linkage is simply inverted to many -> 1 once the
contained region is added to the parent list anyway - just not yet QOM
visible. I think it should work both ways if anything though.
Mandating the many -> 1 unidirectional linkage is awkward IMO.

> There are obvious similarity between the container/address/visible
> properties of a MemoryRegion are very similar to bus/addr/realized that we
> have for -device.

Are you referring to addr as used in Igor's new DIMM work?

  I think we should try and understand better what this
> similarity means, and whether this approach to many:1 relations could become
> a more central part of QOM.
>

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PATCH memory v4 00/10] Memory Region QOMification

[Paolo Bonzini]

Il 07/06/2014 01:50, Peter Crosthwaite ha scritto:
> On Sat, Jun 7, 2014 at 12:52 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 06/06/2014 15:36, Peter Crosthwaite ha scritto:
>>
>>>> However, I'd rather have the concept bake for a bit by starting with
>>>> read-only properties.
>>>
>>>
>>> The just a matter of pulling our the setters?
>>
>>
>> Yes.  You can leave in all the preparatory refactoring.
>>
>>
>>> I guess it can be done
>>> but I don't fully understand the motivation.
>>
>>
>> I'm not sure what the interactions should be between the setters and, for
>> example, the "realized" property.
>
> Realized of what? MR being just an OBJECT has no realized itself but I
> suppose you could think about some correlation to the encapsulating
> device (if one even exist - it wont for RAM).

Yes, realized of the owner (and even /machine could be a device).

> In addition, some memory regions (for
>> example PCI BARs) are meant to be moved around by the guest, not the host.
>
> So the intention is the free-for-all nature of the non-DEVICE
> properties solves this.

Free-for-all in what sense?

> 1: Properties may be changed at any-time and run-time code (like
> PCI-BAR control code) sets the QOM properties.
> 2: We preserve the current non-QOM API's (e.g.
> memory_region_add_subregion) for run-time changers.
>
> If we go with option #2 I guess we can lock down the setters post
> realize.

Yes, possibly.  We can also add an API to "lock" memory regions (or more 
generally objects).  Then realize can use it.

>> By comparison, with getters only not many things can go wrong.
>
> True. It's worth nothing however that the setters in the patch do not
> change the underlying MR datastructures in any way and all existing
> APIs are preserved without QOMification. My reasoning for not
> QOMifying the regular MR usage API is pretty much your "let it bake
> for a bit". Generally speaking, the setters are yet-to-be-used by
> anyone except my follow up RFC work.

But also in principle the user could use them via qom-set, with 
interesting effects. :)

>>> Can you elaborate this a little more?
>>
>> Basically the container property acts as a kind of "backdoor" to express
>> many:1 relations (which are a bit ugly to express with the current QOM
>> property model).  If object X is on the 1 side and object Y is on the "many"
>> side, you then write the path to Y to a property of X.
>
> I still maintain that this is valid.  Mandating the many -> 1 unidirectional
> linkage is awkward IMO.

I certainly agree, and in fact would like to use it more!

In any case, even if this series is committed without setters, it's a 
good thing.  It's already a point of no return. :)

One thing that is missing is more documentation of the reference 
counting policies, which are important for unpluggable devices.  IIUC 
you are relying on unrealize (and hence unparent) removing the memory 
region from the address spaces.  Then when the child property for the 
memory region is removed at finalize time, the MR dies.  Is this 
correct?  If so, this might even let us remove memory_region_destroy 
altogether.  It might also lead us to implementing floating references 
like GObject's.

Also, the difference between memory_region_ref(mr) and object_ref(mr) is 
important.  Is there a case where we want the latter except as an 
internal implementation detail?  I think no, but I might be wrong.

>> There are obvious similarity between the container/address/visible
>> properties of a MemoryRegion are very similar to bus/addr/realized that we
>> have for -device.
>
> Are you referring to addr as used in Igor's new DIMM work?

I was thinking of PCI addresses, actually.

Paolo