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

[Alexander Graf <agraf@suse.de>]

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