[RFC,7/7] target-arm: Embed CPUARMState in QOM ARMCPU

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 <afaerber@suse.de>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Paul Brook <paul@codesourcery.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
---
 target-arm/cpu-core.c |   13 +++++++++++++
 target-arm/cpu-core.h |    7 +++++++
 target-arm/helper.c   |   13 ++++++-------
 3 files changed, 26 insertions(+), 7 deletions(-)

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<afaerber@suse.de>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> Cc: Paul Brook<paul@codesourcery.com>
> Cc: Peter Maydell<peter.maydell@linaro.org>
> Cc: Richard Henderson<rth@twiddle.net>
> ---
>   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)

Am 30.01.2012 03:22, schrieb Anthony Liguori:
> On 01/29/2012 07:25 AM, Andreas Färber wrote:
>> +#define ENV_GET_OBJECT(e) \
>> +    (Object *)((void *)(e) - offsetof(ARMCPU, env))
> 
> sizeof(CPU) should be sizeof(void *).

Not following... CPU is a struct, so:
sizeof(ARMCPU) > sizeof(CPU) >= sizeof(Object) == 2x sizeof(void *)

The above formula is calculating the address of the ARMCPU object the
CPUState *env pointer belongs to. Maybe I should make it an inline
function for readability, I just followed the QOM naming conventions.

> Presumably it's okay to add 8
> bytes to the beginning of CPUState?

The whole problem with today's CPUState is that CPU_COMMON fields are at
the back or in the middle, with target-specific offset from the pointer.
We can of course add an Object *obj field to the start of each
CPU*State, but rth's (and Peter's?) worry was that adding things to the
front may push fields in a (TCG) hot path out of range for
immediate-offset load/stores. Thus if we just do it in the back for ARM
I don't see a benefit over the above solution. Per target we know the
offset.

Fields in the front of CPUState are usually
memset(env, 0, offsetof(CPUState, breakpoints));
on reset - one issue this series is working towards cleaning up. :) If
we add an Object* to the front we also have to save and restore it on
reset per target.

Again, the idea here is an interim solution to get moving in the right
direction. Things that are target-specific should not be accessed from
generic code but indirected through CPUClass methods, dispatching to
target code (e.g., reset), at some point obsoleting the aliased
cpu_reset() functions with one real function in cpu.c.
If it turns out that the TLB is common code modulo target_ulong or so
then it may be moved from CPU*State to CPU, making common code work with
CPU rather than CPUState. Long-term, today's CPUState would remain
what's needed for TCG and it's AREG0 + offset(CPUState, x) calculations.

Regards,
Andreas

> If so, that would make things much nicer from a QOM perspective.
> 
> Regards,
> 
> Anthony Liguori

Am 30.01.2012 03:22, schrieb Anthony Liguori:
> On 01/29/2012 07:25 AM, Andreas Färber wrote:
>> +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,

Fixed, thanks.

Andreas

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