Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

[Aleksandar Markovic]

Hello, Fredrik.

I appreciate your response and efforts!

>
> From: Fredrik Noring <address@hidden>
>
> Subject: Re: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 
> M{F,T}{HI,LO}1 and DIV[> U]1
>
> Thank you for your review, Aleksandar,
>
> > For LL, SC, LLD and SCD instructions, there is a need to properly insulate
> > their R5900 versions too, similar to this:
> >
> >     case OPC_SC:
> >         if(ctx->insn_flags & INSN_R5900) {
> >              check_insn_opc_user_only(ctx, INSN_R5900);
> >         } else {
> >             check_insn(ctx, ISA_MIPS2);
> >         }
> >         gen_st_cond(ctx, op, rt, rs, imm);
> >         break;
>
> Would you accept the simplification to omit the else clause? Like this:
>
>     case OPC_SC:
>         if (ctx->insn_flags & INSN_R5900) {
>             check_insn_opc_user_only(ctx, INSN_R5900);
>         }
>         check_insn(ctx, ISA_MIPS2);
>         check_insn_opc_removed(ctx, ISA_MIPS32R6);
>         gen_st_cond(ctx, op, rt, rs, imm);
>         break;
>

I think the following code would be even better:

    case OPC_SC:
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

> The code will, of course, expand into a double-check of INSN_R5900:
>
>         if (ctx->insn_flags & INSN_R5900) {
> #ifndef CONFIG_USER_ONLY
>             if (unlikely(ctx->insn_flags & INSN_R5900)) {
>                 generate_exception_end(ctx, EXCP_RI);
>             }
> #endif
>         }
>

I don't mind. Later on, we can drop the second argument of 
check_insn_opc_user_only() altogether, and the code would expand to the minimal 
and clear:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            generate_exception_end(ctx, EXCP_RI);
#endif
        }

but at this moment this is not a source of concern to me at all.

> > (the code above is just a form of pseudocode illustrating the idea; I
> > don't guarantee the correctness for build purposes, or if this is the best
> > code organization)
> >
> > Non-R5900 code (for the time being) should never invoke
> > check_insn_opc_user_only(). *The only way* of distinguishing R5900 code
> > paths from the other CPUs code paths should be by using
> > "if(ctx->insn_flags & INSN_R5900)"!
>
> OK.
>
> > For changes in decode_opc_special_legacy(), there shouldn't be there, but
> > there should be a separate function decode_opc_special_tx59() or so.
>
> Sure, I will copy the 82 line function then...
>

No, no, you don't need to copy 82 lines. First, you can assume that you are 
already in INSN_R5900 case - no need for frequent check_insn()s. Further, you 
can omit everything that is not needed for R5900 (for example, entire OPC_MOVCI 
case).

> ..., and patch the following:
>
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -23904,7 +23904,7 @@ static void decode_opc_special_legacy(CPUMIPSState 
> *env, DisasContext > *ctx)
>      case OPC_MOVN:         /* Conditional move */
>      case OPC_MOVZ:
>          check_insn(ctx, ISA_MIPS4 | ISA_MIPS32 |
> -                   INSN_LOONGSON2E | INSN_LOONGSON2F | INSN_R5900);
> +                   INSN_LOONGSON2E | INSN_LOONGSON2F);
>          gen_cond_move(ctx, op1, rd, rs, rt);
>          break;
>      case OPC_MFHI:          /* Move from HI/LO */
> @@ -23931,8 +23931,6 @@ static void decode_opc_special_legacy(CPUMIPSState 
> *env, DisasContext > *ctx)
>              check_insn(ctx, INSN_VR54XX);
>              op1 = MASK_MUL_VR54XX(ctx->opcode);
>              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> -        } else if (ctx->insn_flags & INSN_R5900) {
> -            gen_mul_txx9(ctx, op1, rd, rs, rt);
>          } else {
>              gen_muldiv(ctx, op1, rd & 3, rs, rt);
>          }
> @@ -23947,7 +23945,6 @@ static void decode_opc_special_legacy(CPUMIPSState 
> *env, DisasContext > *ctx)
>      case OPC_DDIV:
>      case OPC_DDIVU:
>          check_insn(ctx, ISA_MIPS3);
> -        check_insn_opc_user_only(ctx, INSN_R5900);
>          check_mips_64(ctx);
>          gen_muldiv(ctx, op1, 0, rs, rt);
>          break;
>

