[Qemu-devel] [PATCH 0/2] implementation of UserLocal register for MIPS

[Qemu-devel] [PATCH 0/2] implementation of UserLocal register for MIPS

[Petar Jovanovic]

This is a small series of two patches. One expands the number of arguments
for gen_mfc0(), gen_mtc0(), gen_dmfc0(), and gen_dmtc0(), and the second
patch actually implements the register and access to it.

Regards,
Petar

Petar Jovanovic (2):
  target-mips: pass CPUMIPSState to gen_mfc0/mtc0/dmfc0/dmtc0
  target-mips: implement UserLocal Register

 target-mips/cpu.h       |    2 ++
 target-mips/helper.h    |    1 +
 target-mips/op_helper.c |   20 ++++++++++-
 target-mips/translate.c |   87 +++++++++++++++++++++++++++++++++++------------
 4 files changed, 87 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] target-mips: pass CPUMIPSState to gen_mfc0/mtc0/dmfc0/dmtc0

[Petar Jovanovic]

From: Petar Jovanovic <petar.jovanovic@imgtec.com>

Add CPUMIPSState in gen_mfc0/mtc0/dmfc0/dmtc0, as it will be used in the
next patch that implements UserLocal register and access to it.

Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
---
 target-mips/translate.c |   44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 05f82d2..d629b73 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4070,7 +4070,8 @@ static inline void gen_mtc0_store64 (TCGv arg, target_ulong off)
     tcg_gen_st_tl(arg, cpu_env, off);
 }
 
-static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel)
+static void gen_mfc0(CPUMIPSState *env, DisasContext *ctx,
+                     TCGv arg, int reg, int sel)
 {
     const char *rn = "invalid";
 
@@ -4657,7 +4658,8 @@ die:
     generate_exception(ctx, EXCP_RI);
 }
 
-static void gen_mtc0(DisasContext *ctx, TCGv arg, int reg, int sel)
+static void gen_mtc0(CPUMIPSState *env, DisasContext *ctx,
+                     TCGv arg, int reg, int sel)
 {
     const char *rn = "invalid";
 
@@ -5264,7 +5266,8 @@ die:
 }
 
 #if defined(TARGET_MIPS64)
-static void gen_dmfc0(DisasContext *ctx, TCGv arg, int reg, int sel)
+static void gen_dmfc0(CPUMIPSState *env, DisasContext *ctx,
+                      TCGv arg, int reg, int sel)
 {
     const char *rn = "invalid";
 
@@ -5833,7 +5836,8 @@ die:
     generate_exception(ctx, EXCP_RI);
 }
 
-static void gen_dmtc0(DisasContext *ctx, TCGv arg, int reg, int sel)
+static void gen_dmtc0(CPUMIPSState *env, DisasContext *ctx,
+                      TCGv arg, int reg, int sel)
 {
     const char *rn = "invalid";
 
@@ -6483,7 +6487,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext *ctx, int rt, int rd,
                 gen_helper_mftc0_tcschefback(t0, cpu_env);
                 break;
             default:
-                gen_mfc0(ctx, t0, rt, sel);
+                gen_mfc0(env, ctx, t0, rt, sel);
                 break;
             }
             break;
@@ -6493,7 +6497,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext *ctx, int rt, int rd,
                 gen_helper_mftc0_entryhi(t0, cpu_env);
                 break;
             default:
-                gen_mfc0(ctx, t0, rt, sel);
+                gen_mfc0(env, ctx, t0, rt, sel);
                 break;
             }
         case 12:
@@ -6502,7 +6506,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext *ctx, int rt, int rd,
                 gen_helper_mftc0_status(t0, cpu_env);
                 break;
             default:
-                gen_mfc0(ctx, t0, rt, sel);
+                gen_mfc0(env, ctx, t0, rt, sel);
                 break;
             }
         case 13:
@@ -6551,12 +6555,12 @@ static void gen_mftr(CPUMIPSState *env, DisasContext *ctx, int rt, int rd,
                 gen_helper_mftc0_debug(t0, cpu_env);
                 break;
             default:
-                gen_mfc0(ctx, t0, rt, sel);
+                gen_mfc0(env, ctx, t0, rt, sel);
                 break;
             }
             break;
         default:
-            gen_mfc0(ctx, t0, rt, sel);
+            gen_mfc0(env, ctx, t0, rt, sel);
         }
     } else switch (sel) {
     /* GPR registers. */
@@ -6701,7 +6705,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext *ctx, int rd, int rt,
                 gen_helper_mttc0_tcschefback(cpu_env, t0);
                 break;
             default:
-                gen_mtc0(ctx, t0, rd, sel);
+                gen_mtc0(env, ctx, t0, rd, sel);
                 break;
             }
             break;
