Re: [Qemu-devel] [PATCH v3 11/24] cpu/a15mpcore: Embed GICState

["Andreas Färber" <afaerber@suse.de>]

Am 21.08.2013 23:05, schrieb Peter Maydell:
> On 20 August 2013 16:21, Andreas Färber <afaerber@suse.de> wrote:
>> From: Andreas Färber <andreas.faerber@web.de>
>>
>> This covers both emulated and KVM GIC.
> 
>> @@ -35,40 +36,48 @@ typedef struct A15MPPrivState {
>>      uint32_t num_cpu;
>>      uint32_t num_irq;
>>      MemoryRegion container;
>> -    DeviceState *gic;
>> +
>> +    GICState gic;
>>  } A15MPPrivState;
> 
>>  static void a15mp_priv_initfn(Object *obj)
>>  {
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>      A15MPPrivState *s = A15MPCORE_PRIV(obj);
>> +    DeviceState *gicdev;
>> +    const char *gictype = "arm_gic";
>> +
>> +    if (kvm_irqchip_in_kernel()) {
>> +        gictype = "kvm-arm-gic";
>> +    }
>>
>>      memory_region_init(&s->container, obj, "a15mp-priv-container", 0x8000);
>>      sysbus_init_mmio(sbd, &s->container);
>> +
>> +    object_initialize(&s->gic, gictype);
>> +    gicdev = DEVICE(&s->gic);
>> +    qdev_set_parent_bus(gicdev, sysbus_get_default());
>> +    qdev_prop_set_uint32(gicdev, "revision", 2);
> 
> So this is basically assuming that kvm-arm-gic and arm-gic
> both have an instance struct of exactly the same size,
> even though they're different classes (they happen to be
> so at the moment, because neither adds extra state beyond
> that needed by common base class). Is that really a good
> idea? (If it ever becomes not true we get silent memory
> corruption here...)

Not sure if a union of only one member is permitted? We're not actually
accessing the GICState, only void* and DEVICE()/SYS_BUS_DEVICE(), so it
just needs to block the memory, hopefully without needing to distinguish
between ->gic.emulated and ->gic.kvm pointers.
The decision doesn't depend on any user-settable property, just on the
at this point global kvm_enabled() state, so I see nowhere else to
allocate it dynamically.

If you change the .instance_size struct one of the GICs uses, then a
number of places will need to be reviewed, including
ARM_GIC_COMMON()[*], ARM_GIC() and KVM_ARM_GIC() all returning the same
type.

[*] When we're through with the functional changes, we should
s/ARM_GIC_COMMON/COMMON_ARM_GIC/g to match the general pattern.

Andreas

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