[Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Peter Maydell]

Adjust the conditional which guards the implementation of
cpu_get_real_ticks() via RDTSC, so that we don't try to use it
on x86 CPUs which don't implement RDTSC.  Instead we will fall
back to the no-cycle-counter-available default implementation.

Reported-by: Yurij Popov <oss@djphoenix.ru>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-timer.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-timer.h b/qemu-timer.h
index da7e97c..e35f163 100644
--- a/qemu-timer.h
+++ b/qemu-timer.h
@@ -169,7 +169,7 @@ static inline int64_t cpu_get_real_ticks(void)
     return retval;
 }
 
-#elif defined(__i386__)
+#elif defined(__i586__)
 
 static inline int64_t cpu_get_real_ticks(void)
 {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Paolo Bonzini]

Il 23/11/2012 16:12, Peter Maydell ha scritto:
> Adjust the conditional which guards the implementation of
> cpu_get_real_ticks() via RDTSC, so that we don't try to use it
> on x86 CPUs which don't implement RDTSC.  Instead we will fall
> back to the no-cycle-counter-available default implementation.
> 
> Reported-by: Yurij Popov <oss@djphoenix.ru>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-timer.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-timer.h b/qemu-timer.h
> index da7e97c..e35f163 100644
> --- a/qemu-timer.h
> +++ b/qemu-timer.h
> @@ -169,7 +169,7 @@ static inline int64_t cpu_get_real_ticks(void)
>      return retval;
>  }
>  
> -#elif defined(__i386__)
> +#elif defined(__i586__)
>  
>  static inline int64_t cpu_get_real_ticks(void)
>  {
> 

You should at least test __i686__ too:

$ gcc -m32 -dM -E -x c /dev/null |grep __i
#define __i686 1
#define __i686__ 1
#define __i386 1
#define __i386__ 1

Paolo

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Peter Maydell]

On 23 November 2012 15:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 23/11/2012 16:12, Peter Maydell ha scritto:
>> Adjust the conditional which guards the implementation of
>>
>> -#elif defined(__i386__)
>> +#elif defined(__i586__)
>>
>>  static inline int64_t cpu_get_real_ticks(void)
>>  {
>>
>
> You should at least test __i686__ too:
>
> $ gcc -m32 -dM -E -x c /dev/null |grep __i
> #define __i686 1
> #define __i686__ 1
> #define __i386 1
> #define __i386__ 1

Yuck. I had assumed gcc would define everything from i386
on up when building for later cores.

-- PMM

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Jamie Lokier]

Peter Maydell wrote:
> On 23 November 2012 15:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 23/11/2012 16:12, Peter Maydell ha scritto:
> >> Adjust the conditional which guards the implementation of
> >>
> >> -#elif defined(__i386__)
> >> +#elif defined(__i586__)
> >>
> >>  static inline int64_t cpu_get_real_ticks(void)
> >>  {
> >>
> >
> > You should at least test __i686__ too:
> >
> > $ gcc -m32 -dM -E -x c /dev/null |grep __i
> > #define __i686 1
> > #define __i686__ 1
> > #define __i386 1
> > #define __i386__ 1
> 
> Yuck. I had assumed gcc would define everything from i386
> on up when building for later cores.

No, and it doesn't define __i686__ on all x86-32 targets after i686 either:

    $ gcc -march=core2 -dM -E -x c /dev/null | grep __[0-9a-z] | sort
    #define __core2 1
    #define __core2__ 1
    #define __gnu_linux__ 1
    #define __i386 1
    #define __i386__ 1
    #define __linux 1
    #define __linux__ 1
    #define __tune_core2__ 1
    #define __unix 1
    #define __unix__ 1

x86 instruction sets haven't followed a linear progression of features
for quite a while, especially including non-Intel chips, so it stopped
making sense for GCC to indicate the instruction set in that way.

GCC 4.6.3 defines __i586__ only when the target arch is set by -march
(or default) to i586, pentium or pentium-mmx.

And it defines __i686__ only when -march= is set (or default) to c3-2,
i686, pentiumpro, pentium2, pentium3, pentium3m or pentium-m.

Otherwise it's just things like __athlon__, __corei7__, etc.

The only one that's consistent is __i386__ (and __i386).

-- Jamie

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Peter Maydell]

