Memory and GPIO QOMification

RFC In review Peter Crosthwaite 2 participants 31 messages May 16, 2014 – May 26, 2014 Arm/Zynq architecture

lore

Share & tools

lore

Peter Crosthwaite

author

2014-05-16 01:52 UTC #0 lore raw

cover Memory and GPIO QOMification

Hi All,

This series is my Memory and GPIO qomification as promised. This was discussed at length on the KVM call 2014-05-13.

This is the preview RFC designed to show the capability of the full solution. There are many parts to this that stand in their own right and the merge would be done in smaller chunks (E.G. Memory and GPIO works can go separately).

Theres some Microblaze stuff at the back of the series, leading to an example of sysbus hotplug.

Some patches are already on list. I put those at the front of the series.

Only build tested for microblaze. There's some periphery build issues for fully configured builds that i'll figure should we get some conceptual level acceptance of this whole idea.

configure --target-list=microblaze-softmmu

to play with it.

Contents:

QOMify Memory regions. So they are added as child objects to devices. Devices can do this explicitly in instance_init, or sysbus can handle it - sysbus_init_mmio parents the memory region to the device to transparently convert all existing devs to compliance.

This prepares support for the Memory heirachy work. Machines can create memory regions matching the bus architectures. Masters and slaves can gen hand these to devices as proper Links.

The address and container (the memory region containing this one as a subregion) of memory regions are QOM properties, of type integer and link resp. Setting the properties does the memory_region_add_subregion().

The root Memory Region is parented the machine in exec.c. This give the Memory Region a canonical path.

Sysbus IRQ are abandoned completely and re-implemented as named GPIOs. The sysbus irq API remains and makes this transition behind-the-scenes.

GPIOs are QOMified. qemu_allocate_irqs does the object_new() etc. IRQ inputs are then added as child objects of the instantiating device. Handled by qemu_init_gpio_in_named(). gpio_outs are setup as links. qdev_connect_gpio_out does the linkage.

QOM is patched to allow setting of a device's child's properties from a ref to the parent. Best illustrated by example (see below).

Anyways without a single patch to the command line interface, HMP, QMP, or implementing any machine specific hotplug or adding any new special busses, this works:

-device xlnx.xps-timer,\
sysbus-mr-0/container=/machine/sysmem,\
sysbus-mr-0/addr=0x83c00000,\
sysbus-irq-0=/machine/intc/unnamed-gpio-0

All the other ways to create devices should just work, this is not a command line specific feature.

Note, I edited my machine model to sanitize the canonical path of the interrupt controller.

hw/microblaze/petalogix_s3adsp1800_mmu.c +2 -0

@@ -97,6 +97,8 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
                           1, 0x89, 0x18, 0x0000, 0x0, 1);
 
     dev = qdev_create(NULL, "xlnx.xps-intc");
+    object_property_add_child(qdev_get_machine(), "intc", OBJECT(dev),
+                              &error_abort);

But you could have done the whole /machine/unattached/... ugliness too. If you name the irq inputs in the intc with my named GPIO series stuff the unnamed-gpio ugliness goes away too. If you throw away sysbus and do the memory-regions and IRQs naming the child objects properly the ugly sysbus names can de dispensed with too. But thats a big tree-wide to name everything properly.

Andreas Färber (1):
irq: Slim conversion of qemu_irq to QOM [WIP]

Peter Crosthwaite (24):
qdev: Implement named GPIOs
memory: Simplify mr_add_subregion() if-else
memory: Coreify subregion add functionality
memory: MemoryRegion: QOMify
memory: MemoryRegion: Add contained flag
memory: MemoryRegion: Add container and addr props
memory: MemoryRegion: factor out memory region re-adder
memory: MemoryRegion: Add may-overlap and priority props
memory: MemoryRegion: Add size property
exec: Parent root MRs to the machine
qdev: gpio: Don't allow name share between I and O
qdev: gpio: Register GPIO inputs as child objects
qdev: gpio: Register GPIO outputs as QOM links
qdev: gpio: Re-impement qdev_connect_gpio QOM style
qom: object_property_set/get: Add child recursion
sysbus: Use TYPE_DEVICE GPIO functionality
sysbus: Rework sysbus_mmio_map to use mr QOMification
sysbus: Setup memory regions as dynamic props
sysbus: Enable hotplug.
microblaze: s3adsp: Expand UART creator
microblaze: s3adsp: Parent devices with sane names
timer: xilinx_timer: Convert to realize()
timer: xilinx_timer: init MMIO ASAP
TEST: microblaze: s3adsp: Remove timer

 exec.c                                   |   4 +
 hw/core/irq.c                            |  44 +++++-
 hw/core/qdev.c                           | 107 +++++++++++--
 hw/core/sysbus.c                         |  85 ++++------
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  16 +-
 hw/timer/xilinx_timer.c                  |  26 ++--
 include/exec/memory.h                    |   9 +-
 include/hw/irq.h                         |   2 +
 include/hw/qdev-core.h                   |  24 ++-
 include/hw/sysbus.h                      |  12 +-
 memory.c                                 | 258 +++++++++++++++++++++++++------
 qdev-monitor.c                           |  16 +-
 qom/object.c                             |  27 +++-
 qtest.c                                  |  19 ++-
 14 files changed, 500 insertions(+), 149 deletions(-)

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:53 UTC #1 lore raw

RFC 1/25 qdev: Implement named GPIOs

Implement named GPIOs on the Device layer. Listifies the existing GPIOs stuff using string keys. Legacy un-named GPIOs are preserved by using a NULL name string - they are just a single matchable element in the name list.

Signed-off-by Peter Crosthwaite <[email protected]>

---
Changed since v3 (PMM review):
Added finalize() cleanup of all allocated mem Remove %s printf of NULL in info qtree Restricted Qtest to unnamed GPIOs only
Changed since v2:
Reordered fn args to be name-before-number (PMM review) changed since v1:
Use QLIST instead of RYO list implementation (AF review) Reorder struct fields for consistency

 hw/core/qdev.c         | 82 ++++++++++++++++++++++++++++++++++++++++++++------
 include/hw/qdev-core.h | 24 ++++++++++++---
 qdev-monitor.c         | 16 +++++++---
 qtest.c                | 19 +++++++++---
 4 files changed, 117 insertions(+), 24 deletions(-)

hw/core/qdev.c +72 ~~-10~~

index 936eae6..8efeb04 100644
@@ -312,30 +312,82 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
     return dev->parent_bus;
 }
 
+static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
+                                               const char *name)
+{
+    NamedGPIOList *ngl;
+
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        /* NULL is a valid and matchable name, otherwise do a normal
+         * strcmp match.
+         */
+        if ((!ngl->name && !name) ||
+                (name && ngl->name && !strcmp(name, ngl->name))) {
+            return ngl;
+        }
+    }
+
+    ngl = g_malloc0(sizeof(*ngl));
+    ngl->name = g_strdup(name);
+    QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
+    return ngl;
+}
+
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
+                                     dev, n);
+    gpio_list->num_in += n;
+}
+
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 {
-    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
-                                        dev, n);
-    dev->num_gpio_in += n;
+    qdev_init_gpio_in_named(dev, handler, NULL, n);
+}
+
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(gpio_list->num_out == 0);
+    gpio_list->num_out = n;
+    gpio_list->out = pins;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
 {
-    assert(dev->num_gpio_out == 0);
-    dev->num_gpio_out = n;
-    dev->gpio_out = pins;
+    qdev_init_gpio_out_named(dev, pins, NULL, n);
+}
+
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_in);
+    return gpio_list->in[n];
 }
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
 {
-    assert(n >= 0 && n < dev->num_gpio_in);
-    return dev->gpio_in[n];
+    return qdev_get_gpio_in_named(dev, NULL, n);
+}
+
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_out);
+    gpio_list->out[n] = pin;
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 {
-    assert(n >= 0 && n < dev->num_gpio_out);
-    dev->gpio_out[n] = pin;
+    qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)

