On 02.07.14 11:03, Peter Crosthwaite wrote:
On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <ag...@suse.de> wrote:
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.
So the way this is handled for links is its an open coded check
function added by the property adder. Check
qdev_prop_allow_set_link_before_realize() for a precedent.
Yeah, I realized that this is what you meant only after I sent the mail
:). Considering that this a deeply philosophical question and Paolo
likes the wrapper approach better, I don't think I'll really touch this
though.
Instead, I'll export all the simple integer get/set helpers to the world
and use object_property_add directly. That way I can also hook in my
release function that I need with this approach.
+ 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 ;).
I'm not following? Do you mean another level of indirection?
Same same :).
}
/* 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
Or a sysbus-specific check fn
check whether the property
is SYSBUS_DYNAMIC and only allow writes if it is? Then I can just always use
the QOM setter functions.
Yep. You can use the setters on your own object in absence of local ptr.
That still leaves the question on how the finalize function
It's not the isntance finalise that would do the free-ing, it's the
individual property finalizers.To implement you could add a
"free-the-ptr" option to object_property_add_*_ptr that installs the
relevant finalizer or you can fall back to full open coded properties
and set the property finalize callback.
-ETOOMANYOPTIONS. I'll stick with object_property_add and export a
generic g_free release helper instead :).
Alex
