From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Fpw-0005TT-TR for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:24:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2Fpq-000165-Vd for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:24:16 -0400 Message-ID: <53B3C1A8.7020408@suse.de> Date: Wed, 02 Jul 2014 10:24:08 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1404251378-5242-1-git-send-email-agraf@suse.de> <1404251378-5242-4-git-send-email-agraf@suse.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , Eric Auger , "qemu-devel@nongnu.org Developers" , qemu-ppc Mailing List , "Stalley, Sean" , Paolo Bonzini , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= On 02.07.14 06:12, Peter Crosthwaite wrote: > On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf 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 >> --- >> 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