diff mbox

[12/17] target-openrisc: Enable m[tf]spr from user mode

Message ID 1441239463-18981-13-git-send-email-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson Sept. 3, 2015, 12:17 a.m. UTC 
The architecture manual doesn't say these opcodes are user only.
Leaving them disabled excludes user mode from accessing interesting
SPRs like MACLO/MACHI.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-openrisc/helper.h     |  4 +--
 target-openrisc/sys_helper.c | 77 +++++++++++++++++---------------------------
 target-openrisc/translate.c  | 32 +++++++-----------
 3 files changed, 43 insertions(+), 70 deletions(-)

Comments

Bastian Koppelmann Sept. 5, 2015, 9:35 p.m. UTC | #1 
On 09/03/2015 02:17 AM, Richard Henderson wrote:
> -        if (dc->mem_idx == MMU_USER_IDX) {
> -            gen_illegal_exception(dc);
> -            return;
> +        {
> +            TCGv_i32 tmp = tcg_temp_new_i32();
> +            tcg_gen_trunc_tl_i32(tmp, cpu_R[ra]);
> +            tcg_gen_ori_i32(tmp, tmp, K16);
> +            gen_helper_mfspr(cpu_R[rd], cpu_env, tmp);
> +            tcg_temp_free_i32(tmp);
>           }
> -        t0 = tcg_const_i32(K16);
> -        gen_helper_mfspr(cpu_R[rd], cpu_env, cpu_R[rd], cpu_R[ra], t0);
> -        tcg_temp_free(t0);
> -#endif
>           break;

IIRC a lot of the registers are supervisor only, e.g. VR, NPC or SR and 
the manual is fairly clear about that. User mode cpu ought not to read 
these registers unconditionally.

>   
>       case 0x30:    /* l.mtspr */
>           LOG_DIS("l.mtspr r%d, r%d, %d\n", ra, rb, K5_11);
> -#if defined(CONFIG_USER_ONLY)
> -        return;
> -#else
> -        if (dc->mem_idx == MMU_USER_IDX) {
> -            gen_illegal_exception(dc);
> -            return;
> +        {
> +            TCGv_i32 tmp = tcg_temp_new_i32();
> +            tcg_gen_trunc_tl_i32(tmp, cpu_R[ra]);
> +            tcg_gen_ori_i32(tmp, tmp, K5_11);
> +            gen_helper_mtspr(cpu_env, tmp, cpu_R[rb]);
> +            tcg_temp_free_i32(tmp);
>           }
> -        t0 = tcg_const_tl(K5_11);
> -        gen_helper_mtspr(cpu_env, cpu_R[ra], cpu_R[rb], t0);
> -        tcg_temp_free(t0);
> -#endif
>           break;

Same as above, unconditional write.

Cheers,
Bastian
Richard Henderson Sept. 6, 2015, 8:36 p.m. UTC | #2 
On Sep 5, 2015 14:35, Bastian Koppelmann <kbastian@mail.uni-paderborn.de> wrote:
> IIRC a lot of the registers are supervisor only, e.g. VR, NPC or SR and 
> the manual is fairly clear about that. User mode cpu ought not to read 
> these registers unconditionally. 

When I last discussed this on the openrisc list, back in March, there was no real specification for user mode, and what bits are or should be accessible.

Looking at 
  http://opencores.org/or1k/Architecture_Specification
today, that still seems to be the case.

In the meantime, dropping the privilege check makes linux-user GCC tests work better.

r~
Bastian Koppelmann Sept. 13, 2015, 8:34 a.m. UTC | #3 
On 09/06/2015 10:36 PM, Richard Henderson wrote:
> On Sep 5, 2015 14:35, Bastian Koppelmann <kbastian@mail.uni-paderborn.de> wrote:
>> IIRC a lot of the registers are supervisor only, e.g. VR, NPC or SR and
>> the manual is fairly clear about that. User mode cpu ought not to read
>> these registers unconditionally.
> When I last discussed this on the openrisc list, back in March, there was no real specification for user mode, and what bits are or should be accessible.
>
> Looking at
>    http://opencores.org/or1k/Architecture_Specification
> today, that still seems to be the case.
>
> In the meantime, dropping the privilege check makes linux-user GCC tests work better.
>
Looking at the article, user mode seems to be optional, so I'm not 
against it, but it does look weird. How does ork1sim do it?

Cheers,
Bastian
Richard Henderson Sept. 14, 2015, 5:11 p.m. UTC | #4 
On 09/13/2015 01:34 AM, Bastian Koppelmann wrote:
> Looking at the article, user mode seems to be optional, so I'm not against it,
> but it does look weird. How does ork1sim do it?

It's haphazard.  There are checks for supervisor in the l_mtspr and l_mfspr
insns, but not other uses of the sprs, such as l_rfe.

In the meantime, testing of the multiply-accumulate instructions is hindered by
the fact that linux-user can't read the MACHI and MACLO special registers.


r~
Bastian Koppelmann Sept. 15, 2015, 7:22 a.m. UTC | #5 
On 09/14/2015 07:11 PM, Richard Henderson wrote:
> On 09/13/2015 01:34 AM, Bastian Koppelmann wrote:
>> Looking at the article, user mode seems to be optional, so I'm not against it,
>> but it does look weird. How does ork1sim do it?
> It's haphazard.  There are checks for supervisor in the l_mtspr and l_mfspr
> insns, but not other uses of the sprs, such as l_rfe.
How about we make the optional user mode a feature bit? This way we are 
closer to ork1sim, but don't have the problem that testing is hindered.

Cheers,
Bastian
diff mbox

Patch

diff --git a/target-openrisc/helper.h b/target-openrisc/helper.h
index 4b0a935..ef243cd 100644
--- a/target-openrisc/helper.h
+++ b/target-openrisc/helper.h
@@ -64,5 +64,5 @@  DEF_HELPER_FLAGS_1(fl1, TCG_CALL_NO_RWG_SE, tl, tl)
 DEF_HELPER_FLAGS_1(rfe, 0, void, env)
 
 /* sys */
-DEF_HELPER_FLAGS_4(mtspr, 0, void, env, tl, tl, tl)
-DEF_HELPER_FLAGS_4(mfspr, TCG_CALL_NO_WG, tl, env, tl, tl, tl)
+DEF_HELPER_FLAGS_3(mtspr, 0, void, env, i32, tl)
+DEF_HELPER_FLAGS_2(mfspr, TCG_CALL_NO_WG, tl, env, i32)
diff --git a/target-openrisc/sys_helper.c b/target-openrisc/sys_helper.c
index 9f95a97..19309a2 100644
--- a/target-openrisc/sys_helper.c
+++ b/target-openrisc/sys_helper.c
@@ -23,12 +23,11 @@ 
 
 #define TO_SPR(group, number) (((group) << 11) + (number))
 
-void HELPER(mtspr)(CPUOpenRISCState *env,
-                   target_ulong ra, target_ulong rb, target_ulong offset)
+void HELPER(mtspr)(CPUOpenRISCState *env, uint32_t spr, target_ulong rb)
 {
 #ifndef CONFIG_USER_ONLY
-    int spr = (ra | offset);
     int idx;
+#endif
 
     OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
     CPUState *cs = CPU(cpu);
@@ -48,6 +47,7 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
             tlb_flush(cs, 1);
         }
         cpu_set_sr(env, rb);
+#ifndef CONFIG_USER_ONLY
         if (env->sr & SR_DME) {
             env->tlb->cpu_openrisc_map_address_data =
                 &cpu_openrisc_get_phys_data;
@@ -63,6 +63,7 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
             env->tlb->cpu_openrisc_map_address_code =
                 &cpu_openrisc_get_phys_nommu;
         }
+#endif
         break;
 
     case TO_SPR(0, 18): /* PPC */
@@ -80,6 +81,15 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
     case TO_SPR(0, 64): /* ESR */
         env->esr = rb;
         break;
+
+    case TO_SPR(5, 1):  /* MACLO */
+        env->mac = deposit64(env->mac, 0, 32, rb);
+        break;
+    case TO_SPR(5, 2):  /* MACHI */
+        env->mac = deposit64(env->mac, 32, 32, rb);
+        break;
+
+#ifndef CONFIG_USER_ONLY
     case TO_SPR(1, 512) ... TO_SPR(1, 512+DTLB_SIZE-1): /* DTLBW0MR 0-127 */
         idx = spr - TO_SPR(1, 512);
         if (!(rb & 1)) {
@@ -87,7 +97,6 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
         }
         env->tlb->dtlb[0][idx].mr = rb;
         break;
-
     case TO_SPR(1, 640) ... TO_SPR(1, 640+DTLB_SIZE-1): /* DTLBW0TR 0-127 */
         idx = spr - TO_SPR(1, 640);
         env->tlb->dtlb[0][idx].tr = rb;
@@ -106,7 +115,6 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
         }
         env->tlb->itlb[0][idx].mr = rb;
         break;
-
     case TO_SPR(2, 640) ... TO_SPR(2, 640+ITLB_SIZE-1): /* ITLBW0TR 0-127 */
         idx = spr - TO_SPR(2, 640);
         env->tlb->itlb[0][idx].tr = rb;
@@ -118,12 +126,7 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
     case TO_SPR(2, 1280) ... TO_SPR(2, 1407): /* ITLBW3MR 0-127 */
     case TO_SPR(2, 1408) ... TO_SPR(2, 1535): /* ITLBW3TR 0-127 */
         break;
-    case TO_SPR(5, 1):  /* MACLO */
-        env->mac = deposit64(env->mac, 0, 32, rb);
-        break;
-    case TO_SPR(5, 2):  /* MACHI */
-        env->mac = deposit64(env->mac, 32, 32, rb);
-        break;
+
     case TO_SPR(9, 0):  /* PICMR */
         env->picmr |= rb;
         break;
@@ -167,21 +170,19 @@  void HELPER(mtspr)(CPUOpenRISCState *env,
         }
         cpu_openrisc_timer_update(cpu);
         break;
-    default:
+#endif
 
+    default:
         break;
     }
