Re: [Qemu-devel] [PATCH RFC 07/15] timer/arm_mptimer: Convert to QOM realize

[Andreas Färber]

Am 01.07.2013 11:33, schrieb Peter Crosthwaite:
> Hi Andreas,
> 
> On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <address@hidden> wrote:
>> From: Andreas Färber <address@hidden>
>>
>> Split the SysBusDevice initfn into instance_init and realizefn.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  hw/timer/arm_mptimer.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
>> index 588e34b..a19ffa3 100644
>> --- a/hw/timer/arm_mptimer.c
>> +++ b/hw/timer/arm_mptimer.c
>> @@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev)
>>      }
>>  }
>>
>> -static int arm_mptimer_init(SysBusDevice *dev)
>> +static void arm_mptimer_init(Object *obj)
>>  {
>> +    ARMMPTimerState *s = ARM_MP_TIMER(obj);
>> +
>> +    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
>> +                          "arm_mptimer_timer", 0x20);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +}
> 
> Splitting off only one of the memory region setups to init seems a bit
> awkward. Is it really worth it given that we need to wait for
> realization for the rest to occur anyway?

Well, my main interest wrt MemoryRegions here is to have the
*mpcore_priv container MemoryRegion mappable before realize and to avoid
having to implement unnecessary cleanups in unrealize.

An alternative would be for PMM to use his array properties to
dynamically allocate the MemoryRegions before realize, but so far he has
failed to come up with a solution...
Another solution, since this is a fixed-length array, would be to always
initialize all MemoryRegions and just keep adding all 3(?) in realize.

FWIU adding subregions is desired in instance_init as long as it affects
only containing devices and not a global address space.

Regards,
Andreas

>> +
>> +static void arm_mptimer_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>      ARMMPTimerState *s = ARM_MP_TIMER(dev);
>>      int i;
>>
>> @@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev)
>>       *  * timer for core 1
>>       * and so on.
>>       */
>> -    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
>> -                          "arm_mptimer_timer", 0x20);
>> -    sysbus_init_mmio(dev, &s->iomem);
>>      for (i = 0; i < s->num_cpu; i++) {
>>          TimerBlock *tb = &s->timerblock[i];
>>          tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb);
>> -        sysbus_init_irq(dev, &tb->irq);
>> +        sysbus_init_irq(sbd, &tb->irq);
>>          memory_region_init_io(&tb->iomem, &timerblock_ops, tb,
>>                                "arm_mptimer_timerblock", 0x20);
>> -        sysbus_init_mmio(dev, &tb->iomem);
>> +        sysbus_init_mmio(sbd, &tb->iomem);
>>      }
>> -
>> -    return 0;
>>  }
>>
>>  static const VMStateDescription vmstate_timerblock = {
>> @@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = {
>>  static void arm_mptimer_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
>>
>> -    sbc->init = arm_mptimer_init;
>> +    dc->realize = arm_mptimer_realize;
>>      dc->vmsd = &vmstate_arm_mptimer;
>>      dc->reset = arm_mptimer_reset;
>>      dc->no_user = 1;
>> @@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = {
>>      .name          = TYPE_ARM_MP_TIMER,
>>      .parent        = TYPE_SYS_BUS_DEVICE,
>>      .instance_size = sizeof(ARMMPTimerState),
>> +    .instance_init = arm_mptimer_init,
>>      .class_init    = arm_mptimer_class_init,
>>  };
>>
>> --
>> 1.8.1.4
>>
>>


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg