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

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/02/2012 08:59 PM, 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 instance_init
> function. Same for cpu_model_str, which is now the QOM class name.
>
> Make cpu_reset() call cpu_do_reset() and move most of its code to
> arm_cpu_reset().
>
> Signed-off-by: Andreas Färber<afaerber@suse.de>
> Cc: Anthony Liguori<anthony@codemonkey.ws>
> Cc: Paul Brook<paul@codesourcery.com>
> Cc: Peter Maydell<peter.maydell@linaro.org>
> Cc: Richard Henderson<rth@twiddle.net>
> ---
>   target-arm/cpu-core.h |   11 +++++++
>   target-arm/cpu.c      |   78 +++++++++++++++++++++++++++++++++++++++++++++++++
>   target-arm/helper.c   |   71 ++++----------------------------------------
>   3 files changed, 96 insertions(+), 64 deletions(-)
>
> diff --git a/target-arm/cpu-core.h b/target-arm/cpu-core.h
> index 3e8f046..4ba5ee0 100644
> --- a/target-arm/cpu-core.h
> +++ b/target-arm/cpu-core.h
> @@ -21,6 +21,7 @@
>   #define QEMU_ARM_CPU_CORE_H
>
>   #include "qemu/cpu.h"
> +#include "cpu.h"
>
>   #define TYPE_ARM_CPU "arm-cpu"
>
> @@ -53,7 +54,17 @@ typedef struct ARMCPUClass {
>   typedef struct ARMCPU {
>       /*<  private>*/
>       CPU parent_obj;
> +
> +    /* TODO Inline this and split off common state */
> +    CPUARMState env;
>   } ARMCPU;
>
> +static inline Object *arm_env_get_object(CPUARMState *env)
> +{
> +    return OBJECT((void *)(env) - offsetof(ARMCPU, env));
> +}
> +
> +#define ENV_GET_OBJECT(e) arm_env_get_object(e)

I'd prefer:

ARMCPU *arm_cpu_from_cpu_state(CPUState *env)
{
    return ARM_CPU(container_of(env, ARMCPU, env));
}

Regards,

Anthony Liguori

>
>   #endif
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 9255a19..43231c9 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -20,10 +20,75 @@
>
>   #include "cpu-core.h"
>   #include "qemu-common.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/loader.h"
> +#endif
>
>   static void arm_cpu_reset(CPU *c)
>   {
> +    ARMCPU *cpu = ARM_CPU(c);
> +    CPUARMState *env =&cpu->env;
> +    uint32_t id;
> +    uint32_t tmp;
> +
> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> +        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
> +        log_cpu_state(env, 0);
> +    }
> +
>       cpu_common_reset(c);
> +
> +    id = env->cp15.c0_cpuid;
> +    tmp = env->cp15.c15_config_base_address;
> +    memset(env, 0, offsetof(CPUARMState, breakpoints));
> +    env->cp15.c0_cpuid = id;
> +    env->cp15.c15_config_base_address = tmp;
> +
> +#if defined(CONFIG_USER_ONLY)
> +    env->uncached_cpsr = ARM_CPU_MODE_USR;
> +    /* For user mode we must enable access to coprocessors */
> +    env->vfp.xregs[ARM_VFP_FPEXC] = 1<<  30;
> +    if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
> +        env->cp15.c15_cpar = 3;
> +    } else if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> +        env->cp15.c15_cpar = 1;
> +    }
> +#else
> +    /* SVC mode with interrupts disabled.  */
> +    env->uncached_cpsr = ARM_CPU_MODE_SVC | CPSR_A | CPSR_F | CPSR_I;
> +    /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
> +       clear at reset.  Initial SP and PC are loaded from ROM.  */
> +    if (IS_M(env)) {
> +        uint32_t pc;
> +        uint8_t *rom;
> +        env->uncached_cpsr&= ~CPSR_I;
> +        rom = rom_ptr(0);
> +        if (rom) {
> +            /* We should really use ldl_phys here, in case the guest
> +               modified flash and reset itself.  However images
> +               loaded via -kernel have not been copied yet, so load the
> +               values directly from there.  */
> +            env->regs[13] = ldl_p(rom);
> +            pc = ldl_p(rom + 4);
> +            env->thumb = pc&  1;
> +            env->regs[15] = pc&  ~1;
> +        }
> +    }
> +    env->vfp.xregs[ARM_VFP_FPEXC] = 0;
> +    env->cp15.c2_base_mask = 0xffffc000u;
> +    /* v7 performance monitor control register: same implementor
> +     * field as main ID register, and we implement no event counters.
> +     */
> +    env->cp15.c9_pmcr = (id&  0xff000000);
> +#endif
> +    set_flush_to_zero(1,&env->vfp.standard_fp_status);
> +    set_flush_inputs_to_zero(1,&env->vfp.standard_fp_status);
> +    set_default_nan_mode(1,&env->vfp.standard_fp_status);
> +    set_float_detect_tininess(float_tininess_before_rounding,
> +&env->vfp.fp_status);
> +    set_float_detect_tininess(float_tininess_before_rounding,
> +&env->vfp.standard_fp_status);
> +    tlb_flush(env, 1);
>   }
>
>   /* CPU models */
> @@ -144,6 +209,18 @@ static const ARMCPUInfo arm_cpus[] = {
>       },
>   };
>
> +static void arm_cpu_initfn(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->cp15.c0_cpuid;
> +}
> +
>   static void arm_cpu_class_init(ObjectClass *klass, void *data)
>   {
>       ARMCPUClass *k = ARM_CPU_CLASS(klass);
> @@ -161,6 +238,7 @@ static void cpu_register(const ARMCPUInfo *info)
>           .name = info->name,
>           .parent = TYPE_ARM_CPU,
>           .instance_size = sizeof(ARMCPU),
> +        .instance_init = arm_cpu_initfn,
>           .class_size = sizeof(ARMCPUClass),
>           .class_init = arm_cpu_class_init,
>           .class_data = (void *)info,
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d8425db..c5ba7fd 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7,9 +7,6 @@
>   #include "helper.h"
>   #include "qemu-common.h"
>   #include "host-utils.h"
> -#if !defined(CONFIG_USER_ONLY)
> -#include "hw/loader.h"
> -#endif
>   #include "sysemu.h"
>   #include "cpu-core.h"
>
> @@ -59,7 +56,6 @@ static inline void set_feature(CPUARMState *env, int feature)
>
>   static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>   {
> -    env->cp15.c0_cpuid = id;
>       switch (id) {
>       case ARM_CPUID_ARM926:
>           set_feature(env, ARM_FEATURE_V5);
> @@ -285,64 +281,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>   void cpu_reset(CPUARMState *env)
>   {
>       uint32_t id;
> -    uint32_t tmp = 0;
>
> -    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> -        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
> -        log_cpu_state(env, 0);
> -    }
> +    cpu_do_reset(CPU(ENV_GET_OBJECT(env)));
>
>       id = env->cp15.c0_cpuid;
> -    tmp = env->cp15.c15_config_base_address;
> -    memset(env, 0, offsetof(CPUARMState, breakpoints));
>       if (id)
>           cpu_reset_model_id(env, id);
> -    env->cp15.c15_config_base_address = tmp;
> -#if defined (CONFIG_USER_ONLY)
> -    env->uncached_cpsr = ARM_CPU_MODE_USR;
> -    /* For user mode we must enable access to coprocessors */
> -    env->vfp.xregs[ARM_VFP_FPEXC] = 1<<  30;
> -    if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
> -        env->cp15.c15_cpar = 3;
> -    } else if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> -        env->cp15.c15_cpar = 1;
> -    }
> -#else
> -    /* SVC mode with interrupts disabled.  */
> -    env->uncached_cpsr = ARM_CPU_MODE_SVC | CPSR_A | CPSR_F | CPSR_I;
> -    /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
> -       clear at reset.  Initial SP and PC are loaded from ROM.  */
> -    if (IS_M(env)) {
> -        uint32_t pc;
> -        uint8_t *rom;
> -        env->uncached_cpsr&= ~CPSR_I;
> -        rom = rom_ptr(0);
> -        if (rom) {
> -            /* We should really use ldl_phys here, in case the guest
> -               modified flash and reset itself.  However images
> -               loaded via -kernel have not been copied yet, so load the
> -               values directly from there.  */
> -            env->regs[13] = ldl_p(rom);
> -            pc = ldl_p(rom + 4);
> -            env->thumb = pc&  1;
> -            env->regs[15] = pc&  ~1;
> -        }
> -    }
> -    env->vfp.xregs[ARM_VFP_FPEXC] = 0;
> -    env->cp15.c2_base_mask = 0xffffc000u;
> -    /* v7 performance monitor control register: same implementor
> -     * field as main ID register, and we implement no event counters.
> -     */
> -    env->cp15.c9_pmcr = (id&  0xff000000);
> -#endif
> -    set_flush_to_zero(1,&env->vfp.standard_fp_status);
> -    set_flush_inputs_to_zero(1,&env->vfp.standard_fp_status);
> -    set_default_nan_mode(1,&env->vfp.standard_fp_status);
> -    set_float_detect_tininess(float_tininess_before_rounding,
> -&env->vfp.fp_status);
> -    set_float_detect_tininess(float_tininess_before_rounding,
> -&env->vfp.standard_fp_status);
> -    tlb_flush(env, 1);
>   }
>
>   static int vfp_gdb_get_reg(CPUState *env, uint8_t *buf, int reg)
> @@ -400,7 +344,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;
>
> @@ -413,16 +357,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(cpu_model));
> +    env =&cpu->env;
> +
>       if (tcg_enabled()&&  !inited) {
>           inited = 1;
>           arm_translate_init();
>       }
>
> -    env->cpu_model_str = cpu_model;
> -    env->cp15.c0_cpuid = cpu_class->cp15.c0_cpuid;
>       cpu_reset(env);
>       if (arm_feature(env, ARM_FEATURE_NEON)) {
>           gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg,
> @@ -464,7 +406,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)