On 23 November 2012 15:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 23 November 2012 15:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> You should at least test __i686__ too:
>>
>> $ gcc -m32 -dM -E -x c /dev/null |grep __i
>> #define __i686 1
>> #define __i686__ 1
>> #define __i386 1
>> #define __i386__ 1
>
> Yuck. I had assumed gcc would define everything from i386
> on up when building for later cores.

...and there's an enormous list of x86 cores too. This bites
us already -- if you use '-march=native' to get "best for my
cpu" then on a Core2, say, it will define __i386__ and __core2__
but not __i686__, so TCG won't use cmov :-(

Anybody got any good ideas for how to say "is this at least
a 586/686?" in a way that won't fail for any newly introduced
x86 core types?

-- PMM

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Peter Maydell]

On 23 November 2012 15:31, Jamie Lokier <jamie@shareable.org> wrote:
> x86 instruction sets haven't followed a linear progression of features
> for quite a while, especially including non-Intel chips, so it stopped
> making sense for GCC to indicate the instruction set in that way.

If you're going to go down that route you need to start defining
#defines for features then, so we could say defined(__rdtsc__)
or defined(__cmov__) and so on. I don't see any of those either :-(

-- PMM

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Jamie Lokier]

Peter Maydell wrote:
> On 23 November 2012 15:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 23 November 2012 15:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> You should at least test __i686__ too:
> >>
> >> $ gcc -m32 -dM -E -x c /dev/null |grep __i
> >> #define __i686 1
> >> #define __i686__ 1
> >> #define __i386 1
> >> #define __i386__ 1
> >
> > Yuck. I had assumed gcc would define everything from i386
> > on up when building for later cores.
> 
> ...and there's an enormous list of x86 cores too. This bites
> us already -- if you use '-march=native' to get "best for my
> cpu" then on a Core2, say, it will define __i386__ and __core2__
> but not __i686__, so TCG won't use cmov :-(
> 
> Anybody got any good ideas for how to say "is this at least
> a 586/686?" in a way that won't fail for any newly introduced
> x86 core types?

Fwiw, cmov doesn't work on some VIA "686" class CPUs.

Shouldn't TCG decide whether to use cmov at runtime anyway, using
cpuid?  For dynamically generated code it would seem not very
expensive to do that.

Looking at GCC source, it has an internal flag to say whether the
target has cmov, but doesn't expose it in preprocessor conditionals.

-- Jamie

Re: [Qemu-devel] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Jamie Lokier]

Peter Maydell wrote:
> On 23 November 2012 15:31, Jamie Lokier <jamie@shareable.org> wrote:
> > x86 instruction sets haven't followed a linear progression of features
> > for quite a while, especially including non-Intel chips, so it stopped
> > making sense for GCC to indicate the instruction set in that way.
> 
> If you're going to go down that route you need to start defining
> #defines for features then, so we could say defined(__rdtsc__)
> or defined(__cmov__) and so on. I don't see any of those either :-(

It does for some major architectural instructions groups like MMX,
different kinds of SSE, etc.  But not everything and I don't see cmov
among them.  I agree it's unfortunate.

-- Jamie

[Qemu-devel] [PATCH] tcg-i386: Improve cmov detection

[Richard Henderson]

In addition to better compile-time detection, perform runtime detection.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/i386/tcg-target.c | 34 +++++++++++++++++++++++++++++++++-
 tcg/i386/tcg-target.h |  5 -----
 2 files changed, 33 insertions(+), 6 deletions(-)

Yall are right that there's no particularly good method with which
to detect i686 *or later*, and thus cmov support, in gcc.  If one
uses -march=native with any processor made in the last 5 years,
one will have at least SSE1 support.  So we can reasonably use that
as a clue.