@@ -844,6 +896,7 @@ static void device_initfn(Object *obj)
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL, 0,
                              &error_abort);
+    QLIST_INIT(&dev->gpios);
 }
 
 static void device_post_init(Object *obj)

@@ -854,10 +907,19 @@ static void device_post_init(Object *obj)
 /* Unlink device from bus and free the structure.  */
 static void device_finalize(Object *obj)
 {
+    NamedGPIOList *ngl, *next;
+
     DeviceState *dev = DEVICE(obj);
     if (dev->opts) {
         qemu_opts_del(dev->opts);
     }
+
+    QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
+        QLIST_REMOVE(ngl, node);
+        qemu_free_irqs(ngl->in);
+        g_free(ngl->name);
+        g_free(ngl);
+    }
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)

include/hw/qdev-core.h +20 -4

index dbe473c..ae31575 100644
@@ -131,6 +131,17 @@ typedef struct DeviceClass {
     const char *bus_type;
 } DeviceClass;
 
+typedef struct NamedGPIOList NamedGPIOList;
+
+struct NamedGPIOList {
+    char *name;
+    qemu_irq *in;
+    int num_in;
+    qemu_irq *out;
+    int num_out;
+    QLIST_ENTRY(NamedGPIOList) node;
+};
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.

@@ -148,10 +159,7 @@ struct DeviceState {
     QemuOpts *opts;
     int hotplugged;
     BusState *parent_bus;
-    int num_gpio_out;
-    qemu_irq *gpio_out;
-    int num_gpio_in;
-    qemu_irq *gpio_in;
+    QLIST_HEAD(, NamedGPIOList) gpios;
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     int instance_id_alias;

@@ -252,7 +260,11 @@ void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
+
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin);
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);

@@ -262,6 +274,10 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 /* GPIO inputs also double as IRQ sinks.  */
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n);
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n);
 
 BusState *qdev_get_parent_bus(DeviceState *dev);

qdev-monitor.c +11 -5

index 02cbe43..f87f3d8 100644
@@ -613,14 +613,20 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     ObjectClass *class;
     BusState *child;
+    NamedGPIOList *ngl;
+
     qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
                 dev->id ? dev->id : "");
     indent += 2;
-    if (dev->num_gpio_in) {
-        qdev_printf("gpio-in %d\n", dev->num_gpio_in);
-    }
-    if (dev->num_gpio_out) {
-        qdev_printf("gpio-out %d\n", dev->num_gpio_out);
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->num_in) {
+            qdev_printf("gpio-in \"%s\" %d\n", ngl->name ? ngl->name : "",
+                        ngl->num_in);
+        }
+        if (ngl->num_out) {
+            qdev_printf("gpio-out \"%s\" %d\n", ngl->name ? ngl->name : "",
+                        ngl->num_out);
+        }
     }
     class = object_get_class(OBJECT(dev));
     do {

qtest.c +14 -5

index 2aba20d..b6b60d5 100644
@@ -233,7 +233,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
     g_assert(command);
     if (strcmp(words[0], "irq_intercept_out") == 0
         || strcmp(words[0], "irq_intercept_in") == 0) {
-	DeviceState *dev;
+        DeviceState *dev;
+        NamedGPIOList *ngl;
 
         g_assert(words[1]);
         dev = DEVICE(object_resolve_path(words[1], NULL));

@@ -253,10 +254,18 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
 	    return;
         }
 
-        if (words[0][14] == 'o') {
-            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, dev->num_gpio_out);
-        } else {
-            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, dev->num_gpio_in);
+        QLIST_FOREACH(ngl, &dev->gpios, node) {
+            /* We dont support intercept of named GPIOs yet */
+            if (ngl->name) {
+                continue;
+            }
+            if (words[0][14] == 'o') {
+                qemu_irq_intercept_out(&ngl->out, qtest_irq_handler,
+                                       ngl->num_out);
+            } else {
+                qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
+                                      ngl->num_in);
+            }
         }
         irq_intercept_dev = dev;
         qtest_send_prefix(chr);

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:53 UTC #2 lore raw

RFC 2/25 memory: Simplify mr_add_subregion() if-else

In reply to 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 <[email protected]>

---

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

memory.c +1 -7

index 3f1df23..1352881 100644
@@ -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();
 }

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:54 UTC #3 lore raw

RFC 3/25 memory: Coreify subregion add functionality

In reply to 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 <[email protected]>

---

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

memory.c +12 -6

index 1352881..dd0a576 100644
@@ -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,

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:54 UTC #4 lore raw

RFC 4/25 memory: MemoryRegion: QOMify

In reply to 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 <[email protected]>

---

 include/exec/memory.h |  6 ++++++
 memory.c              | 55 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 42 insertions(+), 19 deletions(-)

include/exec/memory.h +6 -0

index 1d55ad9..371c066 100644
@@ -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;

memory.c +36 ~~-19~~

88 lines not on this page · raw message

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:55 UTC #5 lore raw

RFC 5/25 memory: MemoryRegion: Add contained flag

In reply to Peter Crosthwaite

Rather than use the .parent == NULL check to determine if a memory region is contained, add a purpose specific boolean flag. This allows for .parent to be easily converted to a link property while preserving all the semantics of the legacy API.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 include/exec/memory.h | 1 +
 memory.c              | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

include/exec/memory.h +1 -0

index 371c066..2bb074f 100644
@@ -142,6 +142,7 @@ struct MemoryRegion {
     void *opaque;
     struct Object *owner;
     MemoryRegion *parent;
+    bool contained;
     Int128 size;
     hwaddr addr;
     void (*destructor)(MemoryRegion *mr);

memory.c +5 -4

index 3c32d5a..a37fdd2 100644
@@ -1451,6 +1451,7 @@ static void do_memory_region_add_subregion_common(MemoryRegion *subregion)
     QTAILQ_INSERT_TAIL(&mr->subregions, subregion, subregions_link);
 done:
     memory_region_update_pending |= mr->enabled && subregion->enabled;
+    subregion->contained = true;
     memory_region_transaction_commit();
 }

@@ -1487,8 +1488,8 @@ void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion)
 {
     memory_region_transaction_begin();
-    assert(subregion->parent == mr);
-    subregion->parent = NULL;
+    assert(subregion->parent == mr && subregion->contained);
+    subregion->contained = false;
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
     memory_region_unref(subregion);
     memory_region_update_pending |= mr->enabled && subregion->enabled;

@@ -1510,7 +1511,7 @@ void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
 {
     MemoryRegion *parent = mr->parent;
 
-    if (addr == mr->addr || !parent) {
+    if (addr == mr->addr || !mr->contained) {
         mr->addr = addr;
         return;
     }

@@ -1518,7 +1519,7 @@ void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
     memory_region_transaction_begin();
     memory_region_ref(mr);
     memory_region_del_subregion(parent, mr);
-    memory_region_add_subregion_common(parent, addr, mr);
+    do_memory_region_add_subregion_common(mr);
     memory_region_unref(mr);
     memory_region_transaction_commit();
 }

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:55 UTC #6 lore raw

RFC 6/25 memory: MemoryRegion: Add container and addr props

In reply to Peter Crosthwaite

Expose the already existing .parent and .addr fields as QOM properties. Setting the address will cause the memory subregion adding to happen if it has not already. If the memory region is already contained, then change it's address as per the established memory_region_set_address() semantics.

Signed-off-by Peter Crosthwaite <[email protected]>

---

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

memory.c +54 -0

index a37fdd2..4a70920 100644
@@ -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"

@@ -844,6 +845,49 @@ void memory_region_init(MemoryRegion *mr,
     mr->name = g_strdup(name);
 }
 
+static void do_memory_region_add_subregion_common(MemoryRegion *subregion);
+
+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;
+    }
+
+    /* In an ideal world we wait until both the parent and address is set in
+     * either order, then call do_memory_region_add_subregion_common()
+     * from the latter property setter. However, as parent is just a dumb link
+     * we only support setting from the address setter. We therefore assert
+     * here that the two props are set in correct order.
+     */
+    assert(mr->parent);
+    if (!mr->contained) {
+        mr->addr = value;
+        do_memory_region_add_subregion_common(mr);
+    } else {
+        memory_region_set_address(mr, value);
+    }
+}
+
 static void memory_region_initfn(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);

