diff mbox

[RFC,v4,03/20] target-arm: Embed CPUARMState in QOM ARMCPU

Message ID 1331398436-20761-4-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber March 10, 2012, 4:53 p.m. UTC 
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_state_reset() call cpu_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: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
---
 target-arm/cpu-qom.h |   12 +++++++
 target-arm/cpu.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/helper.c  |   75 ++++------------------------------------------
 3 files changed, 100 insertions(+), 69 deletions(-)

Comments

Paolo Bonzini March 13, 2012, 1:18 p.m. UTC | #1 
Il 10/03/2012 17:53, Andreas Färber ha scritto:
>  /**
>   * ARMCPU:
> + * @env: Legacy CPU state.
>   *
>   * An ARM CPU core.
>   */
> @@ -58,7 +60,17 @@ typedef struct ARMCPU {
>      /*< private >*/
>      CPUState parent_obj;
>      /*< public >*/
> +
> +    /* TODO Inline this and split off common state */
> +    CPUARMState env;
>  } ARMCPU;
>  

As usual I don't like "legacy". :)  A more interesting distinction is
between state that is relevant to the emulation, and state that is
relevant to the main loop or the machine.  The former is reinitialized
after reset and can remain in CPUFooState.  The latter is not
reinitialized after reset, can move to QOM, and in most cases that's
what we access when we use CPUArchState.

Also, I understand that the conversion is not fully mechanical, but
perhaps it can be made "more" mechanical than this?  Splitting this
patch in two would be nice, but I can't say I read it fully.

Paolo
Andreas Färber March 14, 2012, 10:30 p.m. UTC | #2 
Am 13.03.2012 14:18, schrieb Paolo Bonzini:
> Il 10/03/2012 17:53, Andreas Färber ha scritto:
>>  /**
>>   * ARMCPU:
>> + * @env: Legacy CPU state.
>>   *
>>   * An ARM CPU core.
>>   */
>> @@ -58,7 +60,17 @@ typedef struct ARMCPU {
>>      /*< private >*/
>>      CPUState parent_obj;
>>      /*< public >*/
>> +
>> +    /* TODO Inline this and split off common state */
>> +    CPUARMState env;
>>  } ARMCPU;
>>  
> 
> As usual I don't like "legacy". :)

Well, it refers to "former CPUState of which some or many fields are
going to be extracted". I wouldn't want to write it into documentation
as such.
" * @env: CPUARMState" is not telling either.
Better wordings anyone?

>  A more interesting distinction is
> between state that is relevant to the emulation, and state that is
> relevant to the main loop or the machine.  The former is reinitialized
> after reset and can remain in CPUFooState.  The latter is not
> reinitialized after reset, can move to QOM, and in most cases that's
> what we access when we use CPUArchState.

Actually I was planning on keeping CPU*State only for TCG fields.

Whether a field is reset or not should be determined by the reset
callback, not by a semi-magic memset() up to a certain field name.
That's especially problematic for cp15.

The CPU*State struct stays around for two reasons,
1) allowing an incremental conversion of targets,
2) allowing offset calculations for TCG from the middle of the struct.

Mentioning this, we need to review all my *CPU structs to make sure env
is the *first* member and that any fields accessed by offset in its
parent CPUState are placed *last*, to minimize the immediates, as
previously pointed out by rth. Starts to matter once we move icount into
CPUState, as done on qom-cpu-wip branch.

> Also, I understand that the conversion is not fully mechanical, but
> perhaps it can be made "more" mechanical than this?  Splitting this
> patch in two would be nice, but I can't say I read it fully.

Hm, this patch has been around since v1 and so far nobody requested a
particular split... it would be possible to move init and reset code in
separate patches if deemed necessary? Peter?

