Re: [Qemu-devel] [PATCH 3/3] Fix-gen_HILO-to-make-it-adapt-each-arch-which-use-acc

[Aurelien Jarno <aurelien@aurel32.net>]

On Tue, Dec 11, 2012 at 10:28:30PM +0800, Dongxue Zhang wrote:
> The old gen_HILO was used by many arch, and they use acc[0] only. But now we
> know the arch with dsp will have 4 acc reigster. So we need Fix gen_HILO.
> 
> When we add arch mipsdsp, we changed the gen_HILO, but now I found it may cause
> a bug. Because we just get index of acc register from opcode, other arch to use
> gen_HILO my have value at that bit, so, a bug cased.

The MIPS32, MIPS64 and even the R4000 and R4400 bits define these bits
as being 0. They should not be ignored as they must be 0.

What is wrong there though is that a DSP exception is generated instead
of a reserved one. This is a more generic problem than MFHI/MFLO, and
should be fixed by the following patch:

http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg01497.html

I am working on a rework though, given the comments this patch got.

> Now I take acc to be a param of gen_HILO, other arch use gen_HILO with that
> param set to 0, and dsp arch will set the value as which acc register it
> selected.
> 
> TARGET_MIPS64 in gen_HILO changed into (TARGET_LONG_SIZE == 8), on 64bits arch,
> acc register value sign-extended to 64bit value, save into register.

I don't get whey you want to change TARGET_MIPS64 in (TARGET_LONG_SIZE
== 8). What's the goal of that? 