@@ -855,6 +899,16 @@ 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_link(OBJECT(mr), "container", TYPE_MEMORY_REGION,
+                             (Object **)&mr->parent,
+                             object_property_allow_set_link,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &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,

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:56 UTC #7 lore raw

RFC 7/25 memory: MemoryRegion: factor out memory region re-adder

In reply to 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 <[email protected]>

---

 memory.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

memory.c +16 ~~-12~~

index 4a70920..9b9cfad 100644
@@ -829,6 +829,20 @@ 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)
+{
+    if (mr->contained) {
+        memory_region_transaction_begin();
+        memory_region_ref(mr);
+        memory_region_del_subregion(mr->parent, mr);
+        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,

@@ -845,7 +859,6 @@ void memory_region_init(MemoryRegion *mr,
     mr->name = g_strdup(name);
 }
 
-static void do_memory_region_add_subregion_common(MemoryRegion *subregion);
 
 static void memory_region_get_addr(Object *obj, Visitor *v, void *opaque,
                                    const char *name, Error **errp)

@@ -1563,19 +1576,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 || !mr->contained) {
+    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);
-    do_memory_region_add_subregion_common(mr);
-    memory_region_unref(mr);
-    memory_region_transaction_commit();
 }
 
 void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset)

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:57 UTC #8 lore raw

RFC 8/25 memory: MemoryRegion: Add may-overlap and priority props

In reply to 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 <[email protected]>

---

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

include/exec/memory.h +1 -1