To fill in the holes, we can do the check at runtime.  That does
involve a tiny amount of runtime overhead, testing a global variable.
I suspect that this is overhead is unmeasurable.


r~


diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
index 6f3ad3c..b333b46 100644
--- a/tcg/i386/tcg-target.c
+++ b/tcg/i386/tcg-target.c
@@ -97,6 +97,20 @@ static const int tcg_target_call_oarg_regs[] = {
 # define TCG_REG_L1 TCG_REG_EDX
 #endif
 
+/* Attempt to determine at compile-time whether the compiler assumes that
+   cmov is available.  We get 64-bit for free.  P6 (i686) and later include
+   support for cmov, but there is no one preprocessor define that determines
+   this.  Assume that all processors that include sse also support cmov, so
+   that we sorta future-proof this test against new preprocessor defines.  */
+#include <cpuid.h>
+#if (TCG_TARGET_REG_BITS == 64 \
+     || defined(__i686__) || defined(__pentium4__) \
+     || defined(__athlon__) || defined(__SSE__))
+# define have_cmov 1
+#else
+static bool have_cmov;
+#endif
+
 static uint8_t *tb_ret_addr;
 
 static void patch_reloc(uint8_t *code_ptr, int type,
@@ -943,7 +957,14 @@ static void tcg_out_movcond32(TCGContext *s, TCGCond cond, TCGArg dest,
                               TCGArg v1)
 {
     tcg_out_cmp(s, c1, c2, const_c2, 0);
-    tcg_out_modrm(s, OPC_CMOVCC | tcg_cond_to_jcc[cond], dest, v1);
+    if (have_cmov) {
+        tcg_out_modrm(s, OPC_CMOVCC | tcg_cond_to_jcc[cond], dest, v1);
+    } else {
+        int over = gen_new_label();
+        tcg_out_jxx(s, tcg_cond_to_jcc[tcg_invert_cond(cond)], over, 1);
+        tcg_out_mov(s, TCG_TYPE_I32, dest, v1);
+        tcg_out_label(s, over, s->code_ptr);
+    }
 }
 
 #if TCG_TARGET_REG_BITS == 64
@@ -2243,6 +2264,17 @@ static void tcg_target_qemu_prologue(TCGContext *s)
 
 static void tcg_target_init(TCGContext *s)
 {
+    /* If we could not determine cmov availablity at compile time, perform
+       the check at runtime.  99% certainty that we're running on hardware
+       that supports cmov, but we still need to check.  In case cmov is not
+       available, we'll use a small forward branch.  */
+#ifndef have_cmov
+    {
+        unsigned a, b, c, d;
+        have_cmov = (__get_cpuid(1, &a, &b, &c, &d) && (d & bit_CMOV));
+    }
+#endif
+
 #if !defined(CONFIG_USER_ONLY)
     /* fail safe */
     if ((1 << CPU_TLB_ENTRY_BITS) != sizeof(CPUTLBEntry))
diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index dbc6756..450078b 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -90,12 +90,7 @@ typedef enum {
 #define TCG_TARGET_HAS_nand_i32         0
 #define TCG_TARGET_HAS_nor_i32          0
 #define TCG_TARGET_HAS_deposit_i32      1
-#if defined(__x86_64__) || defined(__i686__)
-/* Use cmov only if the compiler is already doing so.  */
 #define TCG_TARGET_HAS_movcond_i32      1
-#else
-#define TCG_TARGET_HAS_movcond_i32      0
-#endif
 
 #if TCG_TARGET_REG_BITS == 64
 #define TCG_TARGET_HAS_div2_i64         1
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH] tcg-i386: Improve cmov detection

[Peter Maydell]

On 24 November 2012 17:39, Richard Henderson <rth@twiddle.net> wrote:
> --- a/tcg/i386/tcg-target.c
> +++ b/tcg/i386/tcg-target.c
> @@ -97,6 +97,20 @@ static const int tcg_target_call_oarg_regs[] = {
>  # define TCG_REG_L1 TCG_REG_EDX
>  #endif
>
> +/* Attempt to determine at compile-time whether the compiler assumes that
> +   cmov is available.  We get 64-bit for free.  P6 (i686) and later include
> +   support for cmov, but there is no one preprocessor define that determines
> +   this.  Assume that all processors that include sse also support cmov, so
> +   that we sorta future-proof this test against new preprocessor defines.  */
> +#include <cpuid.h>

MacOS gcc objects to this:
In file included from /Users/pm215/src/qemu/tcg/tcg.c:174:
/Users/pm215/src/qemu/tcg/i386/tcg-target.c:105:19: warning: cpuid.h:
No such file or directory

(though for some reason not as a fatal error).

-- PMM

Re: [Qemu-devel] [PATCH] tcg-i386: Improve cmov detection

[Aurelien Jarno]

On Sat, Nov 24, 2012 at 09:39:35AM -0800, Richard Henderson wrote:
> In addition to better compile-time detection, perform runtime detection.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/i386/tcg-target.c | 34 +++++++++++++++++++++++++++++++++-
>  tcg/i386/tcg-target.h |  5 -----
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> Yall are right that there's no particularly good method with which
> to detect i686 *or later*, and thus cmov support, in gcc.  If one
> uses -march=native with any processor made in the last 5 years,
> one will have at least SSE1 support.  So we can reasonably use that
> as a clue.
> 
> To fill in the holes, we can do the check at runtime.  That does
> involve a tiny amount of runtime overhead, testing a global variable.
> I suspect that this is overhead is unmeasurable.

If this overhead is unmesurable, and I think it is something true, I
think it would be better to just always use that on i386 (but not on
x86_64) instead of having a complex compile time detection that could
fail.

Otherwise the patch looks fine.


> diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
> index 6f3ad3c..b333b46 100644
> --- a/tcg/i386/tcg-target.c
> +++ b/tcg/i386/tcg-target.c
> @@ -97,6 +97,20 @@ static const int tcg_target_call_oarg_regs[] = {
>  # define TCG_REG_L1 TCG_REG_EDX
>  #endif
>  
> +/* Attempt to determine at compile-time whether the compiler assumes that
> +   cmov is available.  We get 64-bit for free.  P6 (i686) and later include
> +   support for cmov, but there is no one preprocessor define that determines
> +   this.  Assume that all processors that include sse also support cmov, so
> +   that we sorta future-proof this test against new preprocessor defines.  */
> +#include <cpuid.h>
> +#if (TCG_TARGET_REG_BITS == 64 \
> +     || defined(__i686__) || defined(__pentium4__) \
> +     || defined(__athlon__) || defined(__SSE__))
> +# define have_cmov 1
> +#else
> +static bool have_cmov;
> +#endif
> +
>  static uint8_t *tb_ret_addr;
>  
>  static void patch_reloc(uint8_t *code_ptr, int type,
> @@ -943,7 +957,14 @@ static void tcg_out_movcond32(TCGContext *s, TCGCond cond, TCGArg dest,
>                                TCGArg v1)
>  {
>      tcg_out_cmp(s, c1, c2, const_c2, 0);
> -    tcg_out_modrm(s, OPC_CMOVCC | tcg_cond_to_jcc[cond], dest, v1);
> +    if (have_cmov) {
> +        tcg_out_modrm(s, OPC_CMOVCC | tcg_cond_to_jcc[cond], dest, v1);
> +    } else {
> +        int over = gen_new_label();
> +        tcg_out_jxx(s, tcg_cond_to_jcc[tcg_invert_cond(cond)], over, 1);
> +        tcg_out_mov(s, TCG_TYPE_I32, dest, v1);
> +        tcg_out_label(s, over, s->code_ptr);
> +    }
>  }
>  
>  #if TCG_TARGET_REG_BITS == 64
> @@ -2243,6 +2264,17 @@ static void tcg_target_qemu_prologue(TCGContext *s)
>  
>  static void tcg_target_init(TCGContext *s)
>  {
> +    /* If we could not determine cmov availablity at compile time, perform
> +       the check at runtime.  99% certainty that we're running on hardware
> +       that supports cmov, but we still need to check.  In case cmov is not
> +       available, we'll use a small forward branch.  */
> +#ifndef have_cmov
> +    {
> +        unsigned a, b, c, d;
> +        have_cmov = (__get_cpuid(1, &a, &b, &c, &d) && (d & bit_CMOV));
> +    }
> +#endif
> +
>  #if !defined(CONFIG_USER_ONLY)
>      /* fail safe */
>      if ((1 << CPU_TLB_ENTRY_BITS) != sizeof(CPUTLBEntry))
> diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
> index dbc6756..450078b 100644
> --- a/tcg/i386/tcg-target.h
> +++ b/tcg/i386/tcg-target.h
> @@ -90,12 +90,7 @@ typedef enum {
>  #define TCG_TARGET_HAS_nand_i32         0
>  #define TCG_TARGET_HAS_nor_i32          0
>  #define TCG_TARGET_HAS_deposit_i32      1
> -#if defined(__x86_64__) || defined(__i686__)
> -/* Use cmov only if the compiler is already doing so.  */
>  #define TCG_TARGET_HAS_movcond_i32      1
> -#else
> -#define TCG_TARGET_HAS_movcond_i32      0
> -#endif
>  
>  #if TCG_TARGET_REG_BITS == 64
>  #define TCG_TARGET_HAS_div2_i64         1
> -- 
> 1.7.11.7
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] tcg-i386: Improve cmov detection

[Richard Henderson]

On 11/24/2012 10:12 AM, Peter Maydell wrote:
> MacOS gcc objects to this:
> In file included from /Users/pm215/src/qemu/tcg/tcg.c:174:
> /Users/pm215/src/qemu/tcg/i386/tcg-target.c:105:19: warning: cpuid.h:
> No such file or directory
> 
> (though for some reason not as a fatal error).

Bizzare.

Out of curiosity, does llvm ship a cpuid.h?  Or am I going to be
better off not relying on that header at all?


r~

Re: [Qemu-devel] [Qemu-trivial] [PATCH] qemu-timer: Don't use RDTSC on 386s and 486s

[Stefan Hajnoczi]

On Fri, Nov 23, 2012 at 03:12:50PM +0000, Peter Maydell wrote:
> Adjust the conditional which guards the implementation of
> cpu_get_real_ticks() via RDTSC, so that we don't try to use it
> on x86 CPUs which don't implement RDTSC.  Instead we will fall
> back to the no-cycle-counter-available default implementation.
> 
> Reported-by: Yurij Popov <oss@djphoenix.ru>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-timer.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-timer.h b/qemu-timer.h
> index da7e97c..e35f163 100644
> --- a/qemu-timer.h
> +++ b/qemu-timer.h
> @@ -169,7 +169,7 @@ static inline int64_t cpu_get_real_ticks(void)
>      return retval;
>  }
>  
> -#elif defined(__i386__)
> +#elif defined(__i586__)
>  
>  static inline int64_t cpu_get_real_ticks(void)
>  {
> -- 
> 1.7.9.5

Dropping this due to the issue with gcc __i586__ that has been
discussed.

Stefan

Re: [Qemu-devel] [PATCH] tcg-i386: Improve cmov detection

[陳韋任 (Wei-Ren Chen)]

On Mon, Nov 26, 2012 at 08:23:10AM -0800, Richard Henderson wrote:
> On 11/24/2012 10:12 AM, Peter Maydell wrote:
> > MacOS gcc objects to this:
> > In file included from /Users/pm215/src/qemu/tcg/tcg.c:174:
> > /Users/pm215/src/qemu/tcg/i386/tcg-target.c:105:19: warning: cpuid.h:
> > No such file or directory
> > 
> > (though for some reason not as a fatal error).
> 
> Bizzare.
> 
> Out of curiosity, does llvm ship a cpuid.h?  Or am I going to be
> better off not relying on that header at all?

  I don't think LLVM ship cpuid.h.

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj