Re: [Qemu-devel] [PATCH RFC 03/15] cpu/a9mpcore: Embed GICState

[Andreas Färber]

Am 01.07.2013 00:13, schrieb Peter Maydell:
> On 30 June 2013 22:00, Andreas Färber <address@hidden> wrote:
>> From: Andreas Färber <address@hidden>
>>
>> Prepares for conversion to QOM realize.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  hw/cpu/a9mpcore.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>> index 63a4eb1..6340b0f 100644
>> --- a/hw/cpu/a9mpcore.c
>> +++ b/hw/cpu/a9mpcore.c
>> @@ -9,6 +9,7 @@
>>   */
>>
>>  #include "hw/sysbus.h"
>> +#include "hw/intc/gic_internal.h"
> 
> This doesn't look right -- the GIC internals are supposed
> to be internal to the GIC implementation (and its subclasses).
> They shouldn't be exposed to users. (That's why the header
> is in hw/intc and not in include/.)

I can easily add an include/hw/intc/arm_gic.h header to address that.
(Previously all device headers were in hw/.)

>>  #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
>>  #define A9MPCORE_PRIV(obj) \
>> @@ -23,15 +24,17 @@ typedef struct A9MPPrivState {
>>      MemoryRegion container;
>>      DeviceState *mptimer;
>>      DeviceState *wdt;
>> -    DeviceState *gic;
>>      DeviceState *scu;
>>      uint32_t num_irq;
>> +
>> +    GICState gic;
>>  } A9MPPrivState;
> 
> So we sort of had a discussion about this before, but I
> still don't think we have a sensible way of doing
> embedding of devices into container device structures
> like this properly (ie without exposing implementation
> internals that qdev doesn't require you to expose).

We didn't have a discussion, we had you complaining about how great qdev
was and how little you like QOM. Anthony told you quite clearly that
this is how it's supposed to be done with his QOM:

* in TypeInfo::instance_init initialize child devices with
object_initialize(), which is not supposed to fail,
* property setters can report failure via **errp and may therefore
dynamically allocate children - you implemented your array static
properties as a consequence,
* DeviceClass::realize shall not create new devices as they would be
hidden from management tools in the future QMP phase (-S) before
realization and would break recursive realization due to devices
appearing while iterating.

So when we have one or a fixed number of children, it seems clear to me
that they shall be embedded into the struct as done in this series.

You had several months to come up with a solution of your liking, but
apparently you didn't step forward with any patchset or proposal. ;)
I'd be more than happy if you refactored ARM code yourself, that would
mean less work for me and/or Tao. Saying no is not constructive though.

The only way I can think of to shield struct internals while still
reserving space to avoid dynamic allocation would be:

uint8_t gic[sizeof(GICState)];

which would still need access to the real GICState struct through a
header, while prohibiting access so s->gic.foo. This could be hidden in
a macro CHILD(typename, varname) to assure proper alignment.
CC'ing Anthony and Paolo.

But casting to the struct type is so simple to do that we can just use
the struct in the first place and let the compiler handle alignment for
us. As you can see from my Tegra2 patches, I am doing nothing bad with
a9mpcore_priv, not accessing its internals at all! And since you're
reviewing all ARM patches anyway, you could still reject any patches
accessing device internals.

Me being prompted by AArch64, we need an improvement over qdev code
*now* before its patterns get copied into further devices, creating even
more obstacles to implementing recursive realization.

> If we want to do struct-embedding then I think we should
> come up with a standard way of writing that code (eg
> a .h file under include/hw with type name macros and a
> struct with only the public-facing bits and internal
> members hidden behind a typedef somehow,

That is simply not possible in C. There are no public vs. internal
fields since everything can be accessed via pointer dereference anyway,
whatever way we use to declare pointers and fields.

By Anthony's QOM conventions, the /*< private >*/ gtk-doc marker hides
stuff from documentation completely (i.e., the parent_obj field not to
be accessed), whereas device fields should be documented for use within
the device, thus we need to keep them /*< public >*/ there.

Personally, I consider all device fields private and consider direct
access to random devices' fields as an abstraction breach and prefer to
use accessor functions for that (Data Caging). But I remember you
rejecting the accessor functions Anthony introduced for interacting with
QOM pins as "boilerplate", so this argument feels double-tongued.

> and a .priv.h
> in hw/whatever/ with the actual struct that the device
> needs itself), and then start using that. Otherwise we
> should just keep using pointers since they can happily
> be opaque about the details of the struct they point to.

Again, pointers in instance_init break the QOM allocation model.

The point is that g_malloc0(sizeof(MySoCState)) is expected to allocate
the size of the object including its children, whereas the current state
of things allocates the device only and allocates children as part of
qdev_create() in DeviceClass::init, which is too late for the QOM
realize model.

Thanks,
Andreas

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