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

[Qemu-trivial] [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-trivial] [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-trivial] [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-trivial] [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-trivial] [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-trivial] [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-trivial] [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-trivial] [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

Re: [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