diff mbox

[10/14] target-arm: Move feature register setup to per-CPU init fns

Message ID 1335033008-12800-11-git-send-email-peter.maydell@linaro.org
State New
Headers show

Commit Message

Peter Maydell April 21, 2012, 6:30 p.m. UTC 
Move feature register value setup to per-CPU init functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Andreas Färber <afaerber@suse.de>
---
 target-arm/cpu-qom.h |   14 +++++++
 target-arm/cpu.c     |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/helper.c  |   73 +++++++-------------------------------
 3 files changed, 122 insertions(+), 59 deletions(-)

Comments

Paul Brook April 30, 2012, 4:21 p.m. UTC | #1 
> Move feature register value setup to per-CPU init functions.

> +    env->cp15.c0_c1[0] = cpu->id_pfr0;
> +    env->cp15.c0_c1[1] = cpu->id_pfr1;
> +    env->cp15.c0_c1[2] = cpu->id_dfr0;
> +    env->cp15.c0_c1[3] = cpu->id_afr0;
> +    env->cp15.c0_c1[4] = cpu->id_mmfr0;
> +    env->cp15.c0_c1[5] = cpu->id_mmfr1;
> +    env->cp15.c0_c1[6] = cpu->id_mmfr2;
> +    env->cp15.c0_c1[7] = cpu->id_mmfr3;
> +    env->cp15.c0_c2[0] = cpu->id_isar0;
> +    env->cp15.c0_c2[1] = cpu->id_isar1;
> +    env->cp15.c0_c2[2] = cpu->id_isar2;
> +    env->cp15.c0_c2[3] = cpu->id_isar3;
> +    env->cp15.c0_c2[4] = cpu->id_isar4;
> +    env->cp15.c0_c2[5] = cpu->id_isar5;

Why are we copying these values? All these registers are readonly, so the 
duplication seems wrong.  Shouldn't we should be using cpu->whatever 
everywhere?

I feel like I've asked this before, but don't remember seeing an answer.


Also, I'd prefer that id_isr5 were explicitly initialized, rather than relying 
on it being implicitly zero.  Bugs in an earlier patch series show how easy it 
is to accidentally miss a register.   IMO it's worth distinguishing a defined 
register that happens to be zero from a register this core doesn't have.  
Overall I'm not convinced that the new open-coded initialization is better 
then the tables it replaces.

Paul
Peter Maydell April 30, 2012, 4:26 p.m. UTC | #2 
On 30 April 2012 17:21, Paul Brook <paul@codesourcery.com> wrote:
>> Move feature register value setup to per-CPU init functions.
>
>> +    env->cp15.c0_c1[0] = cpu->id_pfr0;
>> +    env->cp15.c0_c1[1] = cpu->id_pfr1;
>> +    env->cp15.c0_c1[2] = cpu->id_dfr0;
>> +    env->cp15.c0_c1[3] = cpu->id_afr0;
>> +    env->cp15.c0_c1[4] = cpu->id_mmfr0;
>> +    env->cp15.c0_c1[5] = cpu->id_mmfr1;
>> +    env->cp15.c0_c1[6] = cpu->id_mmfr2;
>> +    env->cp15.c0_c1[7] = cpu->id_mmfr3;
>> +    env->cp15.c0_c2[0] = cpu->id_isar0;
>> +    env->cp15.c0_c2[1] = cpu->id_isar1;
>> +    env->cp15.c0_c2[2] = cpu->id_isar2;
>> +    env->cp15.c0_c2[3] = cpu->id_isar3;
>> +    env->cp15.c0_c2[4] = cpu->id_isar4;
>> +    env->cp15.c0_c2[5] = cpu->id_isar5;
>
> Why are we copying these values? All these registers are readonly, so the
> duplication seems wrong.  Shouldn't we should be using cpu->whatever
> everywhere?
>
> I feel like I've asked this before, but don't remember seeing an answer.

My proposed cp15 rework deletes this copying code; instead the registers
are defined with values set directly from the cpu->whatever fields:
 http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01811.html

> Also, I'd prefer that id_isr5 were explicitly initialized, rather than relying
> on it being implicitly zero.  Bugs in an earlier patch series show how easy it
> is to accidentally miss a register.   IMO it's worth distinguishing a defined
> register that happens to be zero from a register this core doesn't have.

Yes, I think that's probably a good idea.

