|
| From: | Alexander Graf |
| Subject: | Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints |
| Date: | Wed, 02 Jul 2014 11:07:56 +0200 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 |
On 02.07.14 11:03, Peter Crosthwaite wrote:
On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <address@hidden> wrote:On 02.07.14 06:12, Peter Crosthwaite wrote:On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <address@hidden> 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 <address@hidden> --- 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 setterOr a sysbus-specific check fncheck 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 functionIt'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
| [Prev in Thread] | Current Thread | [Next in Thread] |