> I don't have test log but there is something provide from manual. Instruction
> mfhi in the two manual:
> 
> MD00375-2B-MIPS64DSP-AFP-02.34:
> 31     26  25      21   20 16  15 11  10   6   5
> SPECIAL      0     ac     0      rd      0      MFHI
> 000000      000         00000          00000   010000
> 
> MD00764-2B-microMIPS32DSP-AFP-02.34:
> 31    26   25 21 20  16 15 14  13     6  5
> POOL32A      0     rs    ac      MFHI    POOL32Axf
> 000000     00000               00000001    111100
> 
> Current gen_HILO get acc from bit 21/22, and check_dsp. If arch is
> microMIPS32DSP, then a bug case. So I changed gen_HILO function for furture add
> other arch with dsp.
> 
> MFHI MTHI MFLO MTLO of mipsdsp have been tested, they worked fun.
> 
> Signed-off-by: Dongxue Zhang <elta.era@gmail.com>
> ---
>  target-mips/translate.c |   64 ++++++++++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 1701ca3..2b0972b 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -2571,10 +2571,10 @@ static void gen_shift (CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
>  }
>  
>  /* Arithmetic on HI/LO registers */
> -static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
> +static void gen_HILO(DisasContext *ctx, uint32_t opc, int reg,
> +                     unsigned int acc)
>  {
>      const char *opn = "hilo";
> -    unsigned int acc;
>  
>      if (reg == 0 && (opc == OPC_MFHI || opc == OPC_MFLO)) {
>          /* Treat as NOP. */
> @@ -2582,19 +2582,9 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          return;
>      }
>  
> -    if (opc == OPC_MFHI || opc == OPC_MFLO) {
> -        acc = ((ctx->opcode) >> 21) & 0x03;
> -    } else {
> -        acc = ((ctx->opcode) >> 11) & 0x03;
> -    }
> -
> -    if (acc != 0) {
> -        check_dsp(ctx);
> -    }
> -
>      switch (opc) {
>      case OPC_MFHI:
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>          if (acc != 0) {
>              tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]);
>          } else
> @@ -2605,7 +2595,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          opn = "mfhi";
>          break;
>      case OPC_MFLO:
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>          if (acc != 0) {
>              tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]);
>          } else
> @@ -2617,7 +2607,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          break;
>      case OPC_MTHI:
>          if (reg != 0) {
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>              if (acc != 0) {
>                  tcg_gen_ext32s_tl(cpu_HI[acc], cpu_gpr[reg]);
>              } else
> @@ -2632,7 +2622,7 @@ static void gen_HILO (DisasContext *ctx, uint32_t opc, int reg)
>          break;
>      case OPC_MTLO:
>          if (reg != 0) {
> -#if defined(TARGET_MIPS64)
> +#if (TARGET_LONG_SIZE == 8)
>              if (acc != 0) {
>                  tcg_gen_ext32s_tl(cpu_LO[acc], cpu_gpr[reg]);
>              } else
> @@ -10134,7 +10124,7 @@ static int decode_mips16_opc (CPUMIPSState *env, DisasContext *ctx,
>              gen_logic(env, ctx, OPC_NOR, rx, ry, 0);
>              break;
>          case RR_MFHI:
> -            gen_HILO(ctx, OPC_MFHI, rx);
> +            gen_HILO(ctx, OPC_MFHI, rx, 0);
>              break;
>          case RR_CNVT:
>              switch (cnvt_op) {
> @@ -10166,7 +10156,7 @@ static int decode_mips16_opc (CPUMIPSState *env, DisasContext *ctx,
>              }
>              break;
>          case RR_MFLO:
> -            gen_HILO(ctx, OPC_MFLO, rx);
> +            gen_HILO(ctx, OPC_MFLO, rx, 0);
>              break;
>  #if defined (TARGET_MIPS64)
>          case RR_DSRA:
> @@ -10922,11 +10912,11 @@ static void gen_pool16c_insn (CPUMIPSState *env, DisasContext *ctx, int *is_bran
>          break;
>      case MFHI16 + 0:
>      case MFHI16 + 1:
> -        gen_HILO(ctx, OPC_MFHI, uMIPS_RS5(ctx->opcode));
> +        gen_HILO(ctx, OPC_MFHI, uMIPS_RS5(ctx->opcode), 0);
>          break;
>      case MFLO16 + 0:
>      case MFLO16 + 1:
> -        gen_HILO(ctx, OPC_MFLO, uMIPS_RS5(ctx->opcode));
> +        gen_HILO(ctx, OPC_MFLO, uMIPS_RS5(ctx->opcode), 0);
>          break;
>      case BREAK16:
>          generate_exception(ctx, EXCP_BREAK);
> @@ -11286,16 +11276,16 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs,
>      case 0x35:
>          switch (minor) {
>          case MFHI32:
> -            gen_HILO(ctx, OPC_MFHI, rs);
> +            gen_HILO(ctx, OPC_MFHI, rs, 0);
>              break;
>          case MFLO32:
> -            gen_HILO(ctx, OPC_MFLO, rs);
> +            gen_HILO(ctx, OPC_MFLO, rs, 0);
>              break;
>          case MTHI32:
> -            gen_HILO(ctx, OPC_MTHI, rs);
> +            gen_HILO(ctx, OPC_MTHI, rs, 0);
>              break;
>          case MTLO32:
> -            gen_HILO(ctx, OPC_MTLO, rs);
> +            gen_HILO(ctx, OPC_MTLO, rs, 0);
>              break;
>          default:
>              goto pool32axf_invalid;
> @@ -14447,12 +14437,30 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx, int *is_branch)
>              break;
>          case OPC_MFHI:          /* Move from HI/LO */
>          case OPC_MFLO:
> -            gen_HILO(ctx, op1, rd);
> -            break;
> +            {
> +                unsigned int acc;
> +                acc = ((ctx->opcode) >> 21) & 0x03;
> +
> +                if (acc != 0) {
> +                    check_dsp(ctx);
> +                }
> +
> +                gen_HILO(ctx, op1, rd, acc);
> +                break;
> +            }
>          case OPC_MTHI:
>          case OPC_MTLO:          /* Move to HI/LO */
> -            gen_HILO(ctx, op1, rs);
> -            break;
> +            {
> +                unsigned int acc;
> +                acc = ((ctx->opcode) >> 11) & 0x03;
> +
> +                if (acc != 0) {
> +                    check_dsp(ctx);
> +                }
> +
> +                gen_HILO(ctx, op1, rs, acc);
> +                break;
> +            }
>          case OPC_PMON:          /* Pmon entry point, also R4010 selsl */
>  #ifdef MIPS_STRICT_STANDARD
>              MIPS_INVAL("PMON / selsl");
> -- 
> 1.7.10.4
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net