-- PMM
Andreas Färber April 30, 2012, 4:48 p.m. UTC | #3 
Am 30.04.2012 18:21, schrieb Paul Brook:
>> Move feature register value setup to per-CPU init functions.
> 
>> +    env->cp15.c0_c1[0] = cpu->id_pfr0;
>> +    env->cp15.c0_c1[1] = cpu->id_pfr1;
>> +    env->cp15.c0_c1[2] = cpu->id_dfr0;
>> +    env->cp15.c0_c1[3] = cpu->id_afr0;
>> +    env->cp15.c0_c1[4] = cpu->id_mmfr0;
>> +    env->cp15.c0_c1[5] = cpu->id_mmfr1;
>> +    env->cp15.c0_c1[6] = cpu->id_mmfr2;
>> +    env->cp15.c0_c1[7] = cpu->id_mmfr3;
>> +    env->cp15.c0_c2[0] = cpu->id_isar0;
>> +    env->cp15.c0_c2[1] = cpu->id_isar1;
>> +    env->cp15.c0_c2[2] = cpu->id_isar2;
>> +    env->cp15.c0_c2[3] = cpu->id_isar3;
>> +    env->cp15.c0_c2[4] = cpu->id_isar4;
>> +    env->cp15.c0_c2[5] = cpu->id_isar5;
> 
> Why are we copying these values? All these registers are readonly, so the 
> duplication seems wrong.  Shouldn't we should be using cpu->whatever 
> everywhere?
> 
> I feel like I've asked this before, but don't remember seeing an answer.

You had asked about duplication between ARMCPUClass and ARMCPU, and
there were answers.

Peter has now avoided adding fields to ARMCPUClass in favor of adding
some fields to ARMCPU and multiple initfn functions that spare some
ARMCPUClass -> ARMCPU copying through imperative hardcoding. In
particular that was your request wrt non-declarative feature flags.

There is also a follow-up series by Peter which reworks cp15 as a list
of structs, so touching all cp15 code for cpu-> rather than env-> access
did not seem like a good idea to me.

> Overall I'm not convinced that the new open-coded initialization is better 
> then the tables it replaces.

What I like better after this series is that it's in CPU-specific code
rather than in common code trying to cater to all CPU models based on
CPUID, which includes revision and variant numbers that we would like to
expose as user-tunable QOM properties rather than as different CPU types
where possible.

Andreas
diff mbox

Patch

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 97f7e90..7603eff 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -75,6 +75,20 @@  typedef struct ARMCPU {
     uint32_t mvfr1;
     uint32_t ctr;
     uint32_t reset_sctlr;
+    uint32_t id_pfr0;
+    uint32_t id_pfr1;
+    uint32_t id_dfr0;
+    uint32_t id_afr0;
+    uint32_t id_mmfr0;
+    uint32_t id_mmfr1;
+    uint32_t id_mmfr2;
+    uint32_t id_mmfr3;
+    uint32_t id_isar0;
+    uint32_t id_isar1;
+    uint32_t id_isar2;
+    uint32_t id_isar3;
+    uint32_t id_isar4;
+    uint32_t id_isar5;
 } ARMCPU;
 
 static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 74a7d20..333f7fc 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -130,6 +130,13 @@  static void arm1026_initfn(Object *obj)
 static void arm1136_r2_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
+    /* What qemu calls "arm1136_r2" is actually the 1136 r0p2, ie an
+     * older core than plain "arm1136". In particular this does not
+     * have the v6K features.
+     * These ID register values are correct for 1136 but may be wrong
+     * for 1136_r2 (in particular r0p2 does not actually implement most
+     * of the ID registers).
+     */
     set_feature(&cpu->env, ARM_FEATURE_V6);
     set_feature(&cpu->env, ARM_FEATURE_VFP);
     cpu->midr = ARM_CPUID_ARM1136_R2;
@@ -138,6 +145,18 @@  static void arm1136_r2_initfn(Object *obj)
     cpu->mvfr1 = 0x00000000;
     cpu->ctr = 0x1dd20d2;
     cpu->reset_sctlr = 0x00050078;
+    cpu->id_pfr0 = 0x111;
+    cpu->id_pfr1 = 0x1;
+    cpu->id_dfr0 = 0x2;
+    cpu->id_afr0 = 0x3;
+    cpu->id_mmfr0 = 0x01130003;
+    cpu->id_mmfr1 = 0x10030302;
+    cpu->id_mmfr2 = 0x01222110;
+    cpu->id_isar0 = 0x00140011;
+    cpu->id_isar1 = 0x12002111;
+    cpu->id_isar2 = 0x11231111;
+    cpu->id_isar3 = 0x01102131;
+    cpu->id_isar4 = 0x141;
 }
 
 static void arm1136_initfn(Object *obj)
