Re: [Qemu-devel] [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64

Re: [Qemu-devel] [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64

[Jin Guojie]

Here I can describe the problem when patch 11 is not applied.

When booting Linux kernel with qemu-system-i386 on mips64el host, the
guest CPU enters infinite loop:


0xc01f3b90:  cmp    0x14(%esp),%esi
0xc01f3b94:  jae    0xc01f3b99

 qemu_ld_i32 tmp1,tmp2,leul,2
 mov_i32 tmp0,esi
 mov_i32 cc_src,tmp1
 brcond_i32 tmp0,cc_src,geu,$L1

0xffd64e8430:  lw       s2,24(s0)    <-- esi
0xffd64e8444:  sltu     at,s2,s1

s2(representing ESI) is loaded via lw, which always do sign-extension.
This is just the same case as Richard and Aurelien commented.

However, qemu_ld_i32 returns through slow path.
The memory address 0x14(%esp) is read by calling helper_le_ldul_mmu(). The
returned value V0 is then assigned to s1.

The helper function is defined in softmmu_template.h as:

#define WORD_TYPE tcg_target_ulong   // same as uint64_t
#define DATA_TYPE  uint32_t

WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr,
                            TCGMemOpIdx oi, uintptr_t retaddr)
{
    DATA_TYPE res;
    ......
}

By adding printf(), I noticed that res is returned without high 32-bit extended.

That means: if %esi and 0x14(%esp) has the same 32-bit value 0xcccccccc, they 
have different 64-bit value when being compared by sltu:

s2: 0xffffffffcccccccc
s1: 0x00000000cccccccc

To verify this, I wrote another small C program:

[a.c]
unsigned long add()
{
  return (unsigned int)0xcccccccc;
}

main()
{
  printf("%lx\n", add());
}

$ gcc a.c -mabi=64
$ ./a.out
cccccccc

This is just the value which helper_le_ld_name() returns.

To fix the comparision result, I introduced patch 11, in which the 32-bit registers in condition functions
are forced to be signed-extended, and then kernel booting is OK.

By reading Richard and Aurelien's comment, I realized now the best way to solve this problem
is not to add ext32s in brcond_32i, but to fix the helper function. In another word,
the register value should be 32-bit sign-extened at where it's being *created*, not where
it's being *utilized*. Maybe I can do this ext32s after helper_le_ld_name() is call back to ensure 
 V0 to be sign-extended. 

I notice that, in tcg/sparc/tcg-target.inc.c, there is a similar comment "We let the helper sign-extend SB and SW, 
but leave SL for here". This may be a good evidence supporting my idea.

If you think my idea is reasonable, I will submit a better solution in v4 patch soon.

Jin Guojie
 
------------------ Original ------------------
From:  "Aurelien Jarno";<aurelien@aurel32.net>;
Date:  Nov 25, 2016
To:  "Richard Henderson"<rth@twiddle.net>; 
Cc:  "Jin Guojie"<jinguojie@loongson.cn>; "qemu-devel"<qemu-devel@nongnu.org>; "James Hogan"<james.hogan@imgtec.com>; 
Subject:  Re: [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64



On 2016-11-25 13:06, Richard Henderson wrote:
> On 11/25/2016 04:31 AM, Jin Guojie wrote:
> > 32-bit condition functions(like brcond_i32) should only
> > compare the low half parts of two 64-bit host registers.
> > However, MIPS64 does not have distinct instruction for
> > such operation. The operands should be sign extended
> > to fit the case.
> > 
> > Gcc handles 32-bit comparison in the same way, as the
> > following example shows:
> > 
> > [a.c]
> > main()
> > {
> >   long a = 0xcccccccc;
> >   long b = 0xdddddddd;
> >   int c = (int)a > (int)b;
> > }
> 
> This problem is why opcodes like
> 
>    OPC_INDEX_extrl_i64_i32
>    OPC_INDEX_extrh_i64_i32
>    OPC_INDEX_ext_i32_i64
>    OPC_INDEX_extu_i32_i64
> 
> exist.  The intention is to keep 32-bit values in their sign-extended form,
> exactly as the mips hardware manual requires.  At which point all 32-bit
> opcodes (ADDIU, SLL, etc) will preserve the 32-bit sign extension property.

It's even stronger than that. It's required for 32-bit opcodes to work
correctly. The manual says:

| If GPR rs does not contain a sign-extended 32-bit value (bits 63..31
| equal), then the result of the operation is UNPREDICTABLE.

Aurelien

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

Re: [Qemu-devel] [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64

[Richard Henderson]

On 11/27/2016 11:42 PM, Jin Guojie wrote:
> By reading Richard and Aurelien's comment, I realized now the best way to solve this problem
> is not to add ext32s in brcond_32i, but to fix the helper function. In another word,
> the register value should be 32-bit sign-extened at where it's being *created*, not where
> it's being *utilized*. Maybe I can do this ext32s after helper_le_ld_name() is call back to ensure
>  V0 to be sign-extended.

It's not necessarily V0 that needs to be extended, but the destination register 
(S1 in this case).  So perhaps

      tcg_out_opc_br(s, OPC_BEQ, TCG_REG_ZERO, TCG_REG_ZERO);
      /* delay slot */
-    tcg_out_mov(s, TCG_TYPE_REG, v0, TCG_REG_V0);
+    if (TCG_TARGET_REG_BITS == 64 && l->type == TCG_TYPE_I32) {
+        tcg_out_opc_sa(s, OPC_SLL, v0, TCG_REG_V0, 0);
+    } else {
+        tcg_out_opc_reg(s, OPC_OR, v0, TCG_REG_V0, TCG_REG_ZERO);
+    }

so that we always sign-extend 32-bit loads.


r~

Re: [Qemu-devel] [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64

[Aurelien Jarno]

On 2016-11-25 13:06, Richard Henderson wrote:
> On 11/25/2016 04:31 AM, Jin Guojie wrote:
> > 32-bit condition functions(like brcond_i32) should only
> > compare the low half parts of two 64-bit host registers.
> > However, MIPS64 does not have distinct instruction for
> > such operation. The operands should be sign extended
> > to fit the case.
> > 
> > Gcc handles 32-bit comparison in the same way, as the
> > following example shows:
> > 
> > [a.c]
> > main()
> > {
> >   long a = 0xcccccccc;
> >   long b = 0xdddddddd;
> >   int c = (int)a > (int)b;
> > }
> 
> This problem is why opcodes like
> 
>    OPC_INDEX_extrl_i64_i32
>    OPC_INDEX_extrh_i64_i32
>    OPC_INDEX_ext_i32_i64
>    OPC_INDEX_extu_i32_i64
> 
> exist.  The intention is to keep 32-bit values in their sign-extended form,
> exactly as the mips hardware manual requires.  At which point all 32-bit
> opcodes (ADDIU, SLL, etc) will preserve the 32-bit sign extension property.

It's even stronger than that. It's required for 32-bit opcodes to work
correctly. The manual says:

| If GPR rs does not contain a sign-extended 32-bit value (bits 63..31
| equal), then the result of the operation is UNPREDICTABLE.

Aurelien

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

Re: [Qemu-devel] [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64

[Richard Henderson]

On 11/25/2016 04:31 AM, Jin Guojie wrote:
> 32-bit condition functions(like brcond_i32) should only
> compare the low half parts of two 64-bit host registers.
> However, MIPS64 does not have distinct instruction for
> such operation. The operands should be sign extended
> to fit the case.
>
> Gcc handles 32-bit comparison in the same way, as the
> following example shows:
>
> [a.c]
> main()
> {
>   long a = 0xcccccccc;
>   long b = 0xdddddddd;
>   int c = (int)a > (int)b;
> }

This problem is why opcodes like

    OPC_INDEX_extrl_i64_i32
    OPC_INDEX_extrh_i64_i32
    OPC_INDEX_ext_i32_i64
    OPC_INDEX_extu_i32_i64

exist.  The intention is to keep 32-bit values in their sign-extended form, 
exactly as the mips hardware manual requires.  At which point all 32-bit 
opcodes (ADDIU, SLL, etc) will preserve the 32-bit sign extension property.

So you *should* never see a 32-bit comparison input that is not already 
sign-extended.

A more appropriate gcc example would be

$ cat z.c
int foo(int a, int b)
{
     return a > b;
}
$ mips64-linux-gcc -mabi=64 -O -S z.c
$ cat z.s
	...
         jr      $31
         slt     $2,$5,$4
	...


If you require this patch for getting correct results, then you have found a 
bug that needs to be fixed elsewhere.  Can you describe the problem that you saw?


r~

[Qemu-devel] [PATCH v3 11/11] tcg-mips: Adjust condition functions for mips64

[Jin Guojie]

32-bit condition functions(like brcond_i32) should only
compare the low half parts of two 64-bit host registers.
However, MIPS64 does not have distinct instruction for
such operation. The operands should be sign extended
to fit the case.

Gcc handles 32-bit comparison in the same way, as the
following example shows:

[a.c]
main()
{
  long a = 0xcccccccc;
  long b = 0xdddddddd;
  int c = (int)a > (int)b;
}

$ gcc a.c -S -mabi=64
$ vi a.s

        .......
        sd      $2,0($fp)
        sd      $3,8($fp)
        lw      $2,0($fp)
        lw      $3,8($fp)
        slt     $1,$2,$3
        ........

Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: James Hogan <james.hogan@imgtec.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Jin Guojie <jinguojie@loongson.cn>
---
 tcg/mips/tcg-target.inc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
index 95090b6..0eb48ce 100644
--- a/tcg/mips/tcg-target.inc.c
+++ b/tcg/mips/tcg-target.inc.c
@@ -696,6 +696,11 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg ret, TCGReg arg)
     }
 }
 
+static inline void tcg_out_ext32s(TCGContext *s, TCGReg ret, TCGReg arg)
+{
+    tcg_out_opc_reg(s, OPC_ADDU, ret, arg, TCG_REG_ZERO);
+}
+
 static void tcg_out_ldst(TCGContext *s, MIPSInsn opc, TCGReg data,
                          TCGReg addr, intptr_t ofs)
 {
@@ -2023,6 +2028,11 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
         break;
 
     case INDEX_op_brcond_i32:
+#if TCG_TARGET_REG_BITS == 64
+        tcg_out_ext32s(s, a0, a0);
+        tcg_out_ext32s(s, a1, a1);
+        /* FALLTHRU */
+#endif
     case INDEX_op_brcond_i64:
         tcg_out_brcond(s, a2, a0, a1, arg_label(args[3]));
         break;
@@ -2031,11 +2041,21 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
         break;
 
     case INDEX_op_movcond_i32:
+#if TCG_TARGET_REG_BITS == 64
+        tcg_out_ext32s(s, a1, a1);
+        tcg_out_ext32s(s, a2, a2);
+        /* FALLTHRU */
+#endif
     case INDEX_op_movcond_i64:
         tcg_out_movcond(s, args[5], a0, a1, a2, args[3], args[4]);
         break;
 
     case INDEX_op_setcond_i32:
+#if TCG_TARGET_REG_BITS == 64
+        tcg_out_ext32s(s, a1, a1);
+        tcg_out_ext32s(s, a2, a2);
+        /* FALLTHRU */
+#endif
     case INDEX_op_setcond_i64:
         tcg_out_setcond(s, args[3], a0, a1, a2);
         break;
-- 
2.1.0