qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
@ 2015-06-30 16:19 Laurent Vivier
  2015-06-30 16:45 ` Peter Maydell
  0 siblings, 1 reply; 10+ messages in thread
From: Laurent Vivier @ 2015-06-30 16:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, Laurent Vivier

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.

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))
+#endif
 #endif
 
 #define h2g_nocheck(x) ({ \
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-06-30 16:19 [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base Laurent Vivier
@ 2015-06-30 16:45 ` Peter Maydell
  2015-06-30 17:13   ` Laurent Vivier
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2015-06-30 16:45 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers

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

(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).

thanks
-- PMM

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-06-30 16:45 ` Peter Maydell
@ 2015-06-30 17:13   ` Laurent Vivier
  2015-06-30 17:20     ` Peter Maydell
  0 siblings, 1 reply; 10+ messages in thread
From: Laurent Vivier @ 2015-06-30 17:13 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Riku Voipio, QEMU Developers



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-06-30 17:13   ` Laurent Vivier
@ 2015-06-30 17:20     ` Peter Maydell
  2015-06-30 23:58       ` Laurent Vivier
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2015-06-30 17:20 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers

On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> 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.

Hmm. That's a fair chunk of speedup. On the downside:
 * you only get this if you're willing to build QEMU from
   source with funny options
 * it won't work for all guest/host combinations (sometimes
   the guest really wants to be able to map at low addresses
   the host won't permit)
 * it's an extra configuration to maintain which we're
   clearly not testing at all upstream

I'd still favour removing it completely, personally...

>> "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
> ...

Oops, yes, I didn't read back far enough.

As a patch to fix a compile warning for 2.4,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(it's a bit sad the compiler doesn't notice that
it will never evaluate the x < 0 expression.)

thanks
-- PMM

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-06-30 17:20     ` Peter Maydell
@ 2015-06-30 23:58       ` Laurent Vivier
  2015-07-01 11:12         ` Peter Maydell
  2015-07-01 13:15         ` Aurelien Jarno
  0 siblings, 2 replies; 10+ messages in thread
From: Laurent Vivier @ 2015-06-30 23:58 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Riku Voipio, QEMU Developers



Le 30/06/2015 19:20, Peter Maydell a écrit :
> On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>>
>> 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.
> 
> Hmm. That's a fair chunk of speedup. On the downside:
>  * you only get this if you're willing to build QEMU from
>    source with funny options
>  * it won't work for all guest/host combinations (sometimes
>    the guest really wants to be able to map at low addresses
>    the host won't permit)
>  * it's an extra configuration to maintain which we're
>    clearly not testing at all upstream
> 
> I'd still favour removing it completely, personally...

In fact, I have made more measurements, it saves only ~10 seconds on a
13 minutes build.

my test is: "make -j 4 vmlinux"
(target: m68k, host: x86_64, 4 cores x 2 threads)

--enable-guest-base

real    13m26.134s	13m28.712s	13m28.053s	13m28.875s
user    52m44.882s	52m56.075s	52m49.223s	52m55.366s
sys     0m33.452s	0m33.613s	0m33.013s	0m33.336s

--disable-guest-base

real    13m20.412s	13m17.773s	13m15.836s	13m13.278s
user    52m23.165s	52m7.184s	52m1.547s	51m50.277s
sys     0m33.427s	0m33.392s	0m32.954s	0m33.430s

Laurent

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-06-30 23:58       ` Laurent Vivier
@ 2015-07-01 11:12         ` Peter Maydell
  2015-07-01 13:15         ` Aurelien Jarno
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2015-07-01 11:12 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers

On 1 July 2015 at 00:58, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 30/06/2015 19:20, Peter Maydell a écrit :
>> On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>>
>>> 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.
>>
>> Hmm. That's a fair chunk of speedup. On the downside:
>>  * you only get this if you're willing to build QEMU from
>>    source with funny options
>>  * it won't work for all guest/host combinations (sometimes
>>    the guest really wants to be able to map at low addresses
>>    the host won't permit)
>>  * it's an extra configuration to maintain which we're
>>    clearly not testing at all upstream
>>
>> I'd still favour removing it completely, personally...
>
> In fact, I have made more measurements, it saves only ~10 seconds on a
> 13 minutes build.

...that seems like it swings the argument pretty strongly
towards dropping the no-guest-base config entirely.

