[Qemu-devel] [PATCH] target-mips: Fix ALIGN instruction when bp=0

[Miodrag Dinic]

Hello to everyone,

We have uncovered a use-case with ALIGN instruction which is
not handled correctly by QEMU. It impacts both, user and system mode emulation.

Using ALIGN instruction with bp=0 as the last argument, should behave
as a register to register move with sign extension if running on a mips64 
system.
The problem is that the sign extension is not preformed.

Taken from the official documentation :

Operation

ALIGN:
    tmp_rt_hi = unsigned_word(GPR[rt]) << (8*bp)
    tmp_rs_lo =unsigned_word(GPR[rs]) >> (8*(4-bp))
    tmp = tmp_rt_hi || tmp_rt_lo
    GPR[rd] = sign_extend.32(tmp)

DALIGN
    tmp_rt_hi = unsigned_doubleword(GPR[rt]) << (8*bp)
    tmp_rs_lo =unsigned_doubleword(GPR[rs]) >> (8*(8-bp))
    tmp = tmp_rt_hi || tmp_rt_lo
    GPR[rd] = tmp


Here is a simple test for reproducing the problem :

#include <stdio.h>
#include <limits>
#include <stdint.h>


int main(int argc, char * argv[]) {

  int64_t param1, param2, result, resultd;
  int64_t *param1_ptr = &param1, *param2_ptr = &param2, *result_ptr = &result, 
*resultd_ptr = &resultd;

  uint64_t param1_arr[] = { 0xfedcba98, 0x01234567, 0xaabbccdd, 0x004488bb };
  uint64_t param2_arr[] = { 0x01234567, 0xaabbccdd, 0xfedcba98, 0x004488bb };
  uint64_t result_arr[] = { 0xfffffffffedcba98, 0x1234567, 0xffffffffaabbccdd, 
0x4488bb };
  uint64_t resultd_arr[] = { 0xfedcba98, 0x1234567, 0xaabbccdd, 0x4488bb };

  printf("\n\n");

  for (int i = 0; i < 4; i++) {
    param1 = param1_arr[i];
    param2 = param2_arr[i];
  __asm__ volatile(
      "move          $t0, %0\n\t"
      "move          $t1, %1\n\t"
      "move          $t2, %2\n\t"
      "move          $t3, %3\n\t"

      "ld            $a0, 0($t0)\n\t"
      "ld            $a1, 0($t0)\n\t"
      "align         $a2, $a1, $a0, 0\n\t"
      "dalign        $a3, $a1, $a0, 0\n\t"
      "sd            $a2, 0($t2)\n\t"
      "sd            $a3, 0($t3)\n\t"
      :
      : "r" (param1_ptr), "r" (param2_ptr), "r" (result_ptr), "r"(resultd_ptr)
      : "a0", "a1", "a2", "a3", "t0", "t1", "t2", "t3", "cc", "memory");
      printf("ALIGN %s: ", result == result_arr[i] ? "PASS" : "FAIL");
      printf("Expected = %lx, actual = %lx\n", result_arr[i], result);
      printf("DALIGN %s: ", resultd == resultd_arr[i] ? "PASS" : "FAIL");
      printf("Expected = %lx, actual = %lx\n", resultd_arr[i], resultd);
  }
}

Compile it with any available R6 enabled toolchain :

mips-img-linux-gnu-gcc -static -mips64r6 -mabi=64 align-instr.cc -o 
align-instr-test

Running the test :

<QEMU>/mips64el-linux-user/qemu-mips64el -cpu MIPS64R6-generic 
./align-instr-test

ALIGN FAIL: Expected = fffffffffedcba98, actual = fedcba98
DALIGN PASS: Expected = fedcba98, actual = fedcba98
ALIGN PASS: Expected = 1234567, actual = 1234567
DALIGN PASS: Expected = 1234567, actual = 1234567
ALIGN FAIL: Expected = ffffffffaabbccdd, actual = aabbccdd
DALIGN PASS: Expected = aabbccdd, actual = aabbccdd
ALIGN PASS: Expected = 4488bb, actual = 4488bb
DALIGN PASS: Expected = 4488bb, actual = 4488bb


Test includes dalign instruction checking for verification
there is no regression after retesting with a patch applied.

A fix for this issue is to add sign extension for the bp=0
case when running within a 64-bit target.

After applying a patch, the test output is :
<QEMU>/mips64el-linux-user/qemu-mips64el -cpu MIPS64R6-generic 
./align-instr-test

ALIGN PASS: Expected = fffffffffedcba98, actual = fffffffffedcba98
DALIGN PASS: Expected = fedcba98, actual = fedcba98
ALIGN PASS: Expected = 1234567, actual = 1234567
DALIGN PASS: Expected = 1234567, actual = 1234567
ALIGN PASS: Expected = ffffffffaabbccdd, actual = ffffffffaabbccdd
DALIGN PASS: Expected = aabbccdd, actual = aabbccdd
ALIGN PASS: Expected = 4488bb, actual = 4488bb
DALIGN PASS: Expected = 4488bb, actual = 4488bb


Patch is in the attachment.

Regards,
Miodrag


From e01539a11061c447bece8dccde1715da9534024d Mon Sep 17 00:00:00 2001
From: Miodrag Dinic <miodrag.di...@imgtec.com>
Date: Thu, 3 Dec 2015 16:48:57 +0100
Subject: [PATCH] target-mips: Fix ALIGN instruction when bp=0

If executing ALIGN with shift count bp=0 within mips64 emulation,
the result of the operation should be sign extended.

Taken from the official documentation (pseudo code) :

ALIGN:
	tmp_rt_hi = unsigned_word(GPR[rt]) << (8*bp)
	tmp_rs_lo = unsigned_word(GPR[rs]) >> (8*(4-bp))
	tmp = tmp_rt_hi || tmp_rt_lo
	GPR[rd] = sign_extend.32(tmp)

Signed-off-by: Miodrag Dinic <miodrag.di...@imgtec.com>
---
 target-mips/translate.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 5626647..f20678c 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4630,7 +4630,15 @@ static void gen_align(DisasContext *ctx, int opc, int rd, int rs, int rt,
     t0 = tcg_temp_new();
     gen_load_gpr(t0, rt);
     if (bp == 0) {
-        tcg_gen_mov_tl(cpu_gpr[rd], t0);
+        switch (opc) {
+        case OPC_ALIGN:
+#if defined(TARGET_MIPS64)
+            tcg_gen_ext32s_i64(cpu_gpr[rd], t0);
+            break;
+#endif
+        default:
+            tcg_gen_mov_tl(cpu_gpr[rd], t0);
+        }
     } else {
         TCGv t1 = tcg_temp_new();
         gen_load_gpr(t1, rs);
-- 
1.9.1