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

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

[Miodrag Dinic]

[-- Attachment #1.1: Type: text/plain, Size: 3957 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 6282 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-target-mips-Fix-ALIGN-instruction-when-bp-0.patch --]
[-- Type: text/x-patch; name="0001-target-mips-Fix-ALIGN-instruction-when-bp-0.patch", Size: 1394 bytes --]

From e01539a11061c447bece8dccde1715da9534024d Mon Sep 17 00:00:00 2001
From: Miodrag Dinic <miodrag.dinic@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.dinic@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

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

[Aurelien Jarno]

[snip]

> From e01539a11061c447bece8dccde1715da9534024d Mon Sep 17 00:00:00 2001
> From: Miodrag Dinic <miodrag.dinic@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.dinic@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

The way to fix that is basically ok. However you should just use
tcg_gen_ext32s_tl instead of tcg_gen_ext32s_i64 and drop the
TARGET_MIPS64 #ifdef.

> +        default:

Then you can replace this by OPC_DALIGN for more clarity.

> +            tcg_gen_mov_tl(cpu_gpr[rd], t0);
> +        }
>      } else {
>          TCGv t1 = tcg_temp_new();
>          gen_load_gpr(t1, rs);

The resulting binary code should be the same, but less #ifdef means less
risk of breakage, as the code is always compiled.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net