[Qemu-devel] [PATCH] mips: Correct the writes to CP0 Status and Cause registers via gdbstub

[Maciej W. Rozycki]

Make writes to CP0.Status and CP0.Cause have the same effect as
executing corresponding MTC0 instructions would in Kernel Mode.  Also
ignore writes in the user emulation mode.

Currently for requests from the GDB stub we write all the bits across
both registers, ignoring any read-only locations, and do not synchronise
the environment to evaluate side effects.  We also write these registers
in the user emulation mode even though a real kernel presents them as
read only.

Signed-off-by: Maciej W. Rozycki <address@hidden>
---
Hi,

 I have verified the correct operation of this patch in the system 
emulation mode, with the 24Kf processor selected, by:

1. Booting Linux and running full glibc and GDB test suites within it, 
   the latter with gdbserver run locally and GDB connecting to it over 
   TCP from the host running QEMU.  This should have executed enough 
   kernel paths to ensure the semantics of the two registers hasn't 
   changed as seen by code emulated.

2. Connecting with GDB through the GDB stub, downloading a simple 
   bare-iron app linked to a KSEG0 address and single-stepping through 
   it; then poking at CP0.Status to switch to User Mode (KSU set to 
   0b10, ERL & EXL cleared) and single-stepping further, reaching the 
   generic exception handler right away due the an AdEL exception 
   (PC set to 0x80000184, CP0.Status.EXL set and CP0.Cause.ExcCode set 
   to 0x04) triggered with an instruction fetch from KSEG0 that is not 
   allowed in User Mode.  With the change reverted single-stepping 
   continues as if nothing happened.

The change for the user emulation mode is obvious.

 While testing this change I noticed that memory accesses made through 
the GDB stub are incorrectly validated against the current execution 
mode set in CP0.Status.  This is obviously incorrect, privilege checks 
should be suppressed as if the GDB stub accessed memory from Debug Mode.  
This is a separate bug to fix though.

 It would be great if QEMU stopped at the first exception handler 
instruction too, that is at 0x80000180 in this case, as the change in 
the execution flow is not necessarily expected or the effects easily 
determined and it would be good to see the new state before the first 
exception handler instruction has been executed; this is what real JTAG 
hardware probes actually do, at least certainly on the MIPS target and 
IIRC on Freescale Power and Intel x86 too.

 Please apply.

  Maciej

qemu-mips-status.diff
Index: qemu-git-trunk/target-mips/cpu.h
===================================================================
--- qemu-git-trunk.orig/target-mips/cpu.h       2014-11-09 23:44:32.000000000 
+0000
+++ qemu-git-trunk/target-mips/cpu.h    2014-11-09 23:49:54.997846651 +0000
@@ -904,4 +904,93 @@ static inline void compute_hflags(CPUMIP
     }
 }
 
+#ifndef CONFIG_USER_ONLY
+/* Called for updates to CP0_Status.  */
+static inline void sync_c0_status(CPUMIPSState *env, 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);
+}
+
+static inline void cpu_mips_store_status(CPUMIPSState *env, target_ulong val)
+{
+    uint32_t mask = env->CP0_Status_rw_bitmask;
+
+    if (env->insn_flags & ISA_MIPS32R6) {
+        bool has_supervisor = extract32(mask, CP0St_KSU, 2) == 0x3;
+
+        if (has_supervisor && extract32(val, CP0St_KSU, 2) == 0x3) {
+            mask &= ~(3 << CP0St_KSU);
+        }
+        mask &= ~(((1 << CP0St_SR) | (1 << CP0St_NMI)) & val);
+    }
+
+    env->CP0_Status = (env->CP0_Status & ~mask) | (val & mask);
+    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+        sync_c0_status(env, 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;
+    }
+    if (env->insn_flags & ISA_MIPS32R6) {
+        mask &= ~((1 << CP0Ca_WP) & val);
+    }
+
+    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
+
 #endif /* !defined (__MIPS_CPU_H__) */
Index: qemu-git-trunk/target-mips/gdbstub.c
===================================================================
--- qemu-git-trunk.orig/target-mips/gdbstub.c   2014-11-09 23:44:32.000000000 
+0000
+++ qemu-git-trunk/target-mips/gdbstub.c        2014-11-09 23:49:54.997846651 
+0000
@@ -112,7 +112,9 @@ int mips_cpu_gdb_write_register(CPUState
     }
     switch (n) {
     case 32:
-        env->CP0_Status = tmp;
+#ifndef CONFIG_USER_ONLY
+        cpu_mips_store_status(env, tmp);
+#endif
         break;
     case 33:
         env->active_tc.LO[0] = tmp;
@@ -124,7 +126,9 @@ int mips_cpu_gdb_write_register(CPUState
         env->CP0_BadVAddr = tmp;
         break;
     case 36:
-        env->CP0_Cause = tmp;
+#ifndef CONFIG_USER_ONLY
+        cpu_mips_store_cause(env, tmp);
+#endif
         break;
     case 37:
         env->active_tc.PC = tmp & ~(target_ulong)1;
Index: qemu-git-trunk/target-mips/op_helper.c
===================================================================
--- qemu-git-trunk.orig/target-mips/op_helper.c 2014-11-09 23:45:27.000000000 
+0000
+++ qemu-git-trunk/target-mips/op_helper.c      2014-11-09 23:49:54.997846651 
+0000
@@ -625,40 +625,9 @@ static CPUMIPSState *mips_cpu_map_tc(CPU
 
    These helper call synchronizes the regs for a given cpu.  */
 
-/* Called for updates to CP0_Status.  */
-static void sync_c0_status(CPUMIPSState *env, 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 *env, CPUMIPSState *cpu,
+                                     int tc);  */
 
 /* Called for updates to CP0_TCStatus.  */
 static void sync_c0_tcstatus(CPUMIPSState *cpu, int tc,
@@ -1420,25 +1389,10 @@ void helper_mtc0_status(CPUMIPSState *en
 {
     MIPSCPU *cpu = mips_env_get_cpu(env);
     uint32_t val, old;
-    uint32_t mask = env->CP0_Status_rw_bitmask;
-
-    if (env->insn_flags & ISA_MIPS32R6) {
-        bool has_supervisor = extract32(mask, CP0St_KSU, 2) == 0x3;
 
-        if (has_supervisor && extract32(arg1, CP0St_KSU, 2) == 0x3) {
-            mask &= ~(3 << CP0St_KSU);
-        }
-        mask &= ~(((1 << CP0St_SR) | (1 << CP0St_NMI)) & arg1);
-    }
-
-    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, 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",
@@ -1477,40 +1431,9 @@ void helper_mtc0_srsctl(CPUMIPSState *en
     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;
-    }
-    if (cpu->insn_flags & ISA_MIPS32R6) {
-        mask &= ~((1 << CP0Ca_WP) & arg1);
-    }
-
-    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(CPUMIPSState *env, target_ulong arg1)
 {
-    mtc0_cause(env, arg1);
+    cpu_mips_store_cause(env, arg1);
 }
 
 void helper_mttc0_cause(CPUMIPSState *env, target_ulong arg1)
@@ -1518,7 +1441,7 @@ void helper_mttc0_cause(CPUMIPSState *en
     int other_tc = env->CP0_VPEControl & (0xff << CP0VPECo_TargTC);
     CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc);
 
-    mtc0_cause(other, arg1);
+    cpu_mips_store_cause(other, arg1);
 }
 
 target_ulong helper_mftc0_epc(CPUMIPSState *env)