Patchwork MIPS/GDB: Correct stub handling of CP0 Status and Cause

login
register
mail settings
Submitter Maciej W. Rozycki
Date June 8, 2012, 1:07 a.m.
Message ID <alpine.DEB.1.10.1206080111310.23962@tp.orcam.me.uk>
Download mbox | patch
Permalink /patch/163710/
State New
Headers show

Comments

Maciej W. Rozycki - June 8, 2012, 1:07 a.m.
This change fixes the GDB stub such that write requests to the CP0 Status 
and Config registers:

1. Respect the r/w mask and do not change read-only bits.

2. Correctly execute any side effects, for example enable or disable 
   coprocessors, assert or clear software interrupts, etc.

In practice this change just abstracts code that used to live in 
helper_mtc0_status and mtc0_cause to cpu_mips_store_status and 
cpu_mips_store_cause respectively and wires the new functions to 
cpu_gdb_write_register, helper_mtc0_status, helper_mtc0_cause and 
helper_mttc0_cause as applicable.  To support that rearrangement shared 
code has been moved to a common header.

 I have verified this code manually to work correctly under GDB, I have 
checked register masks to work as expected, and the Status CU1 and FR bits 
to have the desired effects.

 I tried to trigger a software interrupt too by setting Cause to 0x300 and 
then Status to 0x201, and then making a few single steps, but that didn't 
cause the interrupt exception to be taken for some reason.  That does not 
appear to be a problem with my change though.  Perhaps there is a bug 
elsewhere.

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---

 According to checkpatch.pl this switch block in cpu_gdb_write_register 
should be rewritten, however I have refrained from doing that as a part of 
this change as it looks to me that the block is large enough to qualify 
for a separate change to do that.  Please apply.

  Maciej

qemu-mips-status.diff
Richard Henderson - June 12, 2012, 3:04 p.m.
On 2012-06-07 18:07, Maciej W. Rozycki wrote:
> +/* Called for updates to CP0_Status.  */
> +static inline void sync_c0_status(CPUMIPSState *env, int tc)

Any reason why these rather large functions are inline
rather than being in helper.c?

Otherwise, the change to gdbstub.c is most proper.


r~

Patch

