qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Crosthwaite
Subject: Re: [Qemu-ppc] [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
Date: Wed, 2 Jul 2014 19:03:12 +1000

On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <address@hidden> wrote:
>
> On 02.07.14 06:12, Peter Crosthwaite wrote:
>>
>> 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.
>
>
> 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
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]