| Message ID | 1404251378-5242-4-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@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 <agraf@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. > + 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. > + 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? > + 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 ... > } > > /* 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? > 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. Regards, Peter > }; > > void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory); > -- > 1.8.1.4 > >
On 02.07.14 06:12, Peter Crosthwaite wrote: > On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@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 <agraf@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
Il 02/07/2014 10:24, Alexander Graf ha scritto: >>> >> 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. I think your solution is pretty. However, it's probably time to split qom/object.c and/or include/qom/object.h. Paolo
On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <agraf@suse.de> wrote: > > On 02.07.14 06:12, Peter Crosthwaite wrote: >> >> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@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 <agraf@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. > >> >>> + 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? > >> >>> } >>> >>> /* 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. would know which > variables to free when I don't have any reference at all in my state :). > Regards, Peter > > Alex > >
On 02.07.14 11:03, Peter Crosthwaite wrote: > On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <agraf@suse.de> wrote: >> On 02.07.14 06:12, Peter Crosthwaite wrote: >>> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@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 <agraf@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
Il 02/07/2014 11:07, Alexander Graf ha scritto: >> 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. However, unlike Alex's case the link setter is complicated and a simple tail call won't do. It first computes the "val" argument that is passed to the check function, then calls the check function, then does the actual set. Memory hotplug is using "val", so we cannot simply change the check function's signature in such a way that we would use a tail call. > 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. Good idea. But please make a new header file. Paolo
On 02.07.14 11:17, Paolo Bonzini wrote: > Il 02/07/2014 11:07, Alexander Graf ha scritto: >>> 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. > > However, unlike Alex's case the link setter is complicated and a > simple tail call won't do. It first computes the "val" argument that > is passed to the check function, then calls the check function, then > does the actual set. > > Memory hotplug is using "val", so we cannot simply change the check > function's signature in such a way that we would use a tail call. > >> 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. > > Good idea. But please make a new header file. New header file, but same .c file? Why? Alex
Il 02/07/2014 11:19, Alexander Graf ha scritto: > > On 02.07.14 11:17, Paolo Bonzini wrote: >> Il 02/07/2014 11:07, Alexander Graf ha scritto: >>>> 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. >> >> However, unlike Alex's case the link setter is complicated and a >> simple tail call won't do. It first computes the "val" argument that >> is passed to the check function, then calls the check function, then >> does the actual set. >> >> Memory hotplug is using "val", so we cannot simply change the check >> function's signature in such a way that we would use a tail call. >> >>> 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. >> >> Good idea. But please make a new header file. > > New header file, but same .c file? Why? I would all but complain about different .c file. :) Paolo
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; + } + + 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, + void *ptr, int size) +{ + char *name = g_strdup_printf(propstr, n); + Object *obj = OBJECT(dev); + + switch (size) { + case sizeof(uint32_t): + 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)); + 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])); } /* 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; 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])); } 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; }; void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
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 <agraf@suse.de> --- hw/core/sysbus.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++-- include/hw/sysbus.h | 6 +++++ 2 files changed, 77 insertions(+), 2 deletions(-)