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

[Jin Guojie]

------------------ Original ------------------
From:  "Richard Henderson";<address@hidden>;
Send time: Thursday, Dec 1, 2016 11:52 PM
To: "Jin Guojie"<address@hidden>; "qemu-devel"<address@hidden>;
Cc: "Aurelien Jarno"<address@hidden>; "James Hogan"<address@hidden>;
Subject:  Re: [PATCH v5 00/10] tcg mips64 and mips r6 improvements

Thanks a lot for Richard's careful review.
I have some different opinions for discussion:

>  @@ -1233,7 +1235,8 @@ static void tcg_out_tlb_load(TCGContext *s, TCGReg 
> base,
>  -    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.

Since mask is defined as a C type "target_ulong" at the beginning of the 
function, 
I guess an implicit type cast should be already done by the compiler.
So your change is functionally the same with v5 patch. 
To test my idea, I wrote a small case and compiled it on an x86_64 host:

main()
{
  int a_bits = 2;
  int page_mask = 0xfffff000;    /* X86 4KB page*/
  unsigned int mask = page_mask | ((1 << a_bits) - 1);
  unsigned long m = mask;
  printf("mask=%lx\n", m);
}

$ gcc a.c
$ file a.out 
a.out: Mach-O 64-bit executable x86_64
$ ./a.out
mask=fffff003

The output is exactly an unsigned 32-bit quantity.
I didn't see a wrong result generated. 
Could you teach me where is my mistake?

>       if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
>  -        tcg_out_ext32u(s, base, addrl);
>  -        addrl = base;
>  +        tcg_out_ext32u(s, TCG_TMP0, TCG_TMP0);
>       }
> 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.

In v5 patch, TMP0 is first loaded via LD, and then it is zero-extended. This is 
just
the same behavior as LWU.
After OPC_AND, TMP1 also contains unsigned 32-bit quantity. 
So in theory, there should not exist compare problem between TMP1 and TMP0.
I agree with your opinion that addrl should be zero-extended, but It's really 
strange
that the last ALIAS_PADD doesn't generate a bad host address in v5 patch.
If v5 patch really lacks the zero-extension operation of  addrl, the subsequent 
load
or store will access wrong memory space.
However, all the tests so far did not crash at this point.
To further verify my idea, I will do more debug work and test the tlb miss rate
to see if your analysis should be implemented.

At the meantime, I am waiting for Aurelien's test results. I think it's better 
to collect
all people's feedback before I submit the v6 patch.

Jin Guojie