From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2GWL-0007uh-1S for qemu-devel@nongnu.org; Wed, 02 Jul 2014 05:08:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2GWF-0007xs-Fo for qemu-devel@nongnu.org; Wed, 02 Jul 2014 05:08:04 -0400 Message-ID: <53B3CBEC.1030701@suse.de> Date: Wed, 02 Jul 2014 11:07:56 +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> <53B3C1A8.7020408@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 11:03, Peter Crosthwaite wrote: > On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf wrote: >> 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. >> > 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