| Message ID | 20201124134557.569388-1-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-5.2?] target/mips/translate: Check R6 reserved encoding for Load Linked Word | expand |
On 11/24/20 5:45 AM, Philippe Mathieu-Daudé wrote: > Release 6 recoded the 'Load Linked Word' using SPECIAL3 opcode, > this opcode (0b110000) is now reserved. > > Ref: A.2 Instruction Bit Encoding Tables: > > "6Rm instructions signal a Reserved Instruction exception > when executed by a Release 6 implementation." > > The check was added in commit 4368b29a26e ("target-mips: move > LL and SC instructions") but got lost during latter refactor > in commit d9224450208 ("target-mips: Tighten ISA level checks"). I think git blame is confused here -- d9224450208 isn't the one that broke things. The patch has: + case OPC_LL: /* Load and stores */ + check_insn(ctx, ISA_MIPS2); + /* Fallthrough */ + case OPC_LWL: case OPC_LWR: - case OPC_LL: check_insn_opc_removed(ctx, ISA_MIPS32R6); + /* Fallthrough */ Whereever it happened, it's broken now, so Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 11/24/20 4:59 PM, Richard Henderson wrote: > On 11/24/20 5:45 AM, Philippe Mathieu-Daudé wrote: >> Release 6 recoded the 'Load Linked Word' using SPECIAL3 opcode, >> this opcode (0b110000) is now reserved. >> >> Ref: A.2 Instruction Bit Encoding Tables: >> >> "6Rm instructions signal a Reserved Instruction exception >> when executed by a Release 6 implementation." >> >> The check was added in commit 4368b29a26e ("target-mips: move >> LL and SC instructions") but got lost during latter refactor >> in commit d9224450208 ("target-mips: Tighten ISA level checks"). > > I think git blame is confused here -- d9224450208 isn't the one that broke > things. The patch has: > > > + case OPC_LL: /* Load and stores */ > + check_insn(ctx, ISA_MIPS2); > + /* Fallthrough */ > + case OPC_LWL: > case OPC_LWR: > - case OPC_LL: > check_insn_opc_removed(ctx, ISA_MIPS32R6); > + /* Fallthrough */ Sorry I have been confused by the /* Fallthrough */ ... The check is below. Self-NAck then.
On Tue, Nov 24, 2020 at 5:15 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 11/24/20 4:59 PM, Richard Henderson wrote: > > On 11/24/20 5:45 AM, Philippe Mathieu-Daudé wrote: > >> Release 6 recoded the 'Load Linked Word' using SPECIAL3 opcode, > >> this opcode (0b110000) is now reserved. > >> > >> Ref: A.2 Instruction Bit Encoding Tables: > >> > >> "6Rm instructions signal a Reserved Instruction exception > >> when executed by a Release 6 implementation." > >> > >> The check was added in commit 4368b29a26e ("target-mips: move > >> LL and SC instructions") but got lost during latter refactor > >> in commit d9224450208 ("target-mips: Tighten ISA level checks"). > > > > I think git blame is confused here -- d9224450208 isn't the one that broke > > things. The patch has: > > > > > > + case OPC_LL: /* Load and stores */ > > + check_insn(ctx, ISA_MIPS2); > > + /* Fallthrough */ > > + case OPC_LWL: > > case OPC_LWR: > > - case OPC_LL: > > check_insn_opc_removed(ctx, ISA_MIPS32R6); > > + /* Fallthrough */ > > Sorry I have been confused by the /* Fallthrough */ ... > > The check is below. > > Self-NAck then. Duh I hit that again, read the patch again, looks correct. I guess I got confused myself reviewing the offending patch... So I'm applying this patch to mips-next queue, using Fixes: d9224450208 ("target-mips: Tighten ISA level checks") Thanks, Phil.
On Tue, 8 Dec 2020, Philippe Mathieu-Daudé wrote: > Duh I hit that again, read the patch again, looks correct. I guess > I got confused myself reviewing the offending patch... > So I'm applying this patch to mips-next queue, using > Fixes: d9224450208 ("target-mips: Tighten ISA level checks") What's wrong with current code? What I can see is: case OPC_LL: /* Load and stores */ check_insn(ctx, ISA_MIPS2); if (ctx->insn_flags & INSN_R5900) { check_insn_opc_user_only(ctx, INSN_R5900); } /* Fallthrough */ case OPC_LWL: case OPC_LWR: check_insn_opc_removed(ctx, ISA_MIPS32R6); /* Fallthrough */ case OPC_LB: case OPC_LH: case OPC_LW: case OPC_LWPC: case OPC_LBU: case OPC_LHU: gen_ld(ctx, op, rt, rs, imm); break; which looks absolutely right to me: LL is accepted with MIPS2--MIPS32R5 (including R5900 in user emulation only), LWL/LWR are accepted with MIPS1--MIPS32R5 and the remaining loads are accepted everywhere. What else do you need? Maciej
On 12/8/20 7:43 PM, Maciej W. Rozycki wrote: > On Tue, 8 Dec 2020, Philippe Mathieu-Daudé wrote: > >> Duh I hit that again, read the patch again, looks correct. I guess >> I got confused myself reviewing the offending patch... >> So I'm applying this patch to mips-next queue, using >> Fixes: d9224450208 ("target-mips: Tighten ISA level checks") > > What's wrong with current code? What I can see is: > > case OPC_LL: /* Load and stores */ > check_insn(ctx, ISA_MIPS2); > if (ctx->insn_flags & INSN_R5900) { > check_insn_opc_user_only(ctx, INSN_R5900); > } > /* Fallthrough */ > case OPC_LWL: > case OPC_LWR: > check_insn_opc_removed(ctx, ISA_MIPS32R6); > /* Fallthrough */ > case OPC_LB: > case OPC_LH: > case OPC_LW: > case OPC_LWPC: > case OPC_LBU: > case OPC_LHU: > gen_ld(ctx, op, rt, rs, imm); > break; > > which looks absolutely right to me: LL is accepted with MIPS2--MIPS32R5 > (including R5900 in user emulation only), LWL/LWR are accepted with > MIPS1--MIPS32R5 and the remaining loads are accepted everywhere. What > else do you need? I am clearly bleary-eyed... Sorry. Patch dropped.
diff --git a/target/mips/translate.c b/target/mips/translate.c index c64a1bc42e1..b1e7c674d3f 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -30993,6 +30993,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) break; case OPC_LL: /* Load and stores */ 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); }
Release 6 recoded the 'Load Linked Word' using SPECIAL3 opcode, this opcode (0b110000) is now reserved. Ref: A.2 Instruction Bit Encoding Tables: "6Rm instructions signal a Reserved Instruction exception when executed by a Release 6 implementation." The check was added in commit 4368b29a26e ("target-mips: move LL and SC instructions") but got lost during latter refactor in commit d9224450208 ("target-mips: Tighten ISA level checks"). Fixes: d9224450208 ("target-mips: Tighten ISA level checks") Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- target/mips/translate.c | 1 + 1 file changed, 1 insertion(+)