Andreas
Paolo Bonzini March 15, 2012, 9:43 a.m. UTC | #3 
Il 14/03/2012 23:30, Andreas Färber ha scritto:
>> As usual I don't like "legacy". :)
> 
> Well, it refers to "former CPUState of which some or many fields are
> going to be extracted". I wouldn't want to write it into documentation
> as such.
> " * @env: CPUARMState" is not telling either.
> Better wordings anyone?
> 
>>  A more interesting distinction is
>> between state that is relevant to the emulation, and state that is
>> relevant to the main loop or the machine.  The former is reinitialized
>> after reset and can remain in CPUFooState.  The latter is not
>> reinitialized after reset, can move to QOM, and in most cases that's
>> what we access when we use CPUArchState.
> 
> Actually I was planning on keeping CPU*State only for TCG fields.

@env: CPUARMState structure (only TCG fields should remain there)

> Whether a field is reset or not should be determined by the reset
> callback, not by a semi-magic memset() up to a certain field name.
> That's especially problematic for cp15.

True.  My idea was to leave all resettable fields in CPUARMState, and
move the others in ARMCPU.  This is consistent with "machine state" in
CPUARMState and "machine settings + main-loop state" in ARMCPU.
However, it may make little sense to you.  If so, ignore me. :)

>> Also, I understand that the conversion is not fully mechanical, but
>> perhaps it can be made "more" mechanical than this?  Splitting this
>> patch in two would be nice, but I can't say I read it fully.
> 
> Hm, this patch has been around since v1 and so far nobody requested a
> particular split... it would be possible to move init and reset code in
> separate patches if deemed necessary? Peter?

No problem, now that you posted conversion of everything.

Paolo
diff mbox

Patch

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index b2917ea..9e33b6a 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -21,6 +21,7 @@ 
 #define QEMU_ARM_CPU_QOM_H
 
 #include "qemu/cpu.h"
+#include "cpu.h"
 
 #define TYPE_ARM_CPU "arm-cpu"
 
@@ -51,6 +52,7 @@  typedef struct ARMCPUClass {
 
 /**
  * ARMCPU:
+ * @env: Legacy CPU state.
  *
  * An ARM CPU core.
  */
@@ -58,7 +60,17 @@  typedef struct ARMCPU {
     /*< private >*/
     CPUState parent_obj;
     /*< public >*/
+
+    /* TODO Inline this and split off common state */
+    CPUARMState env;
 } ARMCPU;
 
+static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
+{
+    return ARM_CPU(container_of(env, ARMCPU, env));
+}
+
+#define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e))
+
 
 #endif
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index dabc094..00b7ecc 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -20,12 +20,81 @@ 
 
 #include "cpu-qom.h"
 #include "qemu-common.h"
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/loader.h"
+#endif
 
 static void arm_cpu_reset(CPUState *c)
 {
     ARMCPUClass *klass = ARM_CPU_GET_CLASS(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);
+    }
 
     klass->parent_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);
+    /* Reset is a state change for some CPUState fields which we
+     * bake assumptions about into translated code, so we need to
+     * tb_flush().
+     */
+    tb_flush(env);
 }
 
 /* CPU models */
@@ -146,6 +215,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);
@@ -164,6 +245,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 bc29183..39f3c40 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2,9 +2,6 @@ 
 #include "gdbstub.h"
 #include "helper.h"
 #include "host-utils.h"
-#if !defined(CONFIG_USER_ONLY)
-#include "hw/loader.h"
-#endif
 #include "sysemu.h"
 #include "cpu-qom.h"
 
@@ -54,7 +51,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);
@@ -280,69 +276,12 @@  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 void cpu_state_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_reset(ENV_GET_CPU(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);
-    /* Reset is a state change for some CPUARMState fields which we
-     * bake assumptions about into translated code, so we need to
-     * tb_flush().
-     */
-    tb_flush(env);
 }
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -400,7 +339,7 @@  static int vfp_gdb_set_reg(CPUARMState *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 +352,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_state_reset(env);
     if (arm_feature(env, ARM_FEATURE_NEON)) {
         gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg,
@@ -487,7 +424,7 @@  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 
 void cpu_arm_close(CPUARMState *env)
 {
-    g_free(env);
+    object_delete(OBJECT(ENV_GET_CPU(env)));
 }
 
 static int bad_mode_switch(CPUARMState *env, int mode)