Re: [Qemu-devel] target-i386: Allow tsc-frequency to be larger then 2.147G

Re: [Qemu-devel] target-i386: Allow tsc-frequency to be larger then 2.147G

[Michael Tokarev]

This is a follow-up to a more-or-less trivial commit,
2e84849aa2cc7f220d3b3668f5f7e3c57bb1b590 .  I'm adding
some more context - the whole function in question.


commit 2e84849aa2cc7f220d3b3668f5f7e3c57bb1b590
Author: Don Slutz <Don@CloudSwitch.com>
Date:   Fri Sep 21 20:13:13 2012 -0400

    target-i386: Allow tsc-frequency to be larger then 2.147G

    The check using INT_MAX (2147483647) is wrong in this case.

    Signed-off-by: Fred Oliveira <foliveira@cloudswitch.com>
    Signed-off-by: Don Slutz <Don@CloudSwitch.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 423e009..cbc172e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -846,7 +846,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
 {
     X86CPU *cpu = X86_CPU(obj);
     const int64_t min = 0;
-    const int64_t max = INT_MAX;
+    const int64_t max = INT64_MAX;
     int64_t value;

     visit_type_int(v, &value, name, errp);
     if (error_is_set(errp)) {
         return;
     }
     if (value < min || value > max) {
         error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
                   name ? name : "null", value, min, max);
         return;
     }

     cpu->env.tsc_khz = value / 1000;
 }

The patch makes the second test (if value > max)
to be a no-op, since value is of type int64_t,
and max is now INT64_MAX, so value can never be
larger than max.  Overflow can be catched by the
first test (value < 0).

Note this function has another defect: the tsc
frequency is truncated to KHz.  It's okay when
it is called from the default cpu init function,
where the initial value is in khz and is multiplied
by 1000 when calling x86_cpuid_set_tsc_freq(),
but not okay when called as a handler for user-
defined option, like -cpu foo,tsc_frequency=bar.

I'm not sure how often this option is used, however.

Thanks,

/mjt

Re: [Qemu-devel] target-i386: Allow tsc-frequency to be larger then 2.147G

[Don Slutz]

On 12/16/12 12:06, Michael Tokarev wrote:
> This is a follow-up to a more-or-less trivial commit,
> 2e84849aa2cc7f220d3b3668f5f7e3c57bb1b590 .  I'm adding
> some more context - the whole function in question.
>
>
> commit 2e84849aa2cc7f220d3b3668f5f7e3c57bb1b590
> Author: Don Slutz <Don@CloudSwitch.com>
> Date:   Fri Sep 21 20:13:13 2012 -0400
>
>      target-i386: Allow tsc-frequency to be larger then 2.147G
>
>      The check using INT_MAX (2147483647) is wrong in this case.
>
>      Signed-off-by: Fred Oliveira <foliveira@cloudswitch.com>
>      Signed-off-by: Don Slutz <Don@CloudSwitch.com>
>      Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 423e009..cbc172e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -846,7 +846,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>   {
>       X86CPU *cpu = X86_CPU(obj);
>       const int64_t min = 0;
> -    const int64_t max = INT_MAX;
> +    const int64_t max = INT64_MAX;
>       int64_t value;
>
>       visit_type_int(v, &value, name, errp);
>       if (error_is_set(errp)) {
>           return;
>       }
>       if (value < min || value > max) {
>           error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
>                     name ? name : "null", value, min, max);
>           return;
>       }
>
>       cpu->env.tsc_khz = value / 1000;
>   }
>
> The patch makes the second test (if value > max)
> to be a no-op, since value is of type int64_t,
> and max is now INT64_MAX, so value can never be
> larger than max.  Overflow can be catched by the
> first test (value < 0).
You are right, and the 2nd test could be removed.  I was going for the 
simplest change.  Current when not using kvm, this value makes no 
difference.  Based on " 60-100 MHz P5 Pentiums" as being the 1st real 
processors that have rdtsc, the min should be 60M.  I am not sure what a 
real max would be, but a value of 30G would appear to not be that bad.  
However there are use cases where you might want to set a non "real" value.


> Note this function has another defect: the tsc
> frequency is truncated to KHz.  It's okay when
> it is called from the default cpu init function,
> where the initial value is in khz and is multiplied
> by 1000 when calling x86_cpuid_set_tsc_freq(),
> but not okay when called as a handler for user-
> defined option, like -cpu foo,tsc_frequency=bar.
Since the only current use is:
           kvm_vcpu_ioctl(env, KVM_SET_TSC_KHZ, env->tsc_khz)
Which is ok for the current code since it also uses just the khz value.

One of my pending patches

https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02269.html

Does allow the guest access to this value also only needs the khz 
value.  However the patch:

https://lists.gnu.org/archive/html/qemu-devel/2012-08/msg05198.html

Does allow access to the missing data.

What this function looks like in the near term is not clear as the next 
steps in getting QEMU's cpu code converted to QOM are in progress and 
yet to be accepted.  Based on this I am waiting to update the above 
patches till the rate of change to QEMU code in this area is more stable.




> I'm not sure how often this option is used, however.
My quick testing on kvm appeared to not do anything different based on 
this value. So I am assuming that no one is using this option currently.
> Thanks,
>
> /mjt
    -Don Slutz