-#endif
 }
 
-target_ulong HELPER(mfspr)(CPUOpenRISCState *env,
-                           target_ulong rd, target_ulong ra, uint32_t offset)
+target_ulong HELPER(mfspr)(CPUOpenRISCState *env, uint32_t spr)
 {
 #ifndef CONFIG_USER_ONLY
-    int spr = (ra | offset);
-    int idx;
-
     OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
+    int idx;
+#endif
 
     switch (spr) {
     case TO_SPR(0, 0): /* VR */
@@ -217,6 +218,14 @@  target_ulong HELPER(mfspr)(CPUOpenRISCState *env,
     case TO_SPR(0, 64): /* ESR */
         return env->esr;
 
+    case TO_SPR(5, 1):  /* MACLO */
+        return (uint32_t)env->mac;
+        break;
+    case TO_SPR(5, 2):  /* MACHI */
+        return env->mac >> 32;
+        break;
+
+#ifndef CONFIG_USER_ONLY
     case TO_SPR(1, 512) ... TO_SPR(1, 512+DTLB_SIZE-1): /* DTLBW0MR 0-127 */
         idx = spr - TO_SPR(1, 512);
         return env->tlb->dtlb[0][idx].mr;
@@ -249,13 +258,6 @@  target_ulong HELPER(mfspr)(CPUOpenRISCState *env,
     case TO_SPR(2, 1408) ... TO_SPR(2, 1535): /* ITLBW3TR 0-127 */
         break;
 
-    case TO_SPR(5, 1):  /* MACLO */
-        return (uint32_t)env->mac;
-        break;
-    case TO_SPR(5, 2):  /* MACHI */
-        return env->mac >> 32;
-        break;
-
     case TO_SPR(9, 0):  /* PICMR */
         return env->picmr;
 
@@ -268,31 +270,10 @@  target_ulong HELPER(mfspr)(CPUOpenRISCState *env,
     case TO_SPR(10, 1): /* TTCR */
         cpu_openrisc_count_update(cpu);
         return env->ttcr;
+#endif
 
     default:
         break;
     }
-#endif
-
-/*If we later need to add tracepoints (or debug printfs) for the return
-value, it may be useful to structure the code like this:
-
-target_ulong ret = 0;
-
-switch() {
-case x:
- ret = y;
- break;
-case z:
- ret = 42;
- break;
-...
-}
-
-later something like trace_spr_read(ret);
-
-return ret;*/
-
-    /* for rd is passed in, if rd unchanged, just keep it back.  */
-    return rd;
+    return 0;
 }
diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
index 83f4ea6..09c7af6 100644
--- a/target-openrisc/translate.c
+++ b/target-openrisc/translate.c
@@ -738,32 +738,24 @@  static void dec_misc(DisasContext *dc, uint32_t insn)
 
     case 0x2d:    /* l.mfspr */
         LOG_DIS("l.mfspr r%d, r%d, %d\n", rd, ra, K16);
-#if defined(CONFIG_USER_ONLY)
-        return;
-#else
-        if (dc->mem_idx == MMU_USER_IDX) {
-            gen_illegal_exception(dc);
-            return;
+        {
+            TCGv_i32 tmp = tcg_temp_new_i32();
+            tcg_gen_trunc_tl_i32(tmp, cpu_R[ra]);
+            tcg_gen_ori_i32(tmp, tmp, K16);
+            gen_helper_mfspr(cpu_R[rd], cpu_env, tmp);
+            tcg_temp_free_i32(tmp);
         }
-        t0 = tcg_const_i32(K16);
-        gen_helper_mfspr(cpu_R[rd], cpu_env, cpu_R[rd], cpu_R[ra], t0);
-        tcg_temp_free(t0);
-#endif
         break;
 
     case 0x30:    /* l.mtspr */
         LOG_DIS("l.mtspr r%d, r%d, %d\n", ra, rb, K5_11);
-#if defined(CONFIG_USER_ONLY)
-        return;
-#else
-        if (dc->mem_idx == MMU_USER_IDX) {
-            gen_illegal_exception(dc);
-            return;
+        {
+            TCGv_i32 tmp = tcg_temp_new_i32();
+            tcg_gen_trunc_tl_i32(tmp, cpu_R[ra]);
+            tcg_gen_ori_i32(tmp, tmp, K5_11);
+            gen_helper_mtspr(cpu_env, tmp, cpu_R[rb]);
+            tcg_temp_free_i32(tmp);
         }
-        t0 = tcg_const_tl(K5_11);
-        gen_helper_mtspr(cpu_env, cpu_R[ra], cpu_R[rb], t0);
-        tcg_temp_free(t0);
-#endif
         break;
 
     case 0x34:    /* l.sd */