@@ -152,6 +171,18 @@  static void arm1136_initfn(Object *obj)
     cpu->mvfr1 = 0x00000000;
     cpu->ctr = 0x1dd20d2;
     cpu->reset_sctlr = 0x00050078;
+    cpu->id_pfr0 = 0x111;
+    cpu->id_pfr1 = 0x1;
+    cpu->id_dfr0 = 0x2;
+    cpu->id_afr0 = 0x3;
+    cpu->id_mmfr0 = 0x01130003;
+    cpu->id_mmfr1 = 0x10030302;
+    cpu->id_mmfr2 = 0x01222110;
+    cpu->id_isar0 = 0x00140011;
+    cpu->id_isar1 = 0x12002111;
+    cpu->id_isar2 = 0x11231111;
+    cpu->id_isar3 = 0x01102131;
+    cpu->id_isar4 = 0x141;
 }
 
 static void arm1176_initfn(Object *obj)
@@ -166,6 +197,18 @@  static void arm1176_initfn(Object *obj)
     cpu->mvfr1 = 0x00000000;
     cpu->ctr = 0x1dd20d2;
     cpu->reset_sctlr = 0x00050078;
+    cpu->id_pfr0 = 0x111;
+    cpu->id_pfr1 = 0x11;
+    cpu->id_dfr0 = 0x33;
+    cpu->id_afr0 = 0;
+    cpu->id_mmfr0 = 0x01130003;
+    cpu->id_mmfr1 = 0x10030302;
+    cpu->id_mmfr2 = 0x01222100;
+    cpu->id_isar0 = 0x0140011;
+    cpu->id_isar1 = 0x12002111;
+    cpu->id_isar2 = 0x11231121;
+    cpu->id_isar3 = 0x01102131;
+    cpu->id_isar4 = 0x01141;
 }
 
 static void arm11mpcore_initfn(Object *obj)
@@ -179,6 +222,18 @@  static void arm11mpcore_initfn(Object *obj)
     cpu->mvfr0 = 0x11111111;
     cpu->mvfr1 = 0x00000000;
     cpu->ctr = 0x1dd20d2;
+    cpu->id_pfr0 = 0x111;
+    cpu->id_pfr1 = 0x1;
+    cpu->id_dfr0 = 0;
+    cpu->id_afr0 = 0x2;
+    cpu->id_mmfr0 = 0x01100103;
+    cpu->id_mmfr1 = 0x10020302;
+    cpu->id_mmfr2 = 0x01222000;
+    cpu->id_isar0 = 0x00100011;
+    cpu->id_isar1 = 0x12002111;
+    cpu->id_isar2 = 0x11221011;
+    cpu->id_isar3 = 0x01102131;
+    cpu->id_isar4 = 0x141;
 }
 
 static void cortex_m3_initfn(Object *obj)
@@ -202,6 +257,19 @@  static void cortex_a8_initfn(Object *obj)
     cpu->mvfr1 = 0x00011100;
     cpu->ctr = 0x82048004;
     cpu->reset_sctlr = 0x00c50078;
+    cpu->id_pfr0 = 0x1031;
+    cpu->id_pfr1 = 0x11;
+    cpu->id_dfr0 = 0x400;
+    cpu->id_afr0 = 0;
+    cpu->id_mmfr0 = 0x31100003;
+    cpu->id_mmfr1 = 0x20000000;
+    cpu->id_mmfr2 = 0x01202000;
+    cpu->id_mmfr3 = 0x11;
+    cpu->id_isar0 = 0x00101111;
+    cpu->id_isar1 = 0x12112111;
+    cpu->id_isar2 = 0x21232031;
+    cpu->id_isar3 = 0x11112131;
+    cpu->id_isar4 = 0x00111142;
 }
 
 static void cortex_a9_initfn(Object *obj)
@@ -223,6 +291,19 @@  static void cortex_a9_initfn(Object *obj)
     cpu->mvfr1 = 0x01111111;
     cpu->ctr = 0x80038003;
     cpu->reset_sctlr = 0x00c50078;
