Re: [Qemu-ppc] [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints

[Peter Crosthwaite]

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.

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