diff mbox

[RFC] target-arm: Preserve CPUID over CPU reset

Message ID 1319281250-27114-1-git-send-email-andreas.faerber@web.de
State New
Headers show

Commit Message

Andreas Färber Oct. 22, 2011, 11 a.m. UTC 
Previously the CPUID register was set in cpu_arm_init() based on -cpu
model. The CPU was then reset, requiring to save the CPUID and restore
it afterwards.

Change the storage location of c0_cpuid so that it does not get cleared.

OMAP appears to be special in that the CPUID can be switched between
TI915T and TI925T on write. Therefore preserve cpu_reset_model_id() in
slightly simplified form, allowing OMAP to reset the CPUID to TI925.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Paul Brook <paul@codesourcery.com>
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/armv7m_nvic.c     |    2 +-
 target-arm/cpu.h     |    4 ++--
 target-arm/helper.c  |   24 ++++++++++--------------
 target-arm/machine.c |    4 ++--
 4 files changed, 15 insertions(+), 19 deletions(-)

Comments

Andreas Färber Nov. 10, 2011, 10:31 a.m. UTC | #1 
Hello Peter,

Here's a rebased version of my ARM feature inference series. It's based on
my CPUID preservation patch, which hasn't received any comments yet.

As demonstrated by the two trailing patches, this simplifies adding a new
cpu model significantly.

These inference rules are based on your comments for Cortex-R4F and by
analyzing the existing CPUs, so be careful to check that for whether there
are any exceptions to the rule.

Regards,

Andreas


Andreas Färber (5):
  target-arm: Infer ARMv4T feature
  target-arm: Infer ARMv5 feature
  target-arm: Infer ARMv6 feature
  target-arm: Prepare support for Cortex-R4
  target-arm: Add support for Cortex-R4F

 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   79 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 57 insertions(+), 23 deletions(-)
Peter Maydell Nov. 10, 2011, 1:25 p.m. UTC | #2 
On 10 November 2011 10:31, Andreas Färber <andreas.faerber@web.de> wrote:
> Here's a rebased version of my ARM feature inference series. It's based on
> my CPUID preservation patch, which hasn't received any comments yet.

Oops. I'll go back and dig that out of the archive.

> These inference rules are based on your comments for Cortex-R4F and by
> analyzing the existing CPUs, so be careful to check that for whether there
> are any exceptions to the rule.