+    cpu->id_pfr0 = 0x1031;
+    cpu->id_pfr1 = 0x11;
+    cpu->id_dfr0 = 0x000;
+    cpu->id_afr0 = 0;
+    cpu->id_mmfr0 = 0x00100103;
+    cpu->id_mmfr1 = 0x20000000;
+    cpu->id_mmfr2 = 0x01230000;
+    cpu->id_mmfr3 = 0x00002111;
+    cpu->id_isar0 = 0x00101111;
+    cpu->id_isar1 = 0x13112111;
+    cpu->id_isar2 = 0x21232041;
+    cpu->id_isar3 = 0x11112131;
+    cpu->id_isar4 = 0x00111142;
 }
 
 static void cortex_a15_initfn(Object *obj)
@@ -242,6 +323,19 @@  static void cortex_a15_initfn(Object *obj)
     cpu->mvfr1 = 0x11111111;
     cpu->ctr = 0x8444c004;
     cpu->reset_sctlr = 0x00c50078;
+    cpu->id_pfr0 = 0x00001131;
+    cpu->id_pfr1 = 0x00011011;
+    cpu->id_dfr0 = 0x02010555;
+    cpu->id_afr0 = 0x00000000;
+    cpu->id_mmfr0 = 0x10201105;
+    cpu->id_mmfr1 = 0x20000000;
+    cpu->id_mmfr2 = 0x01240000;
+    cpu->id_mmfr3 = 0x02102211;
+    cpu->id_isar0 = 0x02101110;
+    cpu->id_isar1 = 0x13112111;
+    cpu->id_isar2 = 0x21232041;
+    cpu->id_isar3 = 0x11112131;
+    cpu->id_isar4 = 0x10011142;
 }
 
 static void ti925t_initfn(Object *obj)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 319614a..84830ff 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7,45 +7,6 @@ 
 #endif
 #include "sysemu.h"
 
