[Qemu-devel] [PATCH memory v3 0/9] Memory Region QOMification

[Qemu-devel] [PATCH memory v3 0/9] 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-3 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

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.


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  |  16 +++
 memory.c              | 307 +++++++++++++++++++++++++++++++++++++++++++-------
 qom/object.c          |  27 ++---
 4 files changed, 298 insertions(+), 60 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH memory v3 1/9] 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 3f1df23..1352881 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 v3 2/9] 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 e42b254..ec5adf4 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -714,11 +714,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 */
@@ -1119,13 +1125,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 void object_release_link_property(Object *obj, const char *name,
-- 
2.0.0

[Qemu-devel] [PATCH memory v3 3/9] 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 a641dcd..7f96ecf 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1048,6 +1048,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 ec5adf4..b9b2736 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1058,17 +1058,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 v3 4/9] 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 1352881..dd0a576 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 v3 5/9] 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 dd0a576..aa25667 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 v3 6/9] 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 v2:
use object_unref() to Destroy MRs properly (Paolo Review)
Drop stale FIXME

 include/exec/memory.h |  6 ++++++
 memory.c              | 53 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 40 insertions(+), 19 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 aa25667..90c0557 100644
--- a/memory.c
+++ b/memory.c
@@ -850,35 +850,27 @@ 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 +1091,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 +1103,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;
@@ -1886,3 +1886,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

Re: [Qemu-devel] [PATCH memory v3 6/9] memory: MemoryRegion: QOMify

[Peter Maydell]

On 2 June 2014 05:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 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

> +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);

We rely on QOM objects being zero-initialized for other
fields, so why leave the explicit memset for this one?

thanks
-- PMM

Re: [Qemu-devel] [PATCH memory v3 6/9] memory: MemoryRegion: QOMify

[Peter Crosthwaite]

On Mon, Jun 2, 2014 at 10:18 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 June 2014 05:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> 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
>
>> +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);
>
> We rely on QOM objects being zero-initialized for other
> fields, so why leave the explicit memset for this one?
>

It slipped through the cracks in my scan for 0s. Will Fix.

Regards,
Peter

> thanks
> -- PMM
>

[Qemu-devel] [PATCH memory v3 7/9] 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 v2:
Removed un-needed strdup
changed since v1:
Converted container to low level link property and added subregion setup.

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

diff --git a/memory.c b/memory.c
index 90c0557..0296675 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,99 @@ 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 void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
@@ -871,6 +965,17 @@ static void memory_region_initfn(Object *obj)
     QTAILQ_INIT(&mr->subregions);
     memset(&mr->subregions_link, 0, sizeof mr->subregions_link);
     QTAILQ_INIT(&mr->coalesced);
+
+    object_property_add(OBJECT(mr), "container", "link<" TYPE_MEMORY_REGION ">",
+                        memory_region_get_container,
+                        memory_region_set_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 v3 8/9] 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 0296675..2d98020 100644
--- a/memory.c
+++ b/memory.c
@@ -954,6 +954,55 @@ static void memory_region_release_container(Object *obj, const char *name,
     }
 }
 
+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);
@@ -976,6 +1025,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 v3 9/9] 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 2d98020..54fdbb2 100644
--- a/memory.c
+++ b/memory.c
@@ -1003,6 +1003,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);
@@ -1033,6 +1067,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 v3 0/9] Memory Region QOMification

[Peter Maydell]

On 2 June 2014 05:13, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 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-3 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

So could you sketch an example of how this would work
(for board model construction, not command line arguments)?
I'm guessing something like:
   dev = qdev_create(NULL, "my-device");
   object_property_set_something(OBJECT(dev), "container", my_memregion);
   object_property_set_uint64(OBJECT(dev), "addr", 0x40000);
   qdev_init_nofail(dev);

Code wise it looks OK but it feels oddly back-to-front to
put a subregion into a container by setting properties on
the subregion. At least personally I think of the mapping
operation as an operation on the container that says "put
this object X in at address Y", not as an operation on
the object X that says "your container is C and you're
at address Y in it". But I can see how this approach
pretty much falls out of our current MemoryRegion data
structure, so perhaps I just need to reverse the
orientation of my brain...

(Also it doesn't make sense to set only one of the
(container,address) property pair, but I guess two
properties which we can already handle makes more sense
than having one which would need a custom parser of some
kind.)

The other question is how you see this fitting into our
other use-case for passing MemoryRegions around: what
would the code for passing a container region to a
memory transaction source like a CPU object look like?

thanks
-- PMM

Re: [Qemu-devel] [PATCH memory v3 0/9] Memory Region QOMification

[Peter Crosthwaite]

On Tue, Jun 3, 2014 at 12:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 June 2014 05:13, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> 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-3 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
>
> So could you sketch an example of how this would work
> (for board model construction, not command line arguments)?
> I'm guessing something like:
>    dev = qdev_create(NULL, "my-device");
>    object_property_set_something(OBJECT(dev), "container", my_memregion);

object_property_set_link

Although my current thinking is the slaves would not handle the actual
containering device-side like this. It would expose the slave aperture
via sysbus and let the machine do it.

dev = qdev_create(NULL, "my-device");
memory_region_add_subregion(my_memory_region,
        sysbus_get_mmio(SYSBUS_DEVICE(dev), 0), 0xF00);

And we patch the existing public memory_region_add_subregion() API to
back onto the new link setters  (that one is a follow up that I
haven't got to just yet).

The machine could also just do the QOM properties itself although it
may be too verbose:

dev = qdev_create(NULL, "my-device");
MemoryRegion *slave_ap = sysbus_get_mmio(SBD(dev), n);
object_property_set_link(slave_ap, my_memregion, "container", &error_abort);
object_property_set_int(slave_ap, 0xF00, "addr", &error_abort);
/* priority and may overlap could be set too .. */

Size would be set by the device itself in init or realize (preserves
current semantics).

I have some patches demoing this. Will RFC.

>    object_property_set_uint64(OBJECT(dev), "addr", 0x40000);
>    qdev_init_nofail(dev);
>
> Code wise it looks OK but it feels oddly back-to-front to
> put a subregion into a container by setting properties on
> the subregion. At least personally I think of the mapping
> operation as an operation on the container that says "put
> this object X in at address Y", not as an operation on
> the object X that says "your container is C and you're
> at address Y in it". But I can see how this approach
> pretty much falls out of our current MemoryRegion data
> structure, so perhaps I just need to reverse the
> orientation of my brain...
>

If it helps, you can think of it as a slave decoded bus. It's
reasonable to implement a memory mapped bus (the "container" in this
case) as "im just a dumb bus that sits there and spams addresses and
data to everyone and see who responds" and leave the actual address
decoding to the many slaves. So IMO, this approach where the slaves
understand there mappings can have a very real hardware analogue.
Flatview generation will invert this relationship for the actual QEMU
implementation, but an API where slaves insert themselves into a space
is valid.

> (Also it doesn't make sense to set only one of the
> (container,address) property pair, but I guess two
> properties which we can already handle makes more sense
> than having one which would need a custom parser of some
> kind.)
>
> The other question is how you see this fitting into our
> other use-case for passing MemoryRegions around: what
> would the code for passing a container region to a
> memory transaction source like a CPU object look like?
>

I have those patches already too. Will RFC it along with the rest. My
series converts a board (microblaze ML605) to not use
&address_space_memory at all. Includes a DMA controller as well. Cover
letter will elaborate some of your questions.

Regards,
Peter


> thanks
> -- PMM
>