index 2bb074f..1f12c15 100644
@@ -158,7 +158,7 @@ struct MemoryRegion {
     bool flush_coalesced_mmio;
     MemoryRegion *alias;
     hwaddr alias_offset;
-    int priority;
+    uint32_t priority;
     bool may_overlap;
     QTAILQ_HEAD(subregions, MemoryRegion) subregions;
     QTAILQ_ENTRY(MemoryRegion) subregions_link;

memory.c +57 -1

76 lines not on this page · raw message

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-26 01:44 UTC #9 lore raw

Re: [Qemu-devel] [RFC v1 08/25] memory: MemoryRegion: Add may-overlap and priority props

Quoted 16 lines · 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 <[email protected]>
---
 include/exec/memory.h |  2 +-
 memory.c              | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 2 deletions(-)
include/exec/memory.h +0 -0
index 2bb074f..1f12c15 100644

include/exec/memory.h struct MemoryRegion { · line 161

Quoted 5 lines

include/exec/memory.h +1 -1

@@ -158,7 +158,7 @@ struct MemoryRegion {
     bool flush_coalesced_mmio;
     MemoryRegion *alias;
     hwaddr alias_offset;
-    int priority;
+    uint32_t priority;

It appears as if -ve memory region priorities are valid and work. Will preserve this and change to this type to int32_t.

Regards,
Peter

Quoted 15 lines

include/exec/memory.h +0 -0

     bool may_overlap;
     QTAILQ_HEAD(subregions, MemoryRegion) subregions;
     QTAILQ_ENTRY(MemoryRegion) subregions_link;

memory.c +0 -1

index 9b9cfad..e1236c1 100644
@@ -859,7 +859,6 @@ void memory_region_init(MemoryRegion *mr,
     mr->name = g_strdup(name);
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
--

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:57 UTC #10 lore raw

RFC 9/25 memory: MemoryRegion: Add size property

In reply to 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 <[email protected]>

---

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

memory.c +38 -0

index e1236c1..2ff9ab7 100644
@@ -949,6 +949,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);

@@ -978,6 +1012,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,

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:58 UTC #11 lore raw

RFC 10/25 exec: Parent root MRs to the machine

In reply to Peter Crosthwaite

Parent the root MemoryRegions for Memory and IO to the machine. This gives them a QOM path.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 exec.c | 4 ++++
 1 file changed, 4 insertions(+)

exec.c +4 -0

index cf12049..7ffca81 100644
@@ -1886,11 +1886,15 @@ static void memory_map_init(void)
     system_memory = g_malloc(sizeof(*system_memory));
 
     memory_region_init(system_memory, NULL, "system", UINT64_MAX);
+    object_property_add_child(qdev_get_machine(), "sysmem",
+                              OBJECT(system_memory), &error_abort);
     address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
     memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
                           65536);
+    object_property_add_child(qdev_get_machine(), "sysio",
+                              OBJECT(system_io), &error_abort);
     address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:58 UTC #12 lore raw

RFC 11/25 irq: Slim conversion of qemu_irq to QOM [WIP]

In reply to Peter Crosthwaite

From: Andreas Färber <[email protected]>

As a prequel to any big Pin refactoring plans, do an in-place conversion of qemu_irq to an Object, so that we can reference it in link<> properties.

Signed-off-by Andreas Färber <[email protected]>

---

 hw/core/irq.c    | 44 +++++++++++++++++++++++++++++++++++++++++---
 include/hw/irq.h |  2 ++
 2 files changed, 43 insertions(+), 3 deletions(-)

hw/core/irq.c +41 -3

index 03c8cb3..0bcd27b 100644
@@ -23,8 +23,13 @@
  */
 #include "qemu-common.h"
 #include "hw/irq.h"
+#include "qom/object.h"
+
+#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
 
 struct IRQState {
+    Object parent_obj;
+
     qemu_irq_handler handler;
     void *opaque;
     int n;

@@ -38,6 +43,14 @@ void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
+static void irq_nonfirst_free(void *obj)
+{
+    struct IRQState *s = obj;
+
+    /* Unreference the first IRQ in this array */
+    object_unref(OBJECT(s - s->n));
+}
+
 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {

@@ -51,11 +64,23 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
     p = old ? g_renew(struct IRQState, s[0], n + n_old) :
                 g_new(struct IRQState, n);
+    memset(p + n_old, 0, n * sizeof(*p));
     for (i = 0; i < n + n_old; i++) {
         if (i >= n_old) {
+            Object *obj;
+
+            object_initialize(p, sizeof(*p), TYPE_IRQ);
             p->handler = handler;
             p->opaque = opaque;
             p->n = i;
+            obj = OBJECT(p);
+            /* Let the first IRQ clean them all up */
+            if (unlikely(i == 0)) {
+                obj->free = g_free;
+            } else {
+                object_ref(OBJECT(s[0]));
+                obj->free = irq_nonfirst_free;
+            }
         }
         s[i] = p;
         p++;

@@ -72,7 +97,7 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
 {
     struct IRQState *irq;
 
-    irq = g_new(struct IRQState, 1);
+    irq = IRQ(object_new(TYPE_IRQ));
     irq->handler = handler;
     irq->opaque = opaque;
     irq->n = n;

@@ -82,13 +107,13 @@ qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
 
 void qemu_free_irqs(qemu_irq *s)
 {
-    g_free(s[0]);
+    object_unref(OBJECT(s[0]));
     g_free(s);
 }
 
 void qemu_free_irq(qemu_irq irq)
 {
-    g_free(irq);
+    object_unref(OBJECT(irq));
 }
 
 static void qemu_notirq(void *opaque, int line, int level)

@@ -150,3 +175,16 @@ void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n
     qemu_irq *old_irqs = *gpio_out;
     *gpio_out = qemu_allocate_irqs(handler, old_irqs, n);
 }
+
+static const TypeInfo irq_type_info = {
+   .name = TYPE_IRQ,
+   .parent = TYPE_OBJECT,
+   .instance_size = sizeof(struct IRQState),
+};
+
+static void irq_register_types(void)
+{
+    type_register_static(&irq_type_info);
+}
+
+type_init(irq_register_types)

include/hw/irq.h +2 -0

index d08bc02..ac76ea7 100644
@@ -3,6 +3,8 @@
 
 /* Generic IRQ/GPIO pin infrastructure.  */
 
+#define TYPE_IRQ "irq"
+
 typedef struct IRQState *qemu_irq;
 
 typedef void (*qemu_irq_handler)(void *opaque, int n, int level);

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-19 01:52 UTC #13 lore raw

Re: [Qemu-devel] [RFC v1 11/25] irq: Slim conversion of qemu_irq to QOM [WIP]

Quoted 46 lines · Peter Crosthwaite

From: Andreas Färber <[email protected]>

As a prequel to any big Pin refactoring plans, do an in-place conversion of qemu_irq to an Object, so that we can reference it in link<> properties.

Signed-off-by Andreas Färber <[email protected]>

---

 hw/core/irq.c    | 44 +++++++++++++++++++++++++++++++++++++++++---
 include/hw/irq.h |  2 ++
 2 files changed, 43 insertions(+), 3 deletions(-)

hw/core/irq.c +13 -0

index 03c8cb3..0bcd27b 100644
@@ -23,8 +23,13 @@
  */
 #include "qemu-common.h"
 #include "hw/irq.h"
+#include "qom/object.h"
+
+#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
 
 struct IRQState {
+    Object parent_obj;
+
     qemu_irq_handler handler;
     void *opaque;
     int n;

@@ -38,6 +43,14 @@ void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
+static void irq_nonfirst_free(void *obj)
+{
+    struct IRQState *s = obj;
+
+    /* Unreference the first IRQ in this array */
+    object_unref(OBJECT(s - s->n));
+}
+
 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {

hw/core/irq.c qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler, · line 66

Quoted 3 lines

hw/core/irq.c +0 -0

@@ -51,11 +64,23 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
     p = old ? g_renew(struct IRQState, s[0], n + n_old) :
                 g_new(struct IRQState, n);

So using g_renew on the actual object storage is very fragile, as it means you cannot reliably take pointers to the objects. I think however this whole issue could be simplified by de-arrayifying the whole thing, and treating IRQs individually (interdiff):

qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
void *opaque, int n)

 {
     qemu_irq *s;
-    struct IRQState *p;
     int i;

     if (!old) {
         n_old = 0;
     }
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
-    p = old ? g_renew(struct IRQState, s[0], n + n_old) :
-                g_new(struct IRQState, n);
-    memset(p + n_old, 0, n * sizeof(*p));
     for (i = 0; i < n + n_old; i++) {
         if (i >= n_old) {
-            Object *obj;
-
-            object_initialize(p, sizeof(*p), TYPE_IRQ);
-            p->handler = handler;
-            p->opaque = opaque;
-            p->n = i;
-            obj = OBJECT(p);
-            /* Let the first IRQ clean them all up */
-            if (unlikely(i == 0)) {

- obj->free = g_free;
- } else {
- object_ref(OBJECT(s[0]));
- obj->free = irq_nonfirst_free;
- }

+            s[i] = qemu_allocate_irq(handler, opaque, i);
         }
-        s[i] = p;
-        p++;
     }
     return s;

The system for freeing may need some thought, and I wonder if their are API clients out there assuming the IRQ storage elements are contiguous (if there are they have too much internal knowledge and need to be fixed).

But with this change these objects are now usable with links and canon paths etc.

Will roll into V2 of this patch.

Regards,
Peter

Andreas Färber

2014-05-19 09:13 UTC #14 lore raw

Re: [Qemu-devel] [RFC v1 11/25] irq: Slim conversion of qemu_irq to QOM [WIP]

Am 19.05.2014 03:52, schrieb Peter Crosthwaite:

Quoted 8 lines

Quoted 6 lines · Peter Crosthwaite
From: Andreas Färber <[email protected]>
As a prequel to any big Pin refactoring plans, do an in-place conversion of qemu_irq to an Object, so that we can reference it in link<> properties.
Signed-off-by Andreas Färber <[email protected]>

(Note that you forgot to sign off here.)

Quoted 91 lines

Quoted 40 lines

---

 hw/core/irq.c    | 44 +++++++++++++++++++++++++++++++++++++++++---
 include/hw/irq.h |  2 ++
 2 files changed, 43 insertions(+), 3 deletions(-)

hw/core/irq.c +13 -0

index 03c8cb3..0bcd27b 100644
@@ -23,8 +23,13 @@
  */
 #include "qemu-common.h"
 #include "hw/irq.h"
+#include "qom/object.h"
+
+#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
 
 struct IRQState {
+    Object parent_obj;
+
     qemu_irq_handler handler;
     void *opaque;
     int n;

@@ -38,6 +43,14 @@ void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
+static void irq_nonfirst_free(void *obj)
+{
+    struct IRQState *s = obj;
+
+    /* Unreference the first IRQ in this array */
+    object_unref(OBJECT(s - s->n));
+}
+
 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {

hw/core/irq.c qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler, · line 66

Quoted 3 lines

hw/core/irq.c +0 -0

@@ -51,11 +64,23 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
     p = old ? g_renew(struct IRQState, s[0], n + n_old) :
                 g_new(struct IRQState, n);

qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
void *opaque, int n)

 {
     qemu_irq *s;

(quoted diff) +1 ~~-20~~

-    struct IRQState *p;
     int i;
 
     if (!old) {
         n_old = 0;
     }
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
-    p = old ? g_renew(struct IRQState, s[0], n + n_old) :
-                g_new(struct IRQState, n);
-    memset(p + n_old, 0, n * sizeof(*p));
     for (i = 0; i < n + n_old; i++) {
         if (i >= n_old) {
-            Object *obj;
-
-            object_initialize(p, sizeof(*p), TYPE_IRQ);
-            p->handler = handler;
-            p->opaque = opaque;
-            p->n = i;
-            obj = OBJECT(p);
-            /* Let the first IRQ clean them all up */
-            if (unlikely(i == 0)) {
-                obj->free = g_free;
-            } else {
-                object_ref(OBJECT(s[0]));
-                obj->free = irq_nonfirst_free;
-            }
+            s[i] = qemu_allocate_irq(handler, opaque, i);
         }
-        s[i] = p;
-        p++;
     }
     return s;

Quoted 4 lines

But with this change these objects are now usable with links and canon paths etc.
Will roll into V2 of this patch.

Negative!

I was well aware of the two g_renew()s, one of which affects the objects. However I figured, as long as no one has a pointer to those objects it should just continue work (otherwise the pre-QOM pointers would've broken, too -> separate issue) - and so far I haven't found a test case that doesn't work as is.

So while I agree that we should refactor this, your series does iirc not include my genuine qemu_irq* fixes either, and I reported at least two callers of qemu_free_irqs() remaining, in serial-pci.c among others, so your diff above is not yet okay - it would leak any non-first IRQState. Which is why I do these odd object_{ref,unref}() tricks - qemu_free_irqs() cannot tell how many IRQs there are to be freed. And while your use of qemu_allocate_irq() gives them the normal oc->free = g_free for freeing them individually, you do not unref them anywhere, so it will never happen.

Instead of squashing this into my patch I suggest you build upon master plus my first two cleanup patches and get rid of the remaining qemu_free_irqs()s altogether, then we can much more sanely refactor this code and get it in shortly.

Cheers,
Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Crosthwaite

author

2014-05-19 10:22 UTC #15 lore raw

Re: [Qemu-devel] [RFC v1 11/25] irq: Slim conversion of qemu_irq to QOM [WIP]

Quoted 106 lines · Andreas Färber

Am 19.05.2014 03:52, schrieb Peter Crosthwaite:
Quoted 8 lines
Quoted 6 lines · Peter Crosthwaite
From: Andreas Färber <[email protected]>
As a prequel to any big Pin refactoring plans, do an in-place conversion of qemu_irq to an Object, so that we can reference it in link<> properties.
Signed-off-by Andreas Färber <[email protected]>
(Note that you forgot to sign off here.)
Quoted 94 lines
Quoted 40 lines
---
 hw/core/irq.c    | 44 +++++++++++++++++++++++++++++++++++++++++---
 include/hw/irq.h |  2 ++
 2 files changed, 43 insertions(+), 3 deletions(-)
hw/core/irq.c +13 -0
index 03c8cb3..0bcd27b 100644
@@ -23,8 +23,13 @@
  */
 #include "qemu-common.h"
 #include "hw/irq.h"
+#include "qom/object.h"
+
+#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
 
 struct IRQState {
+    Object parent_obj;
+
     qemu_irq_handler handler;
     void *opaque;
     int n;
@@ -38,6 +43,14 @@ void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
+static void irq_nonfirst_free(void *obj)
+{
+    struct IRQState *s = obj;
+
+    /* Unreference the first IRQ in this array */
+    object_unref(OBJECT(s - s->n));
+}
+
 qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
                            void *opaque, int n)
 {
hw/core/irq.c qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler, · line 66
Quoted 3 lines
hw/core/irq.c +0 -0
@@ -51,11 +64,23 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
     p = old ? g_renew(struct IRQState, s[0], n + n_old) :
                 g_new(struct IRQState, n);
So using g_renew on the actual object storage is very fragile, as it means you cannot reliably take pointers to the objects. I think however this whole issue could be simplified by de-arrayifying the whole thing, and treating IRQs individually (interdiff):
qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
void *opaque, int n)
 {
     qemu_irq *s;
(quoted diff) +1 ~~-20~~
-    struct IRQState *p;
     int i;
 
     if (!old) {
         n_old = 0;
     }
     s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
-    p = old ? g_renew(struct IRQState, s[0], n + n_old) :
-                g_new(struct IRQState, n);
-    memset(p + n_old, 0, n * sizeof(*p));
     for (i = 0; i < n + n_old; i++) {
         if (i >= n_old) {
-            Object *obj;
-
-            object_initialize(p, sizeof(*p), TYPE_IRQ);
-            p->handler = handler;
-            p->opaque = opaque;
-            p->n = i;
-            obj = OBJECT(p);
-            /* Let the first IRQ clean them all up */
-            if (unlikely(i == 0)) {
-                obj->free = g_free;
-            } else {
-                object_ref(OBJECT(s[0]));
-                obj->free = irq_nonfirst_free;
-            }
+            s[i] = qemu_allocate_irq(handler, opaque, i);
         }
-        s[i] = p;
-        p++;
     }
     return s;
The system for freeing may need some thought, and I wonder if their are API clients out there assuming the IRQ storage elements are contiguous (if there are they have too much internal knowledge and need to be fixed).
But with this change these objects are now usable with links and canon paths etc.

Quoted 7 lines

Quoted 1 line
Will roll into V2 of this patch.
Negative!
I was well aware of the two g_renew()s, one of which affects the objects. However I figured, as long as no one has a pointer to those objects it should just continue work (otherwise the pre-QOM pointers

Well I introduce the issue 13-15 of this series. They are dependant on pointer stability.

Quoted 7 lines

would've broken, too -> separate issue) - and so far I haven't found a test case that doesn't work as is.
So while I agree that we should refactor this, your series does iirc not include my genuine qemu_irq* fixes either, and I reported at least two callers of qemu_free_irqs() remaining, in serial-pci.c among others, so your diff above is not yet okay -

Understand. I have seen your predecessor patches in your branch. I have left out such patches from this RFC (there are others like it for other features) to avoid a monster series - this is really just for comments on the whole idea. I probably already have a TLDR problem here so i'm trying to not make it worse!

it would leak any non-first IRQState.

Quoted 2 lines

Which is why I do these odd object_{ref,unref}() tricks - qemu_free_irqs() cannot tell how many IRQs there are to be freed.

There are so few call sites, they can probably just keep track of numbers and pass the count to the freer. That or removal as you have started in your short series.

And

Quoted 4 lines

while your use of qemu_allocate_irq() gives them the normal oc->free = g_free for freeing them individually, you do not unref them anywhere, so it will never happen.

Working on it. Left that issue alone just to get something working today. By v2 I mean a v2 RFC :)

Quoted 5 lines

Instead of squashing this into my patch I suggest you build upon master plus my first two cleanup patches and get rid of the remaining qemu_free_irqs()s altogether, then we can much more sanely refactor this code and get it in shortly.

Ok. Thanks for the review.

Regards,
Peter

Quoted 7 lines

Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Crosthwaite

author

2014-05-16 01:59 UTC #16 lore raw

RFC 12/25 qdev: gpio: Don't allow name share between I and O

In reply to Peter Crosthwaite

Only allow a GPIO name to be one or the other. Inputs and outputs are functionally different and should be in different namespaces. Prepares support for the QOMification of IRQs as Links.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/qdev.c | 2 ++
 1 file changed, 2 insertions(+)

hw/core/qdev.c +2 -0

index 8efeb04..3330838 100644
@@ -338,6 +338,7 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
 {
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
+    assert(gpio_list->num_out == 0 || !name);
     gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
                                      dev, n);
     gpio_list->num_in += n;

@@ -353,6 +354,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
 {
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
+    assert(gpio_list->num_in == 0 || !name);
     assert(gpio_list->num_out == 0);
     gpio_list->num_out = n;
     gpio_list->out = pins;

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 01:59 UTC #17 lore raw

RFC 13/25 qdev: gpio: Register GPIO inputs as child objects

In reply to Peter Crosthwaite

To the device that contains them. This will allow for referencing a GPIO input from its canonical path (exciting for dynamic machine generation!)

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/qdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

hw/core/qdev.c +10 -0

index 3330838..cd273e8 100644
@@ -336,11 +336,21 @@ static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
 void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
                              const char *name, int n)
 {
+    int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
     assert(gpio_list->num_out == 0 || !name);
     gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
                                      dev, n);
+
+    for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) {
+        char *propname = g_strdup_printf("%s-%d",
+                                         name ? name : "unnamed-gpio-in", i);
+        object_property_add_child(OBJECT(dev), propname,
+                                  OBJECT(gpio_list->in[i]), &error_abort);
+        g_free(propname);
+    }
+
     gpio_list->num_in += n;
 }

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:00 UTC #18 lore raw

RFC 14/25 qdev: gpio: Register GPIO outputs as QOM links

In reply to Peter Crosthwaite

Within the object that contains the GPIO output. This allows for connecting GPIO outputs via setting of a Link property.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/qdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

hw/core/qdev.c +12 -0

index cd273e8..7b1430b 100644
@@ -362,12 +362,24 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
                               const char *name, int n)
 {
+    int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
     assert(gpio_list->num_in == 0 || !name);
     assert(gpio_list->num_out == 0);
     gpio_list->num_out = n;
     gpio_list->out = pins;
+
+    for (i = 0; i < n; ++i) {
+        char *propname = g_strdup_printf("%s-%d",
+                                         name ? name : "unnamed-gpio-out", i);
+        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
+                                 (Object **)&pins[i],
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+        g_free(propname);
+    }
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:00 UTC #19 lore raw

RFC 15/25 qdev: gpio: Re-impement qdev_connect_gpio QOM style

In reply to Peter Crosthwaite

Re-implement as a link setter. This should allow the QOM framework to keep track of ref counts properly etc.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/qdev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

hw/core/qdev.c +4 -3

index 7b1430b..94b4e0e 100644
@@ -403,10 +403,11 @@ qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
 void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
                                  qemu_irq pin)
 {
-    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+    char *propname = g_strdup_printf("%s-%d",
+                                     name ? name : "unnamed-gpio-out", n);
 
-    assert(n >= 0 && n < gpio_list->num_out);
-    gpio_list->out[n] = pin;
+    object_property_set_link(OBJECT(dev), OBJECT(pin), propname, &error_abort);
+    g_free(propname);
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:01 UTC #20 lore raw

RFC 16/25 qom: object_property_set/get: Add child recursion

In reply to Peter Crosthwaite

Allow giving a "/" delimiter to indicate that a property set/get is accessing a property within a child. E.G. "foo/bar" will set the "bar" property on child "foo". Multiple "/"s will traverse multiple child levels.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 qom/object.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

qom/object.c +25 -2

index e42b254..a23ccb1 100644
@@ -772,6 +772,29 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
     return NULL;
 }
 
+static ObjectProperty *object_property_find_traverse(Object **obj,
+                                                     const char **name,
+                                                     Error **errp)
+{
+    const char *slash = strchr(*name, '/');
+
+    if (slash) {
+        Error *err = NULL;
+        char *child_name = g_strndup(*name, slash - *name);
+        *obj = object_property_get_link(*obj, child_name, &err);
+        g_free(child_name);
+        if (err) {
+            error_propagate(errp, err);
+        } else {
+            *name = slash + 1;
+            return object_property_find_traverse(obj, name, errp);
+        }
+        return NULL;
+    }
+
+    return object_property_find(*obj, *name, errp);
+}
+
 void object_property_del(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name, errp);

@@ -793,7 +816,7 @@ void object_property_del(Object *obj, const char *name, Error **errp)
 void object_property_get(Object *obj, Visitor *v, const char *name,
                          Error **errp)
 {
-    ObjectProperty *prop = object_property_find(obj, name, errp);
+    ObjectProperty *prop = object_property_find_traverse(&obj, &name, errp);
     if (prop == NULL) {
         return;
     }

@@ -808,7 +831,7 @@ void object_property_get(Object *obj, Visitor *v, const char *name,
 void object_property_set(Object *obj, Visitor *v, const char *name,
                          Error **errp)
 {
-    ObjectProperty *prop = object_property_find(obj, name, errp);
+    ObjectProperty *prop = object_property_find_traverse(&obj, &name, errp);
     if (prop == NULL) {
         return;
     }

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:02 UTC #21 lore raw

RFC 17/25 sysbus: Use TYPE_DEVICE GPIO functionality

In reply to Peter Crosthwaite

Re-implement the Sysbus GPIOs to use the existing TYPE_DEVICE GPIO named framework. A constant string name is chosen to avoid conflicts with existing unnamed GPIOs.

This unifies GPIOs are IRQs for sysbus devices and allows removal of all Sysbus state for GPIOs.

Any existing and future-added functionality for GPIOs is now also available for sysbus IRQs.

For the anti-sysbus campaigners, this patch brings us one step closer to deleting the abstraction completely.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/sysbus.c    | 20 +++-----------------
 include/hw/sysbus.h |  7 +++----
 2 files changed, 6 insertions(+), 21 deletions(-)

hw/core/sysbus.c +3 ~~-17~~

index f4e760d..2fae2bd 100644
@@ -41,11 +41,7 @@ static const TypeInfo system_bus_info = {
 
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 {
-    assert(n >= 0 && n < dev->num_irq);
-    dev->irqs[n] = NULL;
-    if (dev->irqp[n]) {
-        *dev->irqp[n] = irq;
-    }
+    qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq);
 }
 
 static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,

@@ -89,22 +85,13 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
 /* Request an IRQ source.  The actual IRQ object may be populated later.  */
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
 {
-    int n;
-
-    assert(dev->num_irq < QDEV_MAX_IRQ);
-    n = dev->num_irq++;
-    dev->irqp[n] = p;
+    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1);
 }
 
 /* Pass IRQs from a target device.  */
 void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
 {
-    int i;
-    assert(dev->num_irq == 0);
-    dev->num_irq = target->num_irq;
-    for (i = 0; i < dev->num_irq; i++) {
-        dev->irqp[i] = target->irqp[i];
-    }
+    /* FIXME */
 }
 
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)

@@ -210,7 +197,6 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
     hwaddr size;
     int i;
 
-    monitor_printf(mon, "%*sirq %d\n", indent, "", s->num_irq);
     for (i = 0; i < s->num_mmio; i++) {
         size = memory_region_size(s->mmio[i].memory);
         monitor_printf(mon, "%*smmio " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",

include/hw/sysbus.h +3 -4

index f5aaa05..7ee4a04 100644
@@ -8,7 +8,6 @@
 
 #define QDEV_MAX_MMIO 32
 #define QDEV_MAX_PIO 32
-#define QDEV_MAX_IRQ 512
 
 #define TYPE_SYSTEM_BUS "System"
 #define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)

@@ -33,6 +32,9 @@ typedef struct SysBusDevice SysBusDevice;
  * SysBusDeviceClass is not overriding #DeviceClass.realize, so derived
  * classes overriding it are not required to invoke its implementation.
  */
+
+#define SYSBUS_DEVICE_GPIO_IRQ "sysbus-irq"
+
 typedef struct SysBusDeviceClass {
     /*< private >*/
     DeviceClass parent_class;

@@ -46,9 +48,6 @@ struct SysBusDevice {
     DeviceState parent_obj;
     /*< public >*/
 
-    int num_irq;
-    qemu_irq irqs[QDEV_MAX_IRQ];
-    qemu_irq *irqp[QDEV_MAX_IRQ];
     int num_mmio;
     struct {
         hwaddr addr;

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-19 01:59 UTC #22 lore raw

Re: [Qemu-devel] [RFC v1 17/25] sysbus: Use TYPE_DEVICE GPIO functionality

Quoted 38 lines · Peter Crosthwaite

Re-implement the Sysbus GPIOs to use the existing TYPE_DEVICE GPIO named framework. A constant string name is chosen to avoid conflicts with existing unnamed GPIOs.
This unifies GPIOs are IRQs for sysbus devices and allows removal of all Sysbus state for GPIOs.
Any existing and future-added functionality for GPIOs is now also available for sysbus IRQs.
For the anti-sysbus campaigners, this patch brings us one step closer to deleting the abstraction completely.
Signed-off-by Peter Crosthwaite <[email protected]>
---
 hw/core/sysbus.c    | 20 +++-----------------
 include/hw/sysbus.h |  7 +++----
 2 files changed, 6 insertions(+), 21 deletions(-)
hw/core/sysbus.c +1 -5
index f4e760d..2fae2bd 100644
@@ -41,11 +41,7 @@ static const TypeInfo system_bus_info = {
 
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 {
-    assert(n >= 0 && n < dev->num_irq);
-    dev->irqs[n] = NULL;
-    if (dev->irqp[n]) {
-        *dev->irqp[n] = irq;
-    }
+    qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq);
 }
 
 static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,

hw/core/sysbus.c void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr, · line 88

Quoted 9 lines

hw/core/sysbus.c +1 -5

@@ -89,22 +85,13 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
 /* Request an IRQ source.  The actual IRQ object may be populated later.  */
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
 {
-    int n;
-
-    assert(dev->num_irq < QDEV_MAX_IRQ);
-    n = dev->num_irq++;
-    dev->irqp[n] = p;
+    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1);

This is not repeatable as qemu_init_gpio_out() doesnt allow repeated calls for the same GPIO output set (doh!) - that feature only exists for GPIO inputs. Rather than investing further into unnamed GPIOs on the DEVICE level though, I think it's better to just name each sysbus IRQ individually. If you really really want arrayified IRQs then you can still use the DEVICE GPIO API directly. To fix in V2:

/* Request an IRQ source. The actual IRQ object may be populated later. */ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
{
- qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1);
+ char *irq_name = g_strdup_printf(SYSBUS_DEVICE_GPIO_IRQ "-%d",
+ dev->num_irq++);
+ qdev_init_gpio_out_named(DEVICE(dev), p, irq_name, 1);
+ g_free(irq_name);
}

Regards,
Peter

Peter Crosthwaite

author

2014-05-16 02:02 UTC #23 lore raw

RFC 18/25 sysbus: Rework sysbus_mmio_map to use mr QOMification

In reply to Peter Crosthwaite

The QOMified MemoryRegion properties "parent", "may-overlap", "priority" and "addr" now handle all these semantics. Just set them unconditionally. The Memory API also preserves the fast path API behaviour implemented where if nothing changes don't bounce the subregion. So all conditional logic can be removed. This also allow for removal of the mmio.addr field within SysBus. Another annoying and redundant piece of SysBus state bites the dust!

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/sysbus.c    | 39 +++++++++++++++------------------------
 include/hw/sysbus.h |  1 -
 2 files changed, 15 insertions(+), 25 deletions(-)

hw/core/sysbus.c +15 ~~-24~~

index 2fae2bd..7cdc428 100644
@@ -47,28 +47,17 @@ void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
                                    bool may_overlap, int priority)
 {
+    MemoryRegion *mr;
     assert(n >= 0 && n < dev->num_mmio);
 
-    if (dev->mmio[n].addr == addr) {
-        /* ??? region already mapped here.  */
-        return;
-    }
-    if (dev->mmio[n].addr != (hwaddr)-1) {
-        /* Unregister previous mapping.  */
-        memory_region_del_subregion(get_system_memory(), dev->mmio[n].memory);
-    }
-    dev->mmio[n].addr = addr;
-    if (may_overlap) {
-        memory_region_add_subregion_overlap(get_system_memory(),
-                                            addr,
-                                            dev->mmio[n].memory,
-                                            priority);
-    }
-    else {
-        memory_region_add_subregion(get_system_memory(),
-                                    addr,
-                                    dev->mmio[n].memory);
-    }
+    mr = dev->mmio[n].memory;
+
+    object_property_set_link(OBJECT(mr), OBJECT(get_system_memory()),
+                             "container", &error_abort);
+    object_property_set_bool(OBJECT(mr), may_overlap, "may-overlap",
+                             &error_abort);
+    object_property_set_int(OBJECT(mr), priority, "priority", &error_abort);
+    object_property_set_int(OBJECT(mr), addr, "addr", &error_abort);
 }
 
 void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr)

@@ -100,7 +89,6 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
 
     assert(dev->num_mmio < QDEV_MAX_MMIO);
     n = dev->num_mmio++;
-    dev->mmio[n].addr = -1;
     dev->mmio[n].memory = memory;
 }

@@ -198,9 +186,11 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
     int i;
 
     for (i = 0; i < s->num_mmio; i++) {
+        hwaddr addr = object_property_get_int(OBJECT(s->mmio[i].memory),
+                                              "addr", &error_abort);
         size = memory_region_size(s->mmio[i].memory);
         monitor_printf(mon, "%*smmio " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",
-                       indent, "", s->mmio[i].addr, size);
+                       indent, "", addr, size);
     }
 }

@@ -213,8 +203,9 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
     off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
 
     if (s->num_mmio) {
-        snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx,
-                 s->mmio[0].addr);
+        hwaddr addr = object_property_get_int(OBJECT(s->mmio[0].memory),
+                                              "addr", &error_abort);
+        snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx, addr);
     } else if (s->num_pio) {
         snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
     }

include/hw/sysbus.h +0 -1

index 7ee4a04..4499020 100644
@@ -50,7 +50,6 @@ struct SysBusDevice {
 
     int num_mmio;
     struct {
-        hwaddr addr;
         MemoryRegion *memory;
     } mmio[QDEV_MAX_MMIO];
     int num_pio;

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:03 UTC #24 lore raw

RFC 19/25 sysbus: Setup memory regions as dynamic props

In reply to Peter Crosthwaite

Dynamically allocate Memory Region pointers and set them up as QOM links. Plug these dynamic links to the sysbus_init_mmio() and sysbus_mmio_get_region APIs.

This allows for removal of the Sysbus Memory Regions as state. All that's needed now is the counter for total number of regions. Another piece of SysBus state bites the dust!

This also removes the artificial limit of 32 memory regions per device. The number of memory regions is now practically unbounded (unless you cause a wrap on the total counter).

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/sysbus.c    | 29 ++++++++++++++++++-----------
 include/hw/sysbus.h |  4 ----
 2 files changed, 18 insertions(+), 15 deletions(-)

hw/core/sysbus.c +18 ~~-11~~

index 7cdc428..6858336 100644
@@ -50,7 +50,7 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
     MemoryRegion *mr;
     assert(n >= 0 && n < dev->num_mmio);
 
-    mr = dev->mmio[n].memory;
+    mr = sysbus_mmio_get_region(dev, n);
 
     object_property_set_link(OBJECT(mr), OBJECT(get_system_memory()),
                              "container", &error_abort);

@@ -85,16 +85,22 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
 
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
 {
-    int n;
+    char *propname = g_strdup_printf("sysbus-mr-%d", dev->num_mmio++);
 
-    assert(dev->num_mmio < QDEV_MAX_MMIO);
-    n = dev->num_mmio++;
-    dev->mmio[n].memory = memory;
+    object_property_add_child(OBJECT(dev), propname, OBJECT(memory),
+                              &error_abort);
+    g_free(propname);
 }
 
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
 {
-    return dev->mmio[n].memory;
+    MemoryRegion *ret;
+    char *propname = g_strdup_printf("sysbus-mr-%d", n);
+
+    ret = MEMORY_REGION(object_property_get_link(OBJECT(dev), propname,
+                        &error_abort));
+    g_free(propname);
+    return ret;
 }
 
 void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size)

@@ -186,9 +192,9 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
     int i;
 
     for (i = 0; i < s->num_mmio; i++) {
-        hwaddr addr = object_property_get_int(OBJECT(s->mmio[i].memory),
-                                              "addr", &error_abort);
-        size = memory_region_size(s->mmio[i].memory);
+        MemoryRegion *mr = sysbus_mmio_get_region(s, i);
+        hwaddr addr = object_property_get_int(OBJECT(mr), "addr", &error_abort);
+        size = memory_region_size(mr);
         monitor_printf(mon, "%*smmio " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",
                        indent, "", addr, size);
     }

@@ -203,8 +209,9 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
     off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
 
     if (s->num_mmio) {
-        hwaddr addr = object_property_get_int(OBJECT(s->mmio[0].memory),
-                                              "addr", &error_abort);
+        hwaddr addr;
+        addr = object_property_get_int(OBJECT(sysbus_mmio_get_region(s, 0)),
+                                       "addr", &error_abort);
         snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx, addr);
     } else if (s->num_pio) {
         snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);

include/hw/sysbus.h +0 -4

index 4499020..1a8e527 100644
@@ -6,7 +6,6 @@
 #include "hw/qdev.h"
 #include "exec/memory.h"
 
-#define QDEV_MAX_MMIO 32
 #define QDEV_MAX_PIO 32
 
 #define TYPE_SYSTEM_BUS "System"

@@ -49,9 +48,6 @@ struct SysBusDevice {
     /*< public >*/
 
     int num_mmio;
-    struct {
-        MemoryRegion *memory;
-    } mmio[QDEV_MAX_MMIO];
     int num_pio;
     pio_addr_t pio[QDEV_MAX_PIO];
 };

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:03 UTC #25 lore raw

RFC 20/25 sysbus: Enable hotplug.

In reply to Peter Crosthwaite

If your willing to set a few props, it can work. All the connections this comment is referring to are QOMified now, so sysbus hotplug is a full reality.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/core/sysbus.c | 7 -------
 1 file changed, 7 deletions(-)

hw/core/sysbus.c +0 -7

index 6858336..a56a74d 100644
@@ -241,13 +241,6 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = sysbus_device_init;
     k->bus_type = TYPE_SYSTEM_BUS;
-    /*
-     * device_add plugs devices into suitable bus.  For "real" buses,
-     * that actually connects the device.  For sysbus, the connections
-     * need to be made separately, and device_add can't do that.  The
-     * device would be left unconnected, and could not possibly work.
-     */
-    k->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sysbus_device_type_info = {

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:04 UTC #26 lore raw

RFC 21/25 microblaze: s3adsp: Expand UART creator

In reply to Peter Crosthwaite

To not use sysbus_create_simple. This prepares for setting the device parent.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/microblaze/petalogix_s3adsp1800_mmu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

hw/microblaze/petalogix_s3adsp1800_mmu.c +4 -2

index 6c45e20..9bc6928 100644
@@ -107,8 +107,10 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
         irq[i] = qdev_get_gpio_in(dev, i);
     }
 
-    sysbus_create_simple("xlnx.xps-uartlite", UARTLITE_BASEADDR,
-                         irq[UARTLITE_IRQ]);
+    dev = qdev_create(NULL, "xlnx.xps-uartlite");
+    qdev_init_nofail(dev);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[UARTLITE_IRQ]);
 
     /* 2 timers at irq 2 @ 62 Mhz.  */
     dev = qdev_create(NULL, "xlnx.xps-timer");

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:05 UTC #27 lore raw

RFC 22/25 microblaze: s3adsp: Parent devices with sane names

In reply to Peter Crosthwaite

For ease of reference by users of their canonical paths.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/microblaze/petalogix_s3adsp1800_mmu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

hw/microblaze/petalogix_s3adsp1800_mmu.c +8 -0

index 9bc6928..8557254 100644
@@ -97,6 +97,8 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
                           1, 0x89, 0x18, 0x0000, 0x0, 1);
 
     dev = qdev_create(NULL, "xlnx.xps-intc");
+    object_property_add_child(qdev_get_machine(), "intc", OBJECT(dev),
+                              &error_abort);
     qdev_prop_set_uint32(dev, "kind-of-intr",
                          1 << ETHLITE_IRQ | 1 << UARTLITE_IRQ);
     qdev_init_nofail(dev);

@@ -108,12 +110,16 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
     }
 
     dev = qdev_create(NULL, "xlnx.xps-uartlite");
+    object_property_add_child(qdev_get_machine(), "uart", OBJECT(dev),
+                              &error_abort);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[UARTLITE_IRQ]);
 
     /* 2 timers at irq 2 @ 62 Mhz.  */
     dev = qdev_create(NULL, "xlnx.xps-timer");
+    object_property_add_child(qdev_get_machine(), "timer", OBJECT(dev),
+                              &error_abort);
     qdev_prop_set_uint32(dev, "one-timer-only", 0);
     qdev_prop_set_uint32(dev, "clock-frequency", 62 * 1000000);
     qdev_init_nofail(dev);

@@ -122,6 +128,8 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
 
     qemu_check_nic_model(&nd_table[0], "xlnx.xps-ethernetlite");
     dev = qdev_create(NULL, "xlnx.xps-ethernetlite");
+    object_property_add_child(qdev_get_machine(), "enet", OBJECT(dev),
+                              &error_abort);
     qdev_set_nic_properties(dev, &nd_table[0]);
     qdev_prop_set_uint32(dev, "tx-ping-pong", 0);
     qdev_prop_set_uint32(dev, "rx-ping-pong", 0);

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:05 UTC #28 lore raw

RFC 23/25 timer: xilinx_timer: Convert to realize()

In reply to Peter Crosthwaite

SysBusDevice::init is depracated. Convert to Object::init and Device::realize as prescribed by QOM conventions.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/timer/xilinx_timer.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

hw/timer/xilinx_timer.c +12 -8

index 3ff1da9..cdb3355 100644
@@ -204,14 +204,11 @@ static void timer_hit(void *opaque)
     timer_update_irq(t);
 }
 
