Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only

[Maciej W. Rozycki]

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