From: Aleksandar Rikalo <arikalo@wavecomp.com>

Use bits from configuration registers for availability control
of MT ASE instructions, rather than only ISA_MT bit in insn_flags.
This is done by adding a field in hflags for MT bit, and adding
functions check_mt() and check_cp0_mt().

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 target/mips/cpu.h       |  3 ++-
 target/mips/internal.h  |  6 +++++-
 target/mips/translate.c | 45 +++++++++++++++++++++++++++++++++++++--------
 3 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 468b686..bf9c634 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -556,7 +556,7 @@ struct CPUMIPSState {
 #define EXCP_INST_NOTAVAIL 0x2 /* No valid instruction word for BadInstr */
     uint32_t hflags;    /* CPU State */
     /* TMASK defines different execution modes */
-#define MIPS_HFLAG_TMASK  0x3F5807FF
+#define MIPS_HFLAG_TMASK  0x7F5807FF
 #define MIPS_HFLAG_MODE   0x00007 /* execution modes                    */
     /* The KSU flags must be the lowest bits in hflags. The flag order
        must be the same as defined for CP0 Status. This allows to use
@@ -608,6 +608,7 @@ struct CPUMIPSState {
 #define MIPS_HFLAG_ITC_CACHE  0x8000000 /* CACHE instr. operates on ITC tag */
 #define MIPS_HFLAG_ERL   0x10000000 /* error level flag */
 #define MIPS_HFLAG_XNP   0x20000000
+#define MIPS_HFLAG_MT    0x40000000
     target_ulong btarget;        /* Jump / branch target               */
     target_ulong bcond;          /* Branch condition (if needed)       */
 
diff --git a/target/mips/internal.h b/target/mips/internal.h
index 97485da..c0e447b 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -308,7 +308,8 @@ static inline void compute_hflags(CPUMIPSState *env)
                      MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU |
                      MIPS_HFLAG_AWRAP | MIPS_HFLAG_DSP | MIPS_HFLAG_DSPR2 |
                      MIPS_HFLAG_SBRI | MIPS_HFLAG_MSA | MIPS_HFLAG_FRE |
-                     MIPS_HFLAG_ELPA | MIPS_HFLAG_ERL | MIPS_HFLAG_XNP);
+                     MIPS_HFLAG_ELPA | MIPS_HFLAG_ERL | MIPS_HFLAG_XNP |
+                     MIPS_HFLAG_MT);
     if (env->CP0_Status & (1 << CP0St_ERL)) {
         env->hflags |= MIPS_HFLAG_ERL;
     }
@@ -405,6 +406,9 @@ static inline void compute_hflags(CPUMIPSState *env)
     if (env->CP0_Config5 & (1 << CP0C5_XNP)) {
         env->hflags |= MIPS_HFLAG_XNP;
     }
+    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+        env->hflags |= MIPS_HFLAG_MT;
+    }
 }
 
 void cpu_mips_tlb_flush(CPUMIPSState *env);
diff --git a/target/mips/translate.c b/target/mips/translate.c
index dcb3d25..e8c0509 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1925,6 +1925,35 @@ static inline void check_xnp(DisasContext *ctx)
     }
 }
 
+/*
+ * This code generates a "reserved instruction" exception if the
+ * Config5 MT bit is NOT set.
+ */
+static inline void check_mt(DisasContext *ctx)
+{
+    if (unlikely(!(ctx->hflags & MIPS_HFLAG_MT))) {
+        generate_exception_end(ctx, EXCP_RI);
+    }
+}
+
+/*
+ * This code generates a "coprocessor unusable" exception if CP0 is not
+ * available, and, if that is not the case, generates a "reserved instruction"
+ * exception if the Config5 MT bit is NOT set. This is needed for availability
+ * control of some of MT ASE instructions.
+ */
+static inline void check_cp0_mt(DisasContext *ctx)
+{
+    if (unlikely(!(ctx->hflags & MIPS_HFLAG_CP0))) {
+        generate_exception_err(ctx, EXCP_CpU, 0);
+    } else {
+        if (unlikely(!(ctx->hflags & MIPS_HFLAG_MT))) {
+            generate_exception_err(ctx, EXCP_RI, 0);
+        }
+    }
+}
+
+
 
 /* Define small wrappers for gen_load_fpr* so that we have a uniform
    calling interface for 32 and 64-bit FPRs.  No sense in changing
@@ -8471,7 +8500,7 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
         opn = "mthc0";
         break;
     case OPC_MFTR:
-        check_insn(ctx, ASE_MT);
+        check_cp0_enabled(ctx);
         if (rd == 0) {
             /* Treat as NOP. */
             return;
