| Message ID | ce19dd9e9f0a9d6a7d4d1d758d4cf76fd7d9a723.1537122775.git.noring@nocrew.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/mips: Support R5900 GCC programs in user mode | expand |
On 9/16/18 5:13 PM, Fredrik Noring wrote: > These MIPS III instructions are unavailable and therefore trapped and > emulated by the Linux kernel. > > Signed-off-by: Fredrik Noring <noring@nocrew.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/mips/translate.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 77d678353e..327e96307b 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -22458,6 +22458,7 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx) > case OPC_DMULTU: > case OPC_DDIV: > case OPC_DDIVU: > + check_insn_opc_user_only(ctx, INSN_R5900); > check_insn(ctx, ISA_MIPS3); > check_mips_64(ctx); > gen_muldiv(ctx, op1, 0, rs, rt); > @@ -24962,6 +24963,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > break; > case OPC_LL: /* Load and stores */ > check_insn(ctx, ISA_MIPS2); > + check_insn_opc_user_only(ctx, INSN_R5900); > /* Fallthrough */ > case OPC_LWL: > case OPC_LWR: > @@ -24987,6 +24989,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > case OPC_SC: > check_insn(ctx, ISA_MIPS2); > check_insn_opc_removed(ctx, ISA_MIPS32R6); > + check_insn_opc_user_only(ctx, INSN_R5900); > gen_st_cond(ctx, op, rt, rs, imm); > break; > case OPC_CACHE: > @@ -25253,9 +25256,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > > #if defined(TARGET_MIPS64) > /* MIPS64 opcodes */ > + case OPC_LLD: > + check_insn_opc_user_only(ctx, INSN_R5900); > + /* fall through */ > case OPC_LDL: > case OPC_LDR: > - case OPC_LLD: > check_insn_opc_removed(ctx, ISA_MIPS32R6); > /* fall through */ > case OPC_LWU: > @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > break; > case OPC_SCD: > check_insn_opc_removed(ctx, ISA_MIPS32R6); > + check_insn_opc_user_only(ctx, INSN_R5900); > check_insn(ctx, ISA_MIPS3); > check_mips_64(ctx); > gen_st_cond(ctx, op, rt, rs, imm); >
Hi Fredrik, Nitpicking here, but I think it's what makes code clean and pleasant to read. On Sun, 16 Sep 2018, Fredrik Noring wrote: > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 77d678353e..327e96307b 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -22458,6 +22458,7 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx) > case OPC_DMULTU: > case OPC_DDIV: > case OPC_DDIVU: > + check_insn_opc_user_only(ctx, INSN_R5900); > check_insn(ctx, ISA_MIPS3); > check_mips_64(ctx); > gen_muldiv(ctx, op1, 0, rs, rt); I think it would make sense to keep the order of the checks consistent, or otherwise code starts looking messy. The predominant order appears to be (as applicable) starting with a check for the lowest ISA membership, followed by a check for the highest ISA membership (R6 instruction cuts) and ending with processor-specific special cases. I think this order makes sense in that it starts with checks that have a wider scope and then moves on to ones of a narrower scope, and I think keeping it would be good (as would fixing where the addition of R6 broke it). Mode checks for otherwise existing instructions then follow, which complements the pattern. So please make it: check_insn(ctx, ISA_MIPS3); check_insn_opc_user_only(ctx, INSN_R5900); check_mips_64(ctx); > @@ -24962,6 +24963,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > break; > case OPC_LL: /* Load and stores */ > check_insn(ctx, ISA_MIPS2); > + check_insn_opc_user_only(ctx, INSN_R5900); > /* Fallthrough */ > case OPC_LWL: > case OPC_LWR: > @@ -24987,6 +24989,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > case OPC_SC: > check_insn(ctx, ISA_MIPS2); > check_insn_opc_removed(ctx, ISA_MIPS32R6); > + check_insn_opc_user_only(ctx, INSN_R5900); > gen_st_cond(ctx, op, rt, rs, imm); > break; > case OPC_CACHE: OK in these two cases (noting a preexisting formatting issue to fix separately). > @@ -25253,9 +25256,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > > #if defined(TARGET_MIPS64) > /* MIPS64 opcodes */ > + case OPC_LLD: > + check_insn_opc_user_only(ctx, INSN_R5900); > + /* fall through */ > case OPC_LDL: > case OPC_LDR: > - case OPC_LLD: > check_insn_opc_removed(ctx, ISA_MIPS32R6); > /* fall through */ > case OPC_LWU: And here, because of the fall-through. > @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > break; > case OPC_SCD: > check_insn_opc_removed(ctx, ISA_MIPS32R6); > + check_insn_opc_user_only(ctx, INSN_R5900); > check_insn(ctx, ISA_MIPS3); > check_mips_64(ctx); > gen_st_cond(ctx, op, rt, rs, imm); And please make it: check_insn_opc_removed(ctx, ISA_MIPS32R6); check_insn(ctx, ISA_MIPS3); check_insn_opc_user_only(ctx, INSN_R5900); check_mips_64(ctx); here (and swapping the two former calls ought to be fixed separately; I haven't checked if there are more cases like this, but if so, then they would best be amended with a single change). Maciej
Hi Maciej, Philippe -- thank you for your reviews, On Mon, Sep 17, 2018 at 06:10:27PM +0100, Maciej W. Rozycki wrote: > Nitpicking here, but I think it's what makes code clean and pleasant to > read. I agree, that is important too. I will post an updated v5 soon. Another alternative change is to define check_insn_opc_user_only as static inline void check_insn_opc_user_only(DisasContext *ctx, int flags) { #ifndef CONFIG_USER_ONLY check_insn_opc_removed(ctx, flags); #endif } by referring to check_insn_opc_removed (instead of copying its definition). Would you consider this an improvement for v5 too? > I think it would make sense to keep the order of the checks consistent, > or otherwise code starts looking messy. > > The predominant order appears to be (as applicable) starting with a > check for the lowest ISA membership, followed by a check for the highest > ISA membership (R6 instruction cuts) and ending with processor-specific > special cases. I think this order makes sense in that it starts with > checks that have a wider scope and then moves on to ones of a narrower > scope, and I think keeping it would be good (as would fixing where the > addition of R6 broke it). Mode checks for otherwise existing > instructions then follow, which complements the pattern. Sure, thanks for clarifying the ordering rules! > So please make it: > > check_insn(ctx, ISA_MIPS3); > check_insn_opc_user_only(ctx, INSN_R5900); > check_mips_64(ctx); Done. > > @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) > > break; > > case OPC_SCD: > > check_insn_opc_removed(ctx, ISA_MIPS32R6); > > + check_insn_opc_user_only(ctx, INSN_R5900); > > check_insn(ctx, ISA_MIPS3); > > check_mips_64(ctx); > > gen_st_cond(ctx, op, rt, rs, imm); > > And please make it: > > check_insn_opc_removed(ctx, ISA_MIPS32R6); > check_insn(ctx, ISA_MIPS3); > check_insn_opc_user_only(ctx, INSN_R5900); > check_mips_64(ctx); Done. > here (and swapping the two former calls ought to be fixed separately; I > haven't checked if there are more cases like this, but if so, then they > would best be amended with a single change). I'll defer other ordering and indentation fixes since I'm not sure whether such changes would be accepted. Fredrik
Hi Fredrik, > I agree, that is important too. I will post an updated v5 soon. Another > alternative change is to define check_insn_opc_user_only as > > static inline void check_insn_opc_user_only(DisasContext *ctx, int flags) > { > #ifndef CONFIG_USER_ONLY > check_insn_opc_removed(ctx, flags); > #endif > } > > by referring to check_insn_opc_removed (instead of copying its definition). > Would you consider this an improvement for v5 too? Yes, it does look like an improvement to me, reducing code duplication. Thanks for looking into it further. > > here (and swapping the two former calls ought to be fixed separately; I > > haven't checked if there are more cases like this, but if so, then they > > would best be amended with a single change). > > I'll defer other ordering and indentation fixes since I'm not sure whether > such changes would be accepted. Sure, no need for you to rush doing that. In the absence of someone willing to do such clean-ups voluntarily I would consider it a maintainer's duty really. Maciej
On 9/18/18 8:26 PM, Maciej W. Rozycki wrote: > Hi Fredrik, > >> I agree, that is important too. I will post an updated v5 soon. Another >> alternative change is to define check_insn_opc_user_only as >> >> static inline void check_insn_opc_user_only(DisasContext *ctx, int flags) >> { >> #ifndef CONFIG_USER_ONLY >> check_insn_opc_removed(ctx, flags); >> #endif >> } >> >> by referring to check_insn_opc_removed (instead of copying its definition). >> Would you consider this an improvement for v5 too? > > Yes, it does look like an improvement to me, reducing code duplication. > Thanks for looking into it further. Yes, also check_insn_opc_removed() is a good place to add debugging/tracing events. Fredrik with this change and the swaps, feel free to keep my previous Reviewed-by tag. >>> here (and swapping the two former calls ought to be fixed separately; I >>> haven't checked if there are more cases like this, but if so, then they >>> would best be amended with a single change). >> >> I'll defer other ordering and indentation fixes since I'm not sure whether >> such changes would be accepted. > > Sure, no need for you to rush doing that. In the absence of someone > willing to do such clean-ups voluntarily I would consider it a > maintainer's duty really. > > Maciej >
diff --git a/target/mips/translate.c b/target/mips/translate.c index 77d678353e..327e96307b 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -22458,6 +22458,7 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx) case OPC_DMULTU: case OPC_DDIV: case OPC_DDIVU: + check_insn_opc_user_only(ctx, INSN_R5900); check_insn(ctx, ISA_MIPS3); check_mips_64(ctx); gen_muldiv(ctx, op1, 0, rs, rt); @@ -24962,6 +24963,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) break; case OPC_LL: /* Load and stores */ check_insn(ctx, ISA_MIPS2); + check_insn_opc_user_only(ctx, INSN_R5900); /* Fallthrough */ case OPC_LWL: case OPC_LWR: @@ -24987,6 +24989,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) case OPC_SC: check_insn(ctx, ISA_MIPS2); check_insn_opc_removed(ctx, ISA_MIPS32R6); + check_insn_opc_user_only(ctx, INSN_R5900); gen_st_cond(ctx, op, rt, rs, imm); break; case OPC_CACHE: @@ -25253,9 +25256,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) #if defined(TARGET_MIPS64) /* MIPS64 opcodes */ + case OPC_LLD: + check_insn_opc_user_only(ctx, INSN_R5900); + /* fall through */ case OPC_LDL: case OPC_LDR: - case OPC_LLD: check_insn_opc_removed(ctx, ISA_MIPS32R6); /* fall through */ case OPC_LWU: @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx) break; case OPC_SCD: check_insn_opc_removed(ctx, ISA_MIPS32R6); + check_insn_opc_user_only(ctx, INSN_R5900); check_insn(ctx, ISA_MIPS3); check_mips_64(ctx); gen_st_cond(ctx, op, rt, rs, imm);
These MIPS III instructions are unavailable and therefore trapped and emulated by the Linux kernel. Signed-off-by: Fredrik Noring <noring@nocrew.org> --- target/mips/translate.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)