[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R59
From: |
Aleksandar Markovic |
Subject: |
Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 |
Date: |
Mon, 5 Nov 2018 18:58:44 +0000 |
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
- Re: [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1, (continued)
- [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Fredrik Noring, 2018/11/02
- Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Philippe Mathieu-Daudé, 2018/11/02
- Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Richard Henderson, 2018/11/04
- Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Fredrik Noring, 2018/11/04
- Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Maciej W. Rozycki, 2018/11/04
- Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Fredrik Noring, 2018/11/05
- Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1, Maciej W. Rozycki, 2018/11/05
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, 2018/11/05