On 02.07.14 06:12, Peter Crosthwaite wrote:
On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <ag...@suse.de> wrote:
We want to give the user the ability to tell our machine file where he wants
to have devices mapped to. This patch adds code to create these hints
dynamically and expose them as object properties that can only be modified
before device realization.
Signed-off-by: Alexander Graf <ag...@suse.de>
---
hw/core/sysbus.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++--
include/hw/sysbus.h | 6 +++++
2 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index f4e760d..84cd0cf 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -86,6 +86,54 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n,
hwaddr addr,
sysbus_mmio_map_common(dev, n, addr, true, priority);
}
+static void sysbus_property_set_uint32_ptr(Object *obj, Visitor *v,
+ void *opaque, const char *name,
+ Error **errp)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ if (dev->realized) {
+ qdev_prop_set_after_realize(dev, name, errp);
+ return;
+ }
+
So this suggests your reasoning for side effected _ptr write is just
for validity checking. So another approach could be to add a "check"
function to the _ptr variants (rather than an open coded) setter. This
has the advantage of being consistent with what we already do for
object_property_add_link.
Heh, yes. Unfortunately "realized" is a field in DeviceStruct which we
don't have access to from object.c.
In fact, this is exactly what I wanted to do before this approach. I
introduced an enum that was either
* read-only
* read-write
* read-write-before-realize
and wanted to do all the checking in object.c.
But then I realized that object.c really shouldn't be aware of
DeviceState and threw away the idea.
+ object_property_set_uint32_ptr(obj, v, opaque, name, errp);
+}
+
+static void sysbus_property_set_uint64_ptr(Object *obj, Visitor *v,
+ void *opaque, const char *name,
+ Error **errp)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ if (dev->realized) {
+ qdev_prop_set_after_realize(dev, name, errp);
+ return;
+ }
+
+ object_property_set_uint64_ptr(obj, v, opaque, name, errp);
+}
+
+static void sysbus_init_prop(SysBusDevice *dev, const char *propstr, int n,
sysbus_init_int_prop.
Ok.
+ void *ptr, int size)
+{
+ char *name = g_strdup_printf(propstr, n);
+ Object *obj = OBJECT(dev);
+
+ switch (size) {
+ case sizeof(uint32_t):
Is it easier to just go lowest common denom of 64-bit int props for
everything to avoid the sizeof stuffs?
Hrm, interesting idea. Let me give it a shot :).
+ object_property_add_uint32_ptr(obj, name, ptr,
+ sysbus_property_set_uint32_ptr, NULL);
+ break;
+ case sizeof(uint64_t):
+ object_property_add_uint64_ptr(obj, name, ptr,
+ sysbus_property_set_uint64_ptr, NULL);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+}
+
/* Request an IRQ source. The actual IRQ object may be populated later. */
void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
{
@@ -93,7 +141,13 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
assert(dev->num_irq < QDEV_MAX_IRQ);
n = dev->num_irq++;
+ dev->user_irqs = g_realloc(dev->user_irqs,
+ sizeof(*dev->user_irqs) * (n + 1));
Will the QOM framework take references to this allocated area before
the final realloc? I had this problem with IRQs leading to this patch
to remove uses of realloc for QOM:
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00051.html
+ dev->user_irqs[n] = (uint32_t)SYSBUS_DYNAMIC;
dev->irqp[n] = p;
+
+ sysbus_init_prop(dev, "irq[%d]", n, &dev->user_irqs[n],
+ sizeof(dev->user_irqs[n]));
You pass a ref to reallocable area here.
Perhaps a cleaner solution is to just malloc a single uint64_t here
locally and pass it off to QOM. Then free the malloc in the property
finalizer ...
Heh, you can always add another level of abstraction ;).
}
/* Pass IRQs from a target device. */
@@ -103,7 +157,7 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice
*target)
assert(dev->num_irq == 0);
dev->num_irq = target->num_irq;
sysbus_init_irq does num_irq incrementing itself so does this need to go?
Yikes - must have sneaked back in on patch reshuffling. Yes, of course.
for (i = 0; i < dev->num_irq; i++) {
- dev->irqp[i] = target->irqp[i];
+ sysbus_init_irq(dev, target->irqp[i]);
}
}
@@ -113,8 +167,14 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion
*memory)
assert(dev->num_mmio < QDEV_MAX_MMIO);
n = dev->num_mmio++;
+ dev->user_mmios = g_realloc(dev->user_mmios,
+ sizeof(*dev->user_mmios) * (n + 1));
+ dev->user_mmios[n] = SYSBUS_DYNAMIC;
dev->mmio[n].addr = -1;
dev->mmio[n].memory = memory;
+
+ sysbus_init_prop(dev, "mmio[%d]", n, &dev->user_mmios[n],
+ sizeof(dev->user_mmios[n]));
You might be able to drop the %d once Paolo wildcard array property
adder stuff gets through.
}
MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
@@ -127,8 +187,17 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t
ioport, pio_addr_t size)
pio_addr_t i;
for (i = 0; i < size; i++) {
+ int n;
+
assert(dev->num_pio < QDEV_MAX_PIO);
- dev->pio[dev->num_pio++] = ioport++;
+ n = dev->num_pio++;
+ dev->user_pios = g_realloc(dev->user_pios,
+ sizeof(*dev->user_pios) * (n + 1));
+ dev->user_pios[n] = (uint32_t)SYSBUS_DYNAMIC;
+ dev->pio[n] = ioport++;
+
+ sysbus_init_prop(dev, "pio[%d]", n, &dev->user_pios[n],
+ sizeof(dev->user_pios[n]));
}
}
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index f5aaa05..870e7cc 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -9,6 +9,7 @@
#define QDEV_MAX_MMIO 32
#define QDEV_MAX_PIO 32
#define QDEV_MAX_IRQ 512
+#define SYSBUS_DYNAMIC -1ULL
#define TYPE_SYSTEM_BUS "System"
#define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
@@ -56,6 +57,11 @@ struct SysBusDevice {
} mmio[QDEV_MAX_MMIO];
int num_pio;
pio_addr_t pio[QDEV_MAX_PIO];
+
+ /* These may be set by the user as hints where to map the device */
+ uint64_t *user_mmios;
+ uint32_t *user_irqs;
+ uint32_t *user_pios;
With the single malloc/free-on-finalise approach to the properties
there's no longer a need for this new state at all.
I'll need to keep a reference to the pointers in here so that I can
still write to them one last time after realize from the machine file to
make sure I convert "dynamic" properties to their respective numbers.
Or do you think it'd be better to make the setter check whether the
property is SYSBUS_DYNAMIC and only allow writes if it is? Then I can
just always use the QOM setter functions.
That still leaves the question on how the finalize function would know
which variables to free when I don't have any reference at all in my
state :).
Alex