I think my suggestion would be:
 1) apply this patch for 2.4
 2) note in the 2.4 changelog that we're planning to drop
   --disable-guest-base for 2.5
 3) remove the config option and the code after 2.4 releases

thanks
-- PMM

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-06-30 23:58       ` Laurent Vivier
  2015-07-01 11:12         ` Peter Maydell
@ 2015-07-01 13:15         ` Aurelien Jarno
  2015-07-01 18:21           ` Laurent Vivier
  1 sibling, 1 reply; 10+ messages in thread
From: Aurelien Jarno @ 2015-07-01 13:15 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Peter Maydell, Riku Voipio, QEMU Developers

On 2015-07-01 01:58, Laurent Vivier wrote:
> 
> 
> Le 30/06/2015 19:20, Peter Maydell a écrit :
> > On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >>
> >> 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.
> > 
> > Hmm. That's a fair chunk of speedup. On the downside:
> >  * you only get this if you're willing to build QEMU from
> >    source with funny options
> >  * it won't work for all guest/host combinations (sometimes
> >    the guest really wants to be able to map at low addresses
> >    the host won't permit)
> >  * it's an extra configuration to maintain which we're
> >    clearly not testing at all upstream
> > 
> > I'd still favour removing it completely, personally...
> 
> In fact, I have made more measurements, it saves only ~10 seconds on a
> 13 minutes build.
> 
> my test is: "make -j 4 vmlinux"
> (target: m68k, host: x86_64, 4 cores x 2 threads)

Note that on x86_64, guest base is implemented by using the gs segment
register. That explains why the impact should be relatively low, as your
test shows.

> --enable-guest-base
> 
> real    13m26.134s	13m28.712s	13m28.053s	13m28.875s
> user    52m44.882s	52m56.075s	52m49.223s	52m55.366s
> sys     0m33.452s	0m33.613s	0m33.013s	0m33.336s
> 
> --disable-guest-base
> 
> real    13m20.412s	13m17.773s	13m15.836s	13m13.278s
> user    52m23.165s	52m7.184s	52m1.547s	51m50.277s
> sys     0m33.427s	0m33.392s	0m32.954s	0m33.430s
> 
> Laurent
> 
> 
> 
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-07-01 13:15         ` Aurelien Jarno
@ 2015-07-01 18:21           ` Laurent Vivier
  2015-07-01 19:46             ` Aurelien Jarno
  2015-07-21  6:51             ` Richard Henderson
  0 siblings, 2 replies; 10+ messages in thread
From: Laurent Vivier @ 2015-07-01 18:21 UTC (permalink / raw)
  To: Aurelien Jarno; +Cc: Peter Maydell, Riku Voipio, QEMU Developers



Le 01/07/2015 15:15, Aurelien Jarno a écrit :
> On 2015-07-01 01:58, Laurent Vivier wrote:
>>
>>
>> Le 30/06/2015 19:20, Peter Maydell a écrit :
>>> On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> Hmm. That's a fair chunk of speedup. On the downside:
>>>  * you only get this if you're willing to build QEMU from
>>>    source with funny options
>>>  * it won't work for all guest/host combinations (sometimes
>>>    the guest really wants to be able to map at low addresses
>>>    the host won't permit)
>>>  * it's an extra configuration to maintain which we're
>>>    clearly not testing at all upstream
>>>
>>> I'd still favour removing it completely, personally...
>>
>> In fact, I have made more measurements, it saves only ~10 seconds on a
>> 13 minutes build.
>>
>> my test is: "make -j 4 vmlinux"
>> (target: m68k, host: x86_64, 4 cores x 2 threads)
> 
> Note that on x86_64, guest base is implemented by using the gs segment
> register. That explains why the impact should be relatively low, as your
> test shows.

I did a similar test on a PowerPC host, It is 2 seconds MORE on an 1m27s
build WITH --disable-guest-base.

So, definitively, I think the option can be dropped.

