Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base

[Laurent Vivier <laurent@vivier.eu>]

Le 30/06/2015 18:45, Peter Maydell a écrit :
> On 30 June 2015 at 17:19, Laurent Vivier <laurent@vivier.eu> wrote:
>> When guest base is disabled, RESERVED_VA is 0, and
>> (__guest < RESERVED_VA) is always false as __guest is unsigned.
>>
>> With -Werror=type-limits, this triggers an error:
>>
>>     include/exec/cpu_ldst.h:60:31: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
>>          (!RESERVED_VA || (__guest < RESERVED_VA)); \
>>
>> This patch removes this comparison when guest base is disabled.
> 
> Is there a useful reason to compile with --disable-guest-base
> (ie why we should retain the !CONFIG_USE_GUEST_BASE code
> in QEMU at all) ? It was originally optional because we
> didn't support it in all our TCG hosts, but we fixed that
> back in 2012...

TCG generates less code, so performance is better (well, it is what I
guess).

I've compiled a kernel with and without guest base in a chrooted
linux-user-qemu.
Without guest base it is ~1 minute less for a 13 minutes build.

I can do more tests if you want.

> (We can certainly take a compile fix for 2.4 even if
> we decide we want to rip it out for 2.5, of course.)
> 
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>  include/exec/cpu_ldst.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>> index 1239c60..f278126 100644
>> --- a/include/exec/cpu_ldst.h
>> +++ b/include/exec/cpu_ldst.h
>> @@ -54,11 +54,16 @@
>>  #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
>>  #define h2g_valid(x) 1
>>  #else
>> +#if defined(CONFIG_USE_GUEST_BASE)
>>  #define h2g_valid(x) ({ \
>>      unsigned long __guest = (unsigned long)(x) - GUEST_BASE; \
>>      (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS)) && \
>>      (!RESERVED_VA || (__guest < RESERVED_VA)); \
>>  })
>> +#else
>> +#define h2g_valid(x) \
>> +    ((unsigned long)(x) < (1ul << TARGET_VIRT_ADDR_SPACE_BITS))
> 
> "ul" as a suffix is almost always wrong, incidentally,
> though obviously here you're just copying the condition
> from the existing code. Consider the case when an
> unsigned long is 32 bits but TARGET_VIRT_ADDR_SPACE_BITS
> is 32 or more (ie almost always on a 32-bit host).

I think it can't happen because of previous lines:

...
#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
#define h2g_valid(x) 1
#else
...

Laurent