The inference rules look good. Some others you can do:
  V7 implies V6K  (this turns on V6K for M3 but that's safe because V6K
        only affects code which isn't reachable for M profile cores)
  V6K implies V6
  V7 implies THUMB2
  VFP4 implies VFP3
  VFP3 implies VFP
  M implies THUMB_DIV
  V6 implies AUXCR

-- PMM
Peter Maydell Nov. 10, 2011, 3:03 p.m. UTC | #3 
On 22 October 2011 12:00, Andreas Färber <andreas.faerber@web.de> wrote:
> Previously the CPUID register was set in cpu_arm_init() based on -cpu
> model. The CPU was then reset, requiring to save the CPUID and restore
> it afterwards.
>
> Change the storage location of c0_cpuid so that it does not get cleared.

I think this is the wrong approach. c0_cpuid should not be handled
any differently from any of the other read-only cp15 registers...

> OMAP appears to be special in that the CPUID can be switched between
> TI915T and TI925T on write. Therefore preserve cpu_reset_model_id() in
> slightly simplified form, allowing OMAP to reset the CPUID to TI925.

...and indeed it's not even purely r/o on those cores.

I don't think we should be doing things based on c0_cpuid. At the
moment we use it for:
 * determining whether we should implement the MPIDR (v7 or 11mpcore)
    -- we should define and use a feature bit instead
 * identifying what to return as the ACTLR value
    -- we should handle this like the other implementation-specific
       cp15 registers instead
 * code in cpu_reset_model_id which sets feature bits and various
   default register values
    -- see below

For cp15 registers I think we need to move to something more like
how target-ppc handles SPRs -- a registration routine of some
kind so each implementation can provide the cp15 registers it
requires and we don't get tangled up in huge nested switch statements
or splitting of "is this register present/ro/whatever?" between
translate.c and helper.c. If we do that then obviously "reset value"
is one of the things you feed in when you register the register,
and we only do the registration once from cpu_init, not at reset.

So cpu_arm_init should pick a base model (defining mostly a set
of cp15 regs and feature bits) and the user can also tweak the
feature bits (cf target-i386 +/-foo syntax), and reset no longer
needs to care at all what "cpu model" the user specified.

-- PMM
diff mbox

Patch

diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
index bf8c3c5..e029537 100644
--- a/hw/armv7m_nvic.c
+++ b/hw/armv7m_nvic.c
@@ -151,7 +151,7 @@  static uint32_t nvic_readl(void *opaque, uint32_t offset)
     case 0x1c: /* SysTick Calibration Value.  */
         return 10000;
     case 0xd00: /* CPUID Base.  */
-        return cpu_single_env->cp15.c0_cpuid;
+        return ARM_CPUID(cpu_single_env);
     case 0xd04: /* Interrypt Control State.  */
         /* VECTACTIVE */
         val = s->gic.running_irq[0];
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 6ab780d..1623a2a 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -106,7 +106,6 @@  typedef struct CPUARMState {
 
     /* System control coprocessor (cp15) */
     struct {
-        uint32_t c0_cpuid;
         uint32_t c0_cachetype;
         uint32_t c0_ccsid[16]; /* Cache size.  */
         uint32_t c0_clid; /* Cache level.  */
@@ -228,6 +227,7 @@  typedef struct CPUARMState {
     } cp[15];
     void *nvic;
     const struct arm_boot_info *boot_info;
+    uint32_t cp15_c0_cpuid;
 } CPUARMState;
 
 CPUARMState *cpu_arm_init(const char *cpu_model);
@@ -398,7 +398,7 @@  void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
    conventional cores (ie. Application or Realtime profile).  */
 
 #define IS_M(env) arm_feature(env, ARM_FEATURE_M)
-#define ARM_CPUID(env) (env->cp15.c0_cpuid)
+#define ARM_CPUID(env) (env->cp15_c0_cpuid)
 
 #define ARM_CPUID_ARM1026     0x4106a262
 #define ARM_CPUID_ARM926      0x41069265
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e2428eb..74e423d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -48,10 +48,9 @@  static inline void set_feature(CPUARMState *env, int feature)
     env->features |= 1u << feature;
 }
 
-static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
+static void cpu_reset_model_id(CPUARMState *env)
 {
-    env->cp15.c0_cpuid = id;
-    switch (id) {
+    switch (env->cp15_c0_cpuid) {
     case ARM_CPUID_ARM926:
         set_feature(env, ARM_FEATURE_V4T);
         set_feature(env, ARM_FEATURE_V5);
@@ -214,7 +213,7 @@  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
     case ARM_CPUID_TI925T:
         set_feature(env, ARM_FEATURE_V4T);
         set_feature(env, ARM_FEATURE_OMAPCP);
-        env->cp15.c0_cpuid = ARM_CPUID_TI925T; /* Depends on wiring.  */
+        env->cp15_c0_cpuid = ARM_CPUID_TI925T; /* Depends on wiring.  */
         env->cp15.c0_cachetype = 0x5109149;
         env->cp15.c1_sys = 0x00000070;
         env->cp15.c15_i_max = 0x000;
@@ -253,7 +252,7 @@  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
         env->cp15.c1_sys = 0x00000070;
         break;
     default:
-        cpu_abort(env, "Bad CPU ID: %x\n", id);
+        cpu_abort(env, "Bad CPU ID: %x\n", env->cp15_c0_cpuid);
         break;
     }
 
@@ -265,17 +264,14 @@  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 
 void cpu_reset(CPUARMState *env)
 {
-    uint32_t id;
-
     if (qemu_loglevel_mask(CPU_LOG_RESET)) {
         qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
         log_cpu_state(env, 0);
     }
 
-    id = env->cp15.c0_cpuid;
     memset(env, 0, offsetof(CPUARMState, breakpoints));
-    if (id)
-        cpu_reset_model_id(env, id);
+    if (env->cp15_c0_cpuid)
+        cpu_reset_model_id(env);
 #if defined (CONFIG_USER_ONLY)
     env->uncached_cpsr = ARM_CPU_MODE_USR;
     /* For user mode we must enable access to coprocessors */
@@ -311,7 +307,7 @@  void cpu_reset(CPUARMState *env)
     /* v7 performance monitor control register: same implementor
      * field as main ID register, and we implement no event counters.
      */
-    env->cp15.c9_pmcr = (id & 0xff000000);
+    env->cp15.c9_pmcr = (env->cp15_c0_cpuid & 0xff000000);
 #endif
     set_flush_to_zero(1, &env->vfp.standard_fp_status);
     set_flush_inputs_to_zero(1, &env->vfp.standard_fp_status);
@@ -392,7 +388,7 @@  CPUARMState *cpu_arm_init(const char *cpu_model)
     }
 
     env->cpu_model_str = cpu_model;
-    env->cp15.c0_cpuid = id;
+    env->cp15_c0_cpuid = id;
     cpu_reset(env);
     if (arm_feature(env, ARM_FEATURE_NEON)) {
         gdb_register_coprocessor(env, vfp_gdb_get_reg, vfp_gdb_set_reg,
@@ -1756,7 +1752,7 @@  void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val)
                 break;
             case 1: /* Set TI925T configuration.  */
                 env->cp15.c15_ticonfig = val & 0xe7;
-                env->cp15.c0_cpuid = (val & (1 << 5)) ? /* OS_TYPE bit */
+                env->cp15_c0_cpuid = (val & (1 << 5)) ? /* OS_TYPE bit */
                         ARM_CPUID_TI915T : ARM_CPUID_TI925T;
                 break;
             case 2: /* Set I_max.  */
@@ -1801,7 +1797,7 @@  uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
             case 0:
                 switch (op2) {
                 case 0: /* Device ID.  */
-                    return env->cp15.c0_cpuid;
+                    return env->cp15_c0_cpuid;
                 case 1: /* Cache Type.  */
 		    return env->cp15.c0_cachetype;
                 case 2: /* TCM status.  */
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 7d4fc54..ed457a5 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -20,7 +20,7 @@  void cpu_save(QEMUFile *f, void *opaque)
         qemu_put_be32(f, env->usr_regs[i]);
         qemu_put_be32(f, env->fiq_regs[i]);
     }
-    qemu_put_be32(f, env->cp15.c0_cpuid);
+    qemu_put_be32(f, env->cp15_c0_cpuid);
     qemu_put_be32(f, env->cp15.c0_cachetype);
     qemu_put_be32(f, env->cp15.c0_cssel);
     qemu_put_be32(f, env->cp15.c1_sys);
@@ -134,7 +134,7 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
         env->usr_regs[i] = qemu_get_be32(f);
         env->fiq_regs[i] = qemu_get_be32(f);
     }
-    env->cp15.c0_cpuid = qemu_get_be32(f);
+    env->cp15_c0_cpuid = qemu_get_be32(f);
     env->cp15.c0_cachetype = qemu_get_be32(f);
     env->cp15.c0_cssel = qemu_get_be32(f);
     env->cp15.c1_sys = qemu_get_be32(f);