Re: [Qemu-devel] [PATCH RFC 7/7] target-arm: Embed CPUARMState in QOM ARMCPU

[Anthony Liguori]

On 01/29/2012 07:25 AM, Andreas Färber wrote:
> We g_malloc0()'ed CPUARMState ourself, and exec.c's cpu_copy() runs
> through cpu_init() as well, so we are at liberty to supply the CPUState
> any way we see fit. Having CPUARMState as field in the QOM CPU allows
> both to access env from an ARMCPU object and to access the QOM Object
> and its ObjectClass from an env pointer, in ARM code for now.
>
> The goal is to convert all CPUs to QOM and to use CPU objects in central
> places, especially once we have property support for Object.
> This will then allow to have TCG AREG0 point to target-specific fields
> where small immediate offsets are desired (as pointed out by rth) while
> allowing for common fields at known offsets from the base class.
>
> Having the CPUID in ARMCPUClass, we can set it from the realize function.
> Same for cpu_model_str, which is now the QOM class name.
>
> Signed-off-by: Andreas Färber<address@hidden>
> Cc: Anthony Liguori<address@hidden>
> Cc: Paul Brook<address@hidden>
> Cc: Peter Maydell<address@hidden>
> Cc: Richard Henderson<address@hidden>
> ---
>   target-arm/cpu-core.c |   13 +++++++++++++
>   target-arm/cpu-core.h |    7 +++++++
>   target-arm/helper.c   |   13 ++++++-------
>   3 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/cpu-core.c b/target-arm/cpu-core.c
> index 9761d8e..b1ac22c 100644
> --- a/target-arm/cpu-core.c
> +++ b/target-arm/cpu-core.c
> @@ -234,12 +234,25 @@ static const struct ARMCPUDef arm_cpu_models[] = {
>       { }
>   };
>
> +static void arm_cpu_realize(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    ARMCPUClass *cpu_class = ARM_CPU_GET_CLASS(obj);
> +
> +    memset(&cpu->env, 0, sizeof(CPUARMState));
> +    cpu_exec_init(&cpu->env);
> +
> +    cpu->env.cpu_model_str = object_get_typename(obj);
> +    cpu->env.cp15.c0_cpuid = cpu_class->id;
> +}
> +
>   static void cpu_register(const struct ARMCPUDef *def)
>   {
>       TypeInfo type = {
>           .name = def->name,
>           .parent = TYPE_ARM_CPU,
>           .instance_size = sizeof(ARMCPU),
> +        .instance_init = arm_cpu_realize,


The convention I'm using is: instance_init => type_name_initfn.

DeviceState::init => type_name_realize,

Eventually, realized will become a property and there will be a realize and 
unrealize method.

>           .class_size = sizeof(ARMCPUClass),
>           .class_init = def->class_init,
>       };
> diff --git a/target-arm/cpu-core.h b/target-arm/cpu-core.h
> index be4bbc3..08b6b2b 100644
> --- a/target-arm/cpu-core.h
> +++ b/target-arm/cpu-core.h
> @@ -10,6 +10,7 @@
>   #define QEMU_ARM_CPU_CORE_H
>
>   #include "qemu/cpu.h"
> +#include "cpu.h"
>
>   #define TYPE_ARM_CPU "arm-cpu-core"
>   #define ARM_CPU_CLASS(klass) \
> @@ -27,7 +28,13 @@ typedef struct ARMCPUClass {
>
>   typedef struct ARMCPU {
>       CPU parent_obj;
> +
> +    /* TODO Inline this and split off common state */
> +    CPUARMState env;

This is an interesting (and good) idea.  I think this could make it fairly easy 
to qomify things.

>   } ARMCPU;
>
> +#define ENV_GET_OBJECT(e) \
> +    (Object *)((void *)(e) - offsetof(ARMCPU, env))

sizeof(CPU) should be sizeof(void *).  Presumably it's okay to add 8 bytes to 
the beginning of CPUState?

If so, that would make things much nicer from a QOM perspective.

Regards,

Anthony Liguori

>   #endif
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ece9635..1ffd7ba 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -400,7 +400,7 @@ static int vfp_gdb_set_reg(CPUState *env, uint8_t *buf, int 
> reg)
>   CPUARMState *cpu_arm_init(const char *cpu_model)
>   {
>       ObjectClass *klass;
> -    ARMCPUClass *cpu_class;
> +    ARMCPU *cpu;
>       CPUARMState *env;
>       static int inited = 0;
>
> @@ -408,16 +408,14 @@ CPUARMState *cpu_arm_init(const char *cpu_model)
>       if (klass == NULL) {
>           return NULL;
>       }
> -    cpu_class = ARM_CPU_CLASS(klass);
> -    env = g_malloc0(sizeof(CPUARMState));
> -    cpu_exec_init(env);
> +    cpu = ARM_CPU(object_new_with_type(klass->type));
> +    env =&cpu->env;
> +
>       if (tcg_enabled()&&  !inited) {
>           inited = 1;
>           arm_translate_init();
>       }
>
> -    env->cpu_model_str = cpu_model;
> -    env->cp15.c0_cpuid = cpu_class->id;
>       cpu_reset(env);
>       if (arm_feature(env, ARM_FEATURE_NEON)) {
>           gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg,
> @@ -459,7 +457,8 @@ void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>
>   void cpu_arm_close(CPUARMState *env)
>   {
> -    g_free(env);
> +    Object *obj = ENV_GET_OBJECT(env);
> +    object_delete(obj);
>   }
>
>   static int bad_mode_switch(CPUState *env, int mode)