Exactly!

> Fredrik
>

Aleksandar

________________________________________
From: Fredrik Noring <address@hidden>
Sent: Monday, November 5, 2018 7:12:42 PM
To: Aleksandar Markovic
Cc: Aurelien Jarno; Philippe Mathieu-Daudé; Jürgen Urban; Maciej W. Rozycki; 
address@hidden
Subject: Re: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 
M{F,T}{HI,LO}1 and DIV[U]1

Thank you for your review, Aleksandar,

> For LL, SC, LLD and SCD instructions, there is a need to properly insulate
> their R5900 versions too, similar to this:
>
>     case OPC_SC:
>         if(ctx->insn_flags & INSN_R5900) {
>              check_insn_opc_user_only(ctx, INSN_R5900);
>         } else {
>             check_insn(ctx, ISA_MIPS2);
>         }
>         gen_st_cond(ctx, op, rt, rs, imm);
>         break;

Would you accept the simplification to omit the else clause? Like this:

    case OPC_SC:
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

The code will, of course, expand into a double-check of INSN_R5900:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            if (unlikely(ctx->insn_flags & INSN_R5900)) {
                generate_exception_end(ctx, EXCP_RI);
            }
#endif
        }

> (the code above is just a form of pseudocode illustrating the idea; I
> don't guarantee the correctness for build purposes, or if this is the best
> code organization)
>
> Non-R5900 code (for the time being) should never invoke
> check_insn_opc_user_only(). *The only way* of distinguishing R5900 code
> paths from the other CPUs code paths should be by using
> "if(ctx->insn_flags & INSN_R5900)"!

OK.

> For changes in decode_opc_special_legacy(), there shouldn't be there, but
> there should be a separate function decode_opc_special_tx59() or so.

Sure, I will copy the 82 line function then, and patch the following:

--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -23904,7 +23904,7 @@ static void decode_opc_special_legacy(CPUMIPSState 
*env, DisasContext *ctx)
     case OPC_MOVN:         /* Conditional move */
     case OPC_MOVZ:
         check_insn(ctx, ISA_MIPS4 | ISA_MIPS32 |
-                   INSN_LOONGSON2E | INSN_LOONGSON2F | INSN_R5900);
+                   INSN_LOONGSON2E | INSN_LOONGSON2F);
         gen_cond_move(ctx, op1, rd, rs, rt);
         break;
     case OPC_MFHI:          /* Move from HI/LO */
@@ -23931,8 +23931,6 @@ static void decode_opc_special_legacy(CPUMIPSState 
*env, DisasContext *ctx)
             check_insn(ctx, INSN_VR54XX);
             op1 = MASK_MUL_VR54XX(ctx->opcode);
             gen_mul_vr54xx(ctx, op1, rd, rs, rt);
-        } else if (ctx->insn_flags & INSN_R5900) {
-            gen_mul_txx9(ctx, op1, rd, rs, rt);
         } else {
             gen_muldiv(ctx, op1, rd & 3, rs, rt);
         }
@@ -23947,7 +23945,6 @@ static void decode_opc_special_legacy(CPUMIPSState 
*env, DisasContext *ctx)
     case OPC_DDIV:
     case OPC_DDIVU:
         check_insn(ctx, ISA_MIPS3);
-        check_insn_opc_user_only(ctx, INSN_R5900);
         check_mips_64(ctx);
         gen_muldiv(ctx, op1, 0, rs, rt);
         break;

Fredrik