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.
> + 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
>
>
