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

[Alexander Graf <agraf@suse.de>]

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