@@ -6711,7 +6715,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext *ctx, int rd, int rt,
                 gen_helper_mttc0_entryhi(cpu_env, t0);
                 break;
             default:
-                gen_mtc0(ctx, t0, rd, sel);
+                gen_mtc0(env, ctx, t0, rd, sel);
                 break;
             }
         case 12:
@@ -6720,7 +6724,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext *ctx, int rd, int rt,
                 gen_helper_mttc0_status(cpu_env, t0);
                 break;
             default:
-                gen_mtc0(ctx, t0, rd, sel);
+                gen_mtc0(env, ctx, t0, rd, sel);
                 break;
             }
         case 13:
@@ -6749,12 +6753,12 @@ static void gen_mttr(CPUMIPSState *env, DisasContext *ctx, int rd, int rt,
                 gen_helper_mttc0_debug(cpu_env, t0);
                 break;
             default:
-                gen_mtc0(ctx, t0, rd, sel);
+                gen_mtc0(env, ctx, t0, rd, sel);
                 break;
             }
             break;
         default:
-            gen_mtc0(ctx, t0, rd, sel);
+            gen_mtc0(env, ctx, t0, rd, sel);
         }
     } else switch (sel) {
     /* GPR registers. */
@@ -6861,7 +6865,7 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
             /* Treat as NOP. */
             return;
         }
-        gen_mfc0(ctx, cpu_gpr[rt], rd, ctx->opcode & 0x7);
+        gen_mfc0(env, ctx, cpu_gpr[rt], rd, ctx->opcode & 0x7);
         opn = "mfc0";
         break;
     case OPC_MTC0:
@@ -6869,7 +6873,7 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
             TCGv t0 = tcg_temp_new();
 
             gen_load_gpr(t0, rt);
-            gen_mtc0(ctx, t0, rd, ctx->opcode & 0x7);
+            gen_mtc0(env, ctx, t0, rd, ctx->opcode & 0x7);
             tcg_temp_free(t0);
         }
         opn = "mtc0";
@@ -6881,7 +6885,7 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
             /* Treat as NOP. */
             return;
         }
-        gen_dmfc0(ctx, cpu_gpr[rt], rd, ctx->opcode & 0x7);
+        gen_dmfc0(env, ctx, cpu_gpr[rt], rd, ctx->opcode & 0x7);
         opn = "dmfc0";
         break;
     case OPC_DMTC0:
@@ -6890,7 +6894,7 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
             TCGv t0 = tcg_temp_new();
 
             gen_load_gpr(t0, rt);
-            gen_dmtc0(ctx, t0, rd, ctx->opcode & 0x7);
+            gen_dmtc0(env, ctx, t0, rd, ctx->opcode & 0x7);
             tcg_temp_free(t0);
         }
         opn = "dmtc0";
@@ -11068,7 +11072,7 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
             /* Treat as NOP. */
             break;
         }
-        gen_mfc0(ctx, cpu_gpr[rt], rs, (ctx->opcode >> 11) & 0x7);
+        gen_mfc0(env, ctx, cpu_gpr[rt], rs, (ctx->opcode >> 11) & 0x7);
         break;
     case MTC0:
     case MTC0 + 32:
@@ -11077,7 +11081,7 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
             TCGv t0 = tcg_temp_new();
 
             gen_load_gpr(t0, rt);
-            gen_mtc0(ctx, t0, rs, (ctx->opcode >> 11) & 0x7);
+            gen_mtc0(env, ctx, t0, rs, (ctx->opcode >> 11) & 0x7);
             tcg_temp_free(t0);
         }
         break;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/2] target-mips: pass CPUMIPSState to gen_mfc0/mtc0/dmfc0/dmtc0

[Richard Henderson]