Laurent

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-07-01 18:21           ` Laurent Vivier
@ 2015-07-01 19:46             ` Aurelien Jarno
  2015-07-21  6:51             ` Richard Henderson
  1 sibling, 0 replies; 10+ messages in thread
From: Aurelien Jarno @ 2015-07-01 19:46 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Peter Maydell, Riku Voipio, QEMU Developers

On 2015-07-01 20:21, Laurent Vivier wrote:
> 
> 
> Le 01/07/2015 15:15, Aurelien Jarno a écrit :
> > On 2015-07-01 01:58, Laurent Vivier wrote:
> >>
> >>
> >> Le 30/06/2015 19:20, Peter Maydell a écrit :
> >>> On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
> >>>>
> >>>>
> >>>> 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.
> >>>
> >>> Hmm. That's a fair chunk of speedup. On the downside:
> >>>  * you only get this if you're willing to build QEMU from
> >>>    source with funny options
> >>>  * it won't work for all guest/host combinations (sometimes
> >>>    the guest really wants to be able to map at low addresses
> >>>    the host won't permit)
> >>>  * it's an extra configuration to maintain which we're
> >>>    clearly not testing at all upstream
> >>>
> >>> I'd still favour removing it completely, personally...
> >>
> >> In fact, I have made more measurements, it saves only ~10 seconds on a
> >> 13 minutes build.
> >>
> >> my test is: "make -j 4 vmlinux"
> >> (target: m68k, host: x86_64, 4 cores x 2 threads)
> > 
> > Note that on x86_64, guest base is implemented by using the gs segment
> > register. That explains why the impact should be relatively low, as your
> > test shows.
> 
> I did a similar test on a PowerPC host, It is 2 seconds MORE on an 1m27s
> build WITH --disable-guest-base.
> 
> So, definitively, I think the option can be dropped.

I fully agree.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base
  2015-07-01 18:21           ` Laurent Vivier
  2015-07-01 19:46             ` Aurelien Jarno
@ 2015-07-21  6:51             ` Richard Henderson
  1 sibling, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2015-07-21  6:51 UTC (permalink / raw)
  To: Laurent Vivier, Aurelien Jarno
  Cc: Peter Maydell, Riku Voipio, QEMU Developers

On 07/01/2015 07:21 PM, Laurent Vivier wrote:
>
>
> Le 01/07/2015 15:15, Aurelien Jarno a écrit :
>> On 2015-07-01 01:58, Laurent Vivier wrote:
>>>
>>>
>>> Le 30/06/2015 19:20, Peter Maydell a écrit :
>>>> On 30 June 2015 at 18:13, Laurent Vivier <laurent@vivier.eu> wrote:
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Hmm. That's a fair chunk of speedup. On the downside:
>>>>   * you only get this if you're willing to build QEMU from
>>>>     source with funny options
>>>>   * it won't work for all guest/host combinations (sometimes
>>>>     the guest really wants to be able to map at low addresses
>>>>     the host won't permit)
>>>>   * it's an extra configuration to maintain which we're
>>>>     clearly not testing at all upstream
>>>>
>>>> I'd still favour removing it completely, personally...
>>>
>>> In fact, I have made more measurements, it saves only ~10 seconds on a
>>> 13 minutes build.
>>>
>>> my test is: "make -j 4 vmlinux"
>>> (target: m68k, host: x86_64, 4 cores x 2 threads)
>>
>> Note that on x86_64, guest base is implemented by using the gs segment
>> register. That explains why the impact should be relatively low, as your
>> test shows.
>
> I did a similar test on a PowerPC host, It is 2 seconds MORE on an 1m27s
> build WITH --disable-guest-base.

That should be measurement noise, since a PPC host changes nothing except the 
register number for the indexed memory address.  Same for aarch64, s390, and 
sparc.  We do reserve a register on ia64, but that does require an extra insn.

The implementation is less friendly for arm and mips.  The former because it's 
pressed for registers, and the later because it hasn't seen much tlc recently.

> So, definitively, I think the option can be dropped.

Also agreed.  One can still force the same effect at runtime with -B.


r~

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-07-21  6:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-30 16:19 [Qemu-devel] [PATCH] linux-user: Avoid compilation error with --disable-guest-base Laurent Vivier
2015-06-30 16:45 ` Peter Maydell
2015-06-30 17:13   ` Laurent Vivier
2015-06-30 17:20     ` Peter Maydell
2015-06-30 23:58       ` Laurent Vivier
2015-07-01 11:12         ` Peter Maydell
2015-07-01 13:15         ` Aurelien Jarno
2015-07-01 18:21           ` Laurent Vivier
2015-07-01 19:46             ` Aurelien Jarno
2015-07-21  6:51             ` Richard Henderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).