-static uint32_t cortexa15_cp15_c0_c1[8] = {
-    0x00001131, 0x00011011, 0x02010555, 0x00000000,
-    0x10201105, 0x20000000, 0x01240000, 0x02102211
-};
-
-static uint32_t cortexa15_cp15_c0_c2[8] = {
-    0x02101110, 0x13112111, 0x21232041, 0x11112131, 0x10011142, 0, 0, 0
-};
-
-static uint32_t cortexa9_cp15_c0_c1[8] =
-{ 0x1031, 0x11, 0x000, 0, 0x00100103, 0x20000000, 0x01230000, 0x00002111 };
-
-static uint32_t cortexa9_cp15_c0_c2[8] =
-{ 0x00101111, 0x13112111, 0x21232041, 0x11112131, 0x00111142, 0, 0, 0 };
-
-static uint32_t cortexa8_cp15_c0_c1[8] =
-{ 0x1031, 0x11, 0x400, 0, 0x31100003, 0x20000000, 0x01202000, 0x11 };
-
-static uint32_t cortexa8_cp15_c0_c2[8] =
-{ 0x00101111, 0x12112111, 0x21232031, 0x11112131, 0x00111142, 0, 0, 0 };
-
-static uint32_t mpcore_cp15_c0_c1[8] =
-{ 0x111, 0x1, 0, 0x2, 0x01100103, 0x10020302, 0x01222000, 0 };
-
-static uint32_t mpcore_cp15_c0_c2[8] =
-{ 0x00100011, 0x12002111, 0x11221011, 0x01102131, 0x141, 0, 0, 0 };
-
-static uint32_t arm1136_cp15_c0_c1[8] =
-{ 0x111, 0x1, 0x2, 0x3, 0x01130003, 0x10030302, 0x01222110, 0 };
-
-static uint32_t arm1136_cp15_c0_c2[8] =
-{ 0x00140011, 0x12002111, 0x11231111, 0x01102131, 0x141, 0, 0, 0 };
-
-static uint32_t arm1176_cp15_c0_c1[8] =
-{ 0x111, 0x11, 0x33, 0, 0x01130003, 0x10030302, 0x01222100, 0 };
-
-static uint32_t arm1176_cp15_c0_c2[8] =
-{ 0x0140011, 0x12002111, 0x11231121, 0x01102131, 0x01141, 0, 0, 0 };
-
 static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 {
     switch (id) {
@@ -58,43 +19,23 @@  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
     case ARM_CPUID_ARM1136:
         /* This is the 1136 r1, which is a v6K core */
     case ARM_CPUID_ARM1136_R2:
-        /* What qemu calls "arm1136_r2" is actually the 1136 r0p2, ie an
-         * older core than plain "arm1136". In particular this does not
-         * have the v6K features.
-         */
-        /* These ID register values are correct for 1136 but may be wrong
-         * for 1136_r2 (in particular r0p2 does not actually implement most
-         * of the ID registers).
-         */
-        memcpy(env->cp15.c0_c1, arm1136_cp15_c0_c1, 8 * sizeof(uint32_t));
-        memcpy(env->cp15.c0_c2, arm1136_cp15_c0_c2, 8 * sizeof(uint32_t));
         break;
     case ARM_CPUID_ARM1176:
-        memcpy(env->cp15.c0_c1, arm1176_cp15_c0_c1, 8 * sizeof(uint32_t));
-        memcpy(env->cp15.c0_c2, arm1176_cp15_c0_c2, 8 * sizeof(uint32_t));
         break;
     case ARM_CPUID_ARM11MPCORE:
-        memcpy(env->cp15.c0_c1, mpcore_cp15_c0_c1, 8 * sizeof(uint32_t));
-        memcpy(env->cp15.c0_c2, mpcore_cp15_c0_c2, 8 * sizeof(uint32_t));
         break;
     case ARM_CPUID_CORTEXA8:
-        memcpy(env->cp15.c0_c1, cortexa8_cp15_c0_c1, 8 * sizeof(uint32_t));
-        memcpy(env->cp15.c0_c2, cortexa8_cp15_c0_c2, 8 * sizeof(uint32_t));
         env->cp15.c0_clid = (1 << 27) | (2 << 24) | 3;
         env->cp15.c0_ccsid[0] = 0xe007e01a; /* 16k L1 dcache. */
         env->cp15.c0_ccsid[1] = 0x2007e01a; /* 16k L1 icache. */
         env->cp15.c0_ccsid[2] = 0xf0000000; /* No L2 icache. */
         break;
     case ARM_CPUID_CORTEXA9:
-        memcpy(env->cp15.c0_c1, cortexa9_cp15_c0_c1, 8 * sizeof(uint32_t));
-        memcpy(env->cp15.c0_c2, cortexa9_cp15_c0_c2, 8 * sizeof(uint32_t));
         env->cp15.c0_clid = (1 << 27) | (1 << 24) | 3;
         env->cp15.c0_ccsid[0] = 0xe00fe015; /* 16k L1 dcache. */
         env->cp15.c0_ccsid[1] = 0x200fe015; /* 16k L1 icache. */
         break;
     case ARM_CPUID_CORTEXA15:
-        memcpy(env->cp15.c0_c1, cortexa15_cp15_c0_c1, 8 * sizeof(uint32_t));
-        memcpy(env->cp15.c0_c2, cortexa15_cp15_c0_c2, 8 * sizeof(uint32_t));
         env->cp15.c0_clid = 0x0a200023;
         env->cp15.c0_ccsid[0] = 0x701fe00a; /* 32K L1 dcache */
         env->cp15.c0_ccsid[1] = 0x201fe00a; /* 32K L1 icache */
@@ -159,6 +100,20 @@  void cpu_state_reset(CPUARMState *env)
     env->vfp.xregs[ARM_VFP_MVFR1] = cpu->mvfr1;
     env->cp15.c0_cachetype = cpu->ctr;
     env->cp15.c1_sys = cpu->reset_sctlr;
+    env->cp15.c0_c1[0] = cpu->id_pfr0;
+    env->cp15.c0_c1[1] = cpu->id_pfr1;
+    env->cp15.c0_c1[2] = cpu->id_dfr0;
+    env->cp15.c0_c1[3] = cpu->id_afr0;
+    env->cp15.c0_c1[4] = cpu->id_mmfr0;
+    env->cp15.c0_c1[5] = cpu->id_mmfr1;
+    env->cp15.c0_c1[6] = cpu->id_mmfr2;
+    env->cp15.c0_c1[7] = cpu->id_mmfr3;
+    env->cp15.c0_c2[0] = cpu->id_isar0;
+    env->cp15.c0_c2[1] = cpu->id_isar1;
+    env->cp15.c0_c2[2] = cpu->id_isar2;
+    env->cp15.c0_c2[3] = cpu->id_isar3;
+    env->cp15.c0_c2[4] = cpu->id_isar4;
+    env->cp15.c0_c2[5] = cpu->id_isar5;
 
     if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
         env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';