On 05/16/2014 11:13 AM, Petar Jovanovic wrote:
>  
> -static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel)
> +static void gen_mfc0(CPUMIPSState *env, DisasContext *ctx,
> +    

You should not be passing around env within the translator, at all.

You should be able to get everything you need from ctx->hflags
or ctx->insn_flags.  If there's something missing from ctx, then
we'll talk about additional changes.


r~

[Qemu-devel] [PATCH 2/2] target-mips: implement UserLocal Register

[Petar Jovanovic]

From: Petar Jovanovic <petar.jovanovic@imgtec.com>

>From MIPS documentation (Volume III):

UserLocal Register (CP0 Register 4, Select 2)
Compliance Level: Recommended.

The UserLocal register is a read-write register that is not interpreted by
the hardware and conditionally readable via the RDHWR instruction.

This register only exists if the Config3-ULRI register field is set.

Privileged software may write this register with arbitrary information and
make it accessable to unprivileged software via register 29 (ULR) of the
RDHWR instruction. To do so, bit 29 of the HWREna register must be set to a
1 to enable unprivileged access to the register.

Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
---
 target-mips/cpu.h       |    2 ++
 target-mips/helper.h    |    1 +
 target-mips/op_helper.c |   20 +++++++++++++++++++-
 target-mips/translate.c |   43 +++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 6c2014e..bb18fb8 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -227,6 +227,7 @@ struct CPUMIPSState {
     target_ulong CP0_EntryLo0;
     target_ulong CP0_EntryLo1;
     target_ulong CP0_Context;
+    target_ulong CP0_UserLocal;
     int32_t CP0_PageMask;
     int32_t CP0_PageGrain;
     int32_t CP0_Wired;
@@ -361,6 +362,7 @@ struct CPUMIPSState {
     int32_t CP0_Config3;
 #define CP0C3_M    31
 #define CP0C3_ISA_ON_EXC 16
+#define CP0C3_ULRI 13
 #define CP0C3_DSPP 10
 #define CP0C3_LPA  7
 #define CP0C3_VEIC 6
diff --git a/target-mips/helper.h b/target-mips/helper.h
index 8c7921a..ec56199 100644
--- a/target-mips/helper.h
+++ b/target-mips/helper.h
@@ -296,6 +296,7 @@ DEF_HELPER_1(rdhwr_cpunum, tl, env)
 DEF_HELPER_1(rdhwr_synci_step, tl, env)
 DEF_HELPER_1(rdhwr_cc, tl, env)
 DEF_HELPER_1(rdhwr_ccres, tl, env)
+DEF_HELPER_1(rdhwr_ul, tl, env)
 DEF_HELPER_2(pmon, void, env, int)
 DEF_HELPER_1(wait, void, env)
 
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 4edec6c..72c0a25 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -1301,7 +1301,13 @@ void helper_mtc0_srsconf4(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong arg1)
 {
-    env->CP0_HWREna = arg1 & 0x0000000F;
+    uint32_t mask = 0x0000000F;
+
+    if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
+        mask |= 0x20000000;
+    }
+
+    env->CP0_HWREna = arg1 & mask;
 }
 
 void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1)
@@ -2091,6 +2097,18 @@ target_ulong helper_rdhwr_ccres(CPUMIPSState *env)
     return 0;
 }
 
+target_ulong helper_rdhwr_ul(CPUMIPSState *env)
+{
+    if ((env->hflags & MIPS_HFLAG_CP0) ||
+        (env->CP0_HWREna & (1 << 29))) {
+        return env->CP0_UserLocal;
+    } else {
+        helper_raise_exception(env, EXCP_RI);
+    }
+
+    return 0;
+}
+
 void helper_pmon(CPUMIPSState *env, int function)
 {
     function /= 2;
diff --git a/target-mips/translate.c b/target-mips/translate.c
index d629b73..792da07 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4216,7 +4216,17 @@ static void gen_mfc0(CPUMIPSState *env, DisasContext *ctx,
         case 1:
 //            gen_helper_mfc0_contextconfig(arg); /* SmartMIPS ASE */
             rn = "ContextConfig";
+            goto die;
 //            break;
+        case 2:
+            if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
+                tcg_gen_ld_tl(arg, cpu_env,
+                              offsetof(CPUMIPSState, CP0_UserLocal));
+                tcg_gen_ext32s_tl(arg, arg);
+                rn = "UserLocal";
+            } else {
+                tcg_gen_movi_tl(arg, 0);
+            }
         default:
             goto die;
         }
@@ -4804,7 +4814,15 @@ static void gen_mtc0(CPUMIPSState *env, DisasContext *ctx,
         case 1:
 //            gen_helper_mtc0_contextconfig(cpu_env, arg); /* SmartMIPS ASE */
             rn = "ContextConfig";
+            goto die;
 //            break;
+        case 2:
+            if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
+                tcg_gen_st_tl(arg, cpu_env,
+                              offsetof(CPUMIPSState, CP0_UserLocal));
+                rn = "UserLocal";
+            }
+            break;
         default:
             goto die;
         }
@@ -5409,7 +5427,17 @@ static void gen_dmfc0(CPUMIPSState *env, DisasContext *ctx,
         case 1:
 //            gen_helper_dmfc0_contextconfig(arg); /* SmartMIPS ASE */
             rn = "ContextConfig";
+            goto die;
 //            break;
+        case 2:
+            if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
+                tcg_gen_ld_tl(arg, cpu_env,
+                              offsetof(CPUMIPSState, CP0_UserLocal));
+                rn = "UserLocal";
+            } else {
+                tcg_gen_movi_tl(arg, 0);
+            }
+            break;
         default:
             goto die;
         }
