Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements

[Richard Henderson]

On 12/01/2016 05:51 AM, Jin Guojie wrote:
> Changes in v5:
>   * Update against master(v2.8.0-rc2)
>   * Fix a bug: 64-bit big-endian guests hang on mips64 little-endian
>       hosts, and vice versa. This bug was first introduced in v2 patch,
>       due to obvious misuse of ret/arg registers in tcg_out_bswap64().
> 
>         tcg_out_opc_reg(s, OPC_DSBH, ret, 0, arg);
>       - tcg_out_opc_reg(s, OPC_DSHD, ret, 0, arg);
>       + tcg_out_opc_reg(s, OPC_DSHD, ret, 0, ret);

Oops.  Thanks.

>   * Fix a style problem: checkpatch.pl forbids 'extern' to be used in .c.
> 
>       ERROR: externs should be avoided in .c files
>       #28: FILE: tcg/mips/tcg-target.inc.c:39:
>       +extern int link_error(void);

Hmph.  This is checkpatch not being helpful, but your change is fine.


I've looked at a diff between your current patch and the last update I had to
my branch.  This probably includes code that post-dates the v2 that you started
with.  There are two things I'd like to point out:


 @@ -1233,7 +1235,8 @@ static void tcg_out_tlb_load(TCGContext *s, TCGReg base,
TCGReg addrl,
      if (a_bits < s_bits) {
          a_bits = s_bits;
      }
 -    mask = (target_ulong)TARGET_PAGE_MASK | ((1 << a_bits) - 1);
 +
 +    mask = TARGET_PAGE_MASK | ((1 << a_bits) - 1);


You need the target_ulong cast here for mips64.
See patch ebb90a005da67147245cd38fb04a965a87a961b7.


      if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
 -        tcg_out_ext32u(s, base, addrl);
 -        addrl = base;
 +        tcg_out_ext32u(s, TCG_TMP0, TCG_TMP0);
      }

Why did you make this change?  It is definitely wrong.

We should be zero-extending the guest address, not the address that we just
loaded from CMP_OFF.  This is because we need to add an unsigned 32-bit
quantity to the full 64-bit host pointer that we load from ADD_OFF.

Did you notice a compare problem between TMP1 and TMP0 below?  If so, I believe
that a partial solution is the TARGET_PAGE_MASK change above.  I guess we also
need to do

  tcg_out_ldst(s, (TARGET_LONG_BITS == 64 ? OPC_LD
                   TCG_TARGET_REG_BITS == 64 : OPC_LWU : OPC_LW),
               TCG_TMP0, TCG_REG_A0, cmp_off);

in the else section of the tlb comparator load above, since mask will be 32-bit
unsigned for TARGET_LONG_BITS == 32, and we need an unsigned quantity to
compare that against.


r~


PS: Ignoring N32 for now.  But as a note to ourselves for future: What is the
proper combination of zero/sign-extend vs ADDU/DADDU for 32/64-bit guests and
an N32 host?  What we have now is technically illegal, with zero-extend + ADDU.
 But I can imagine several valid scenarios.