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