@@ -5982,7 +6010,15 @@ static void gen_dmtc0(CPUMIPSState *env, DisasContext *ctx,
         case 1:
 //           gen_helper_mtc0_contextconfig(cpu_env, arg); /* SmartMIPS ASE */
             rn = "ContextConfig";
+            goto die;
 //           break;
+        case 2:
+            if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
+                tcg_gen_st_tl(arg, cpu_env,
+                              offsetof(CPUMIPSState, CP0_UserLocal));
+                rn = "UserLocal";
+            }
+            break;
         default:
             goto die;
         }
@@ -9068,8 +9104,10 @@ static void gen_rdhwr(DisasContext *ctx, int rt, int rd)
         gen_store_gpr(t0, rt);
         break;
 #else
-        /* XXX: Some CPUs implement this in hardware.
-           Not supported yet. */
+        save_cpu_state(ctx, 1);
+        gen_helper_rdhwr_ul(t0, cpu_env);
+        gen_store_gpr(t0, rt);
+        break;
 #endif
     default:            /* Invalid */
         MIPS_INVAL("rdhwr");
@@ -16034,6 +16072,7 @@ void cpu_state_reset(CPUMIPSState *env)
     /* vectored interrupts not implemented, timer on int 7,
        no performance counters. */
     env->CP0_IntCtl = 0xe0000000;
+    env->CP0_UserLocal = 0;
     {
         int i;
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/2] target-mips: implement UserLocal Register

[Richard Henderson]

On 05/16/2014 11:13 AM, Petar Jovanovic wrote:
> +target_ulong helper_rdhwr_ul(CPUMIPSState *env)
> +{
> +    if ((env->hflags & MIPS_HFLAG_CP0) ||
> +        (env->CP0_HWREna & (1 << 29))) {
> +        return env->CP0_UserLocal;
> +    } else {
> +        helper_raise_exception(env, EXCP_RI);
> +    }
> +
> +    return 0;
> +}
> +

You shouldn't need a helper at all.  We're supposed to check all of these
sorts of permissions at translation time, so you should be able to issue a
load or an exception directly from the translator.

> +            if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {

What is this check, and why isn't it present in ctx->hflags?

> +                tcg_gen_ld_tl(arg, cpu_env,
> +                              offsetof(CPUMIPSState, CP0_UserLocal));
> +                tcg_gen_ext32s_tl(arg, arg);

One operation: tcg_gen_ld32s_tl.

> #else
> -        /* XXX: Some CPUs implement this in hardware.
> -           Not supported yet. */
> +        save_cpu_state(ctx, 1);
> +        gen_helper_rdhwr_ul(t0, cpu_env);
> +        gen_store_gpr(t0, rt);
> +        break;
>  #endif
>  

This should be at least partially merged with the user-only tls_value code.


r~

Re: [Qemu-devel] [PATCH 2/2] target-mips: implement UserLocal Register

[James Hogan]

Hi Petar,

On Friday 16 May 2014 20:13:34 Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> From MIPS documentation (Volume III):
> 
> UserLocal Register (CP0 Register 4, Select 2)
> Compliance Level: Recommended.
> 
> The UserLocal register is a read-write register that is not interpreted by
> the hardware and conditionally readable via the RDHWR instruction.
> 
> This register only exists if the Config3-ULRI register field is set.
> 
> Privileged software may write this register with arbitrary information and
> make it accessable to unprivileged software via register 29 (ULR) of the
> RDHWR instruction. To do so, bit 29 of the HWREna register must be set to a
> 1 to enable unprivileged access to the register.
> 
> Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
> ---
>  target-mips/cpu.h       |    2 ++
>  target-mips/helper.h    |    1 +
>  target-mips/op_helper.c |   20 +++++++++++++++++++-
>  target-mips/translate.c |   43 +++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 63 insertions(+), 3 deletions(-)

I think you need to add it to cpu_save/cpu_load in target-mips/machine.c too, 
otherwise snapshotting and live migration may break.

Cheers
James

Re: [Qemu-devel] [PATCH 2/2] target-mips: implement UserLocal Register

[James Hogan]

On Friday 16 May 2014 20:13:34 Petar Jovanovic wrote:
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 6c2014e..bb18fb8 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -227,6 +227,7 @@ struct CPUMIPSState {
>      target_ulong CP0_EntryLo0;
>      target_ulong CP0_EntryLo1;
>      target_ulong CP0_Context;
> +    target_ulong CP0_UserLocal;

Also according to the MT ASE UserLocal is replicated for each TC, so this 
should really go in struct TCState I think.

Cheers
James