From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RuXY2-0005St-CW for qemu-devel@nongnu.org; Mon, 06 Feb 2012 18:00:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RuXXy-00074O-Ji for qemu-devel@nongnu.org; Mon, 06 Feb 2012 18:00:34 -0500 Received: from mail-yx0-f173.google.com ([209.85.213.173]:41975) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RuXXy-000747-Ew for qemu-devel@nongnu.org; Mon, 06 Feb 2012 18:00:30 -0500 Received: by yenr1 with SMTP id r1so199426yen.4 for ; Mon, 06 Feb 2012 15:00:29 -0800 (PST) Message-ID: <4F305B8A.9080901@codemonkey.ws> Date: Mon, 06 Feb 2012 17:00:26 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1328237992-14953-1-git-send-email-afaerber@suse.de> <1328237992-14953-6-git-send-email-afaerber@suse.de> In-Reply-To: <1328237992-14953-6-git-send-email-afaerber@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH RFC v3 05/21] target-arm: Embed CPUARMState in QOM ARMCPU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Peter Maydell , Paul Brook , qemu-devel@nongnu.org, Richard Henderson 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 > Cc: Anthony Liguori > Cc: Paul Brook > Cc: Peter Maydell > Cc: Richard Henderson > --- > 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)