-static int xilinx_timer_init(SysBusDevice *dev)
+static void xilinx_timer_realize(DeviceState *dev, Error **errp)
 {
     struct timerblock *t = XILINX_TIMER(dev);
     unsigned int i;
 
-    /* All timers share a single irq line.  */
-    sysbus_init_irq(dev, &t->irq);
-
     /* Init all the ptimers.  */
     t->timers = g_malloc0(sizeof t->timers[0] * num_timers(t));
     for (i = 0; i < num_timers(t); i++) {

@@ -226,8 +223,15 @@ static int xilinx_timer_init(SysBusDevice *dev)
 
     memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",
                           R_MAX * 4 * num_timers(t));
-    sysbus_init_mmio(dev, &t->mmio);
-    return 0;
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &t->mmio);
+}
+
+static void xilinx_timer_init(Object *obj)
+{
+    struct timerblock *t = XILINX_TIMER(obj);
+
+    /* All timers share a single irq line.  */
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &t->irq);
 }
 
 static Property xilinx_timer_properties[] = {

@@ -240,9 +244,8 @@ static Property xilinx_timer_properties[] = {
 static void xilinx_timer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = xilinx_timer_init;
+    dc->realize = xilinx_timer_realize;
     dc->props = xilinx_timer_properties;
 }

@@ -250,6 +253,7 @@ static const TypeInfo xilinx_timer_info = {
     .name          = TYPE_XILINX_TIMER,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(struct timerblock),
+    .instance_init = xilinx_timer_init,
     .class_init    = xilinx_timer_class_init,
 };

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:06 UTC #29 lore raw

RFC 24/25 timer: xilinx_timer: init MMIO ASAP

In reply to Peter Crosthwaite

So the memory regions exist pre-realize.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/timer/xilinx_timer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

hw/timer/xilinx_timer.c +5 -3

index cdb3355..1b0e2a1 100644
@@ -221,9 +221,8 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
         ptimer_set_freq(xt->ptimer, t->freq_hz);
     }
 