Index: qemu-git-trunk/gdbstub.c
===================================================================
--- qemu-git-trunk.orig/gdbstub.c	2012-06-07 03:20:08.000000000 +0100
+++ qemu-git-trunk/gdbstub.c	2012-06-08 01:11:04.115609380 +0100
@@ -1135,11 +1135,11 @@  static int cpu_gdb_write_register(CPUMIP
         return sizeof(target_ulong);
     }
     switch (n) {
-    case 32: env->CP0_Status = tmp; break;
+    case 32: cpu_mips_store_status(env, tmp); break;
     case 33: env->active_tc.LO[0] = tmp; break;
     case 34: env->active_tc.HI[0] = tmp; break;
     case 35: env->CP0_BadVAddr = tmp; break;
-    case 36: env->CP0_Cause = tmp; break;
+    case 36: cpu_mips_store_cause(env, tmp); break;
     case 37:
         env->active_tc.PC = tmp & ~(target_ulong)1;
         if (tmp & 1) {
Index: qemu-git-trunk/target-mips/cpu.h
===================================================================
--- qemu-git-trunk.orig/target-mips/cpu.h	2012-06-07 21:16:21.000000000 +0100
+++ qemu-git-trunk/target-mips/cpu.h	2012-06-08 01:27:20.245637475 +0100
@@ -802,4 +802,79 @@  static inline void compute_hflags(CPUMIP
     }
 }
 
+/* Called for updates to CP0_Status.  */
+static inline void sync_c0_status(CPUMIPSState *env, int tc)
+{
+    int32_t tcstatus, *tcst;
+    uint32_t v = env->CP0_Status;
+    uint32_t cu, mx, asid, ksu;
+    uint32_t mask = ((1 << CP0TCSt_TCU3)
+                       | (1 << CP0TCSt_TCU2)
+                       | (1 << CP0TCSt_TCU1)
+                       | (1 << CP0TCSt_TCU0)
+                       | (1 << CP0TCSt_TMX)
+                       | (3 << CP0TCSt_TKSU)
+                       | (0xff << CP0TCSt_TASID));
+
+    cu = (v >> CP0St_CU0) & 0xf;
+    mx = (v >> CP0St_MX) & 0x1;
+    ksu = (v >> CP0St_KSU) & 0x3;
+    asid = env->CP0_EntryHi & 0xff;
+
+    tcstatus = cu << CP0TCSt_TCU0;
+    tcstatus |= mx << CP0TCSt_TMX;
+    tcstatus |= ksu << CP0TCSt_TKSU;
+    tcstatus |= asid;
+
+    if (tc == env->current_tc) {
+        tcst = &env->active_tc.CP0_TCStatus;
+    } else {
+        tcst = &env->tcs[tc].CP0_TCStatus;
+    }
+
+    *tcst &= ~mask;
+    *tcst |= tcstatus;
+    compute_hflags(env);
+}
+
+static inline void cpu_mips_store_status(CPUMIPSState *env, target_ulong val)
+{
+    uint32_t mask = env->CP0_Status_rw_bitmask;
+
+    env->CP0_Status = (env->CP0_Status & ~mask) | (val & mask);
+    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+        sync_c0_status(env, env->current_tc);
+    } else {
+        compute_hflags(env);
+    }
+}
+
+static inline void cpu_mips_store_cause(CPUMIPSState *env, target_ulong val)
+{
+    uint32_t mask = 0x00C00300;
+    uint32_t old = env->CP0_Cause;
+    int i;
+
+    if (env->insn_flags & ISA_MIPS32R2) {
+        mask |= 1 << CP0Ca_DC;
+    }
+
+    env->CP0_Cause = (env->CP0_Cause & ~mask) | (val & mask);
+
+    if ((old ^ env->CP0_Cause) & (1 << CP0Ca_DC)) {
+        if (env->CP0_Cause & (1 << CP0Ca_DC)) {
+            cpu_mips_stop_count(env);
+        } else {
+            cpu_mips_start_count(env);
+        }
+    }
+
+    /* Set/reset software interrupts */
+    for (i = 0 ; i < 2 ; i++) {
+        if ((old ^ env->CP0_Cause) & (1 << (CP0Ca_IP + i))) {
+            cpu_mips_soft_irq(env, i, env->CP0_Cause & (1 << (CP0Ca_IP + i)));
+        }
+    }
+}
+
 #endif /* !defined (__MIPS_CPU_H__) */