@@ -8481,7 +8510,7 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
         opn = "mftr";
         break;
     case OPC_MTTR:
-        check_insn(ctx, ASE_MT);
+        check_cp0_enabled(ctx);
         gen_mttr(env, ctx, rd, rt, (ctx->opcode >> 5) & 1,
                  ctx->opcode & 0x7, (ctx->opcode >> 4) & 1);
         opn = "mttr";
@@ -21797,7 +21826,7 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
         gen_rdhwr(ctx, rt, rd, extract32(ctx->opcode, 6, 3));
         break;
     case OPC_FORK:
-        check_insn(ctx, ASE_MT);
+        check_mt(ctx);
         {
             TCGv t0 = tcg_temp_new();
             TCGv t1 = tcg_temp_new();
@@ -21810,7 +21839,7 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
         }
         break;
     case OPC_YIELD:
-        check_insn(ctx, ASE_MT);
+        check_mt(ctx);
         {
             TCGv t0 = tcg_temp_new();
 
@@ -23107,22 +23136,22 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
                 op2 = MASK_MFMC0(ctx->opcode);
                 switch (op2) {
                 case OPC_DMT:
-                    check_insn(ctx, ASE_MT);
+                    check_cp0_mt(ctx);
                     gen_helper_dmt(t0);
                     gen_store_gpr(t0, rt);
                     break;
                 case OPC_EMT:
-                    check_insn(ctx, ASE_MT);
+                    check_cp0_mt(ctx);
                     gen_helper_emt(t0);
                     gen_store_gpr(t0, rt);
                     break;
                 case OPC_DVPE:
-                    check_insn(ctx, ASE_MT);
+                    check_cp0_mt(ctx);
                     gen_helper_dvpe(t0, cpu_env);
                     gen_store_gpr(t0, rt);
                     break;
                 case OPC_EVPE:
-                    check_insn(ctx, ASE_MT);
+                    check_cp0_mt(ctx);
                     gen_helper_evpe(t0, cpu_env);
                     gen_store_gpr(t0, rt);
                     break;
-- 
2.7.4

On 08/16/2018 07:57 AM, Aleksandar Markovic wrote:
> From: Aleksandar Rikalo <arikalo@wavecomp.com>
> 
> Use bits from configuration registers for availability control
> of MT ASE instructions, rather than only ISA_MT bit in insn_flags.
> This is done by adding a field in hflags for MT bit, and adding
> functions check_mt() and check_cp0_mt().
> 
> Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---
>  target/mips/cpu.h       |  3 ++-
>  target/mips/internal.h  |  6 +++++-
>  target/mips/translate.c | 45 +++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 44 insertions(+), 10 deletions(-)

What was wrong with using insn_flags?

I'll note that hflags should be reserved for things that can change at runtime.
 I thought all of these configuration registers were read-only.

Anyway, with this plus the XNP patch from earlier, you now only have one
remaining bit within hflags and then that resource is exhausted.


r~

> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, August 16, 2018 6:37 PM
> 
> Subject: Re: [PATCH v9 40/84] target/mips: Fix pre-nanoMIPS MT ASE instructions availability control
> 
> On 08/16/2018 07:57 AM, Aleksandar Markovic wrote:
> > From: Aleksandar Rikalo <arikalo@wavecomp.com>
> >
> > Use bits from configuration registers for availability control
> > of MT ASE instructions, rather than only ISA_MT bit in insn_flags.
> > This is done by adding a field in hflags for MT bit, and adding
> > functions check_mt() and check_cp0_mt().
> >
> > Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> > Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> > Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> > ---
> >  target/mips/cpu.h       |  3 ++-
> >  target/mips/internal.h  |  6 +++++-
> >  target/mips/translate.c | 45 +++++++++++++++++++++++++++++++++++++--------
> >  3 files changed, 44 insertions(+), 10 deletions(-)
> 
> What was wrong with using insn_flags?
> 

The problem with using ISA_MT from insn_flags is that it doesn't provide the correct availability control. Even though all instructions from MT ASE are related, they are available under different conditions:

Case 1 - DMT, DVPE, EMT, EVPE:
    
    if IsCoprocessorEnabled(0) then
        if Config3 MT then
            <main fuctionality>
        else
            SignalException(ReservedInstruction)
        endif
    else
        SignalException(CoprocessorUnusable, 0)
    endif

Case 2 - FORK, YIELD:

    if Config3 MT then
        <main fuctionality>
    else
        SignalException(ReservedInstruction)
    endif

Case 3 - MFTR, MTTR:

    if IsCoprocessorEnabled(0) then
        <main fuctionality>
    else
        SignalException(CoprocessorUnusable, 0)
    endif

> I'll note that hflags should be reserved for things that can change at runtime.
>  I thought all of these configuration registers were read-only.
> 
> Anyway, with this plus the XNP patch from earlier, you now only have one
> remaining bit within hflags and then that resource is exhausted.
> 

I think some of the previously-implemented similar cases involving read-only bits were handled the same way, and we just built on that. What would you suggest as a more appropriate solution in such cases (of accessing "preset by hardware" bits)?

Regards,
Aleksandar

On 08/16/2018 10:06 AM, Aleksandar Markovic wrote:
> I think some of the previously-implemented similar cases involving read-only bits were handled the same way, and we just built on that. What would you suggest as a more appropriate solution in such cases (of accessing "preset by hardware" bits)?

Well, ctx->insn_flags and ctx->CP0_Config1 are good examples.
These are 100% read-only and fixed at cpu instantiation.

I see that CP0_Config3 has one writable bit for micromips, but
is fully readonly for nanomips.  Therefore XNP and MT need not
be copied to hflags because they will never vary.

I'd suggest copying CP0_Config3 to ctx as with Config1.


r~

> > I think some of the previously-implemented similar cases involving read-only bits were handled the same way, and we just built on that. What would you suggest as a more appropriate solution in such cases (of accessing "preset by hardware" bits)?
>
> Well, ctx->insn_flags and ctx->CP0_Config1 are good examples.
> These are 100% read-only and fixed at cpu instantiation.
> 
> I see that CP0_Config3 has one writable bit for micromips, but
> is fully readonly for nanomips.  Therefore XNP and MT need not
> be copied to hflags because they will never vary.
> 
> I'd suggest copying CP0_Config3 to ctx as with Config1.
>
>
> r~

Hi, Richard,

The opinion within the team is that we should leave such changes for follow-up clean-up - clean-up of CP0-related functionalities is scheduled anyway soon.

The reason is that the current implementation (in v9) works fine, and this is very late in our dev cycle to change features with no observed bugs.

All other your concerns will be addressed in v10, which is planned to be sent soon.

Yours,
Aleksandar

> > I think some of the previously-implemented similar cases involving read-only bits were handled the same way, and we just built on that. What would you suggest as a more appropriate solution in such cases (of accessing "preset by hardware" bits)?
>
> Well, ctx->insn_flags and ctx->CP0_Config1 are good examples.
> These are 100% read-only and fixed at cpu instantiation.
>
> I see that CP0_Config3 has one writable bit for micromips, but
> is fully readonly for nanomips.  Therefore XNP and MT need not
> be copied to hflags because they will never vary.
>
> I'd suggest copying CP0_Config3 to ctx as with Config1.
>
>
> r~

Hi, Richard,

We ended up implementing this feature the way you suggested in the v11. Sorry about snafu.

Regards,
Aleksandar