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

[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/.)

>  #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).

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, 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.

thanks
-- PMM