-    memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",
-                          R_MAX * 4 * num_timers(t));
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &t->mmio);
+    object_property_set_int(OBJECT(&t->mmio), R_MAX * 4 * num_timers(t),
+                            "size", &error_abort);
 }
 
 static void xilinx_timer_init(Object *obj)

@@ -232,6 +231,9 @@ static void xilinx_timer_init(Object *obj)
 
     /* All timers share a single irq line.  */
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &t->irq);
+    memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t,
+                          "xlnx.xps-timer", 0);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &t->mmio);
 }
 
 static Property xilinx_timer_properties[] = {

Signature · 1 line

1.9.3.1.ga73a6ad

Peter Crosthwaite

author

2014-05-16 02:06 UTC #30 lore raw

RFC 25/25 TEST: microblaze: s3adsp: Remove timer

In reply to Peter Crosthwaite

To make it easy to test sysbus hotplug with the published Microblaze test image. The timer can be added using device_add etc.

On your command line:

-device xlnx.xps-timer,\
sysbus-mr-0.container=/machine/sysmem/root-mr,\
sysbus-mr-0.addr=0x83c00000,\
sysbus-irq-0=/machine/intc/unnamed-gpio-0

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/microblaze/petalogix_s3adsp1800_mmu.c | 2 ++
 1 file changed, 2 insertions(+)

hw/microblaze/petalogix_s3adsp1800_mmu.c +2 -0

index 8557254..74d7f05 100644
@@ -116,6 +116,7 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[UARTLITE_IRQ]);
 
+#if 0
     /* 2 timers at irq 2 @ 62 Mhz.  */
     dev = qdev_create(NULL, "xlnx.xps-timer");
     object_property_add_child(qdev_get_machine(), "timer", OBJECT(dev),

@@ -125,6 +126,7 @@ petalogix_s3adsp1800_init(QEMUMachineInitArgs *args)
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, TIMER_BASEADDR);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[TIMER_IRQ]);
+#endif
 
     qemu_check_nic_model(&nd_table[0], "xlnx.xps-ethernetlite");
     dev = qdev_create(NULL, "xlnx.xps-ethernetlite");

Signature · 1 line

1.9.3.1.ga73a6ad