Index: qemu-git-trunk/target-mips/op_helper.c
===================================================================
--- qemu-git-trunk.orig/target-mips/op_helper.c	2012-06-07 21:16:21.000000000 +0100
+++ qemu-git-trunk/target-mips/op_helper.c	2012-06-08 01:11:04.115609380 +0100
@@ -766,40 +766,8 @@  static CPUMIPSState *mips_cpu_map_tc(int
 
    These helper call synchronizes the regs for a given cpu.  */
 
-/* Called for updates to CP0_Status.  */
-static void sync_c0_status(CPUMIPSState *cpu, int tc)
-{
-    int32_t tcstatus, *tcst;
-    uint32_t v = cpu->CP0_Status;
-    uint32_t cu, mx, asid, ksu;
-    uint32_t mask = ((1 << CP0TCSt_TCU3)
-                       | (1 << CP0TCSt_TCU2)
-                       | (1 << CP0TCSt_TCU1)
-                       | (1 << CP0TCSt_TCU0)
-                       | (1 << CP0TCSt_TMX)
-                       | (3 << CP0TCSt_TKSU)
-                       | (0xff << CP0TCSt_TASID));
-
-    cu = (v >> CP0St_CU0) & 0xf;
-    mx = (v >> CP0St_MX) & 0x1;
-    ksu = (v >> CP0St_KSU) & 0x3;
-    asid = env->CP0_EntryHi & 0xff;
-
-    tcstatus = cu << CP0TCSt_TCU0;
-    tcstatus |= mx << CP0TCSt_TMX;
-    tcstatus |= ksu << CP0TCSt_TKSU;
-    tcstatus |= asid;
-
-    if (tc == cpu->current_tc) {
-        tcst = &cpu->active_tc.CP0_TCStatus;
-    } else {
-        tcst = &cpu->tcs[tc].CP0_TCStatus;
-    }
-
-    *tcst &= ~mask;
-    *tcst |= tcstatus;
-    compute_hflags(cpu);
-}
+/* Called for updates to CP0_Status.  Defined in "cpu.h" for gdbstub.c.  */
+/* static inline void sync_c0_status(CPUMIPSState *cpu, int tc);  */
 
 /* Called for updates to CP0_TCStatus.  */
 static void sync_c0_tcstatus(CPUMIPSState *cpu, int tc, target_ulong v)
@@ -1500,16 +1468,10 @@  void helper_mtc0_compare (target_ulong a
 void helper_mtc0_status (target_ulong arg1)
 {
     uint32_t val, old;
-    uint32_t mask = env->CP0_Status_rw_bitmask;
 
-    val = arg1 & mask;
     old = env->CP0_Status;
-    env->CP0_Status = (env->CP0_Status & ~mask) | val;
-    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
-        sync_c0_status(env, env->current_tc);
-    } else {
-        compute_hflags(env);
-    }
+    cpu_mips_store_status(env, arg1);
+    val = env->CP0_Status;
 
     if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
         qemu_log("Status %08x (%08x) => %08x (%08x) Cause %08x",
@@ -1546,37 +1508,9 @@  void helper_mtc0_srsctl (target_ulong ar
     env->CP0_SRSCtl = (env->CP0_SRSCtl & ~mask) | (arg1 & mask);
 }
 
-static void mtc0_cause(CPUMIPSState *cpu, target_ulong arg1)
-{
-    uint32_t mask = 0x00C00300;
-    uint32_t old = cpu->CP0_Cause;
-    int i;
-
-    if (cpu->insn_flags & ISA_MIPS32R2) {
-        mask |= 1 << CP0Ca_DC;
-    }
-
-    cpu->CP0_Cause = (cpu->CP0_Cause & ~mask) | (arg1 & mask);
-
-    if ((old ^ cpu->CP0_Cause) & (1 << CP0Ca_DC)) {
-        if (cpu->CP0_Cause & (1 << CP0Ca_DC)) {
-            cpu_mips_stop_count(cpu);
-        } else {
-            cpu_mips_start_count(cpu);
-        }
-    }
-
-    /* Set/reset software interrupts */
-    for (i = 0 ; i < 2 ; i++) {
-        if ((old ^ cpu->CP0_Cause) & (1 << (CP0Ca_IP + i))) {
-            cpu_mips_soft_irq(cpu, i, cpu->CP0_Cause & (1 << (CP0Ca_IP + i)));
-        }
-    }
-}
-
 void helper_mtc0_cause(target_ulong arg1)
 {
-    mtc0_cause(env, arg1);
+    cpu_mips_store_cause(env, arg1);
 }
 
 void helper_mttc0_cause(target_ulong arg1)
@@ -1584,7 +1518,7 @@  void helper_mttc0_cause(target_ulong arg
     int other_tc = env->CP0_VPEControl & (0xff << CP0VPECo_TargTC);
     CPUMIPSState *other = mips_cpu_map_tc(&other_tc);
 
-    mtc0_cause(other, arg1);
+    cpu_mips_store_cause(other, arg1);
 }
 
 target_ulong helper_mftc0_epc(void)