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

[Richard Henderson <rth@twiddle.net>]

On 12/05/2016 01:41 AM, Jin Guojie wrote:
> ------------------ Original ------------------
> From:  "Richard Henderson";<rth@twiddle.net>;
> Send time: Thursday, Dec 1, 2016 11:52 PM
> To: "Jin Guojie"<jinguojie@loongson.cn>; "qemu-devel"<qemu-devel@nongnu.org>;
> Cc: "Aurelien Jarno"<aurelien@aurel32.net>; "James Hogan"<james.hogan@imgtec.com>;
> 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

You misunderstand.  For a 64-bit guest, the result we're looking for is
0xfffffffffffff003.

(1) the type of a_bits is unsigned int, not int,
(2) which means the expression "page_mask | ((1 << a_bits) - 1)"
    becomes unsigned int,
(3) which means that the assignment to mask gets truncated.

> 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?

For a 64-bit guest we need a 64-bit quantity.  For a 32-bit guest... we need to
match the load that we issue from cmp_off.  If use use LWU, then we need an
unsigned 32-bit quantity; if we use LW, then we need a signed 32-bit quantity.


r~