Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest

[Liu, Jinsong]

> This requires some logic change and then rewording:
> 
> - enable TSC deadline timer support by default if in-kernel irqchip is
>   used
> - disable it on user request via a cpu feature flag

Yes, the logic has been implemented by the former patch as:
+    if (env->tsc_deadline_timer_enabled) {     // user control, default is to 
authorize tsc deadline timer feature
+        if (kvm_irqchip_in_kernel() &&         // in-kerenl irqchip is used
+            kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
+            env->cpuid_ext_features |= CPUID_EXT_TSC_DEADLINE_TIMER;
+        }
+    }

> - disable it for older machine types (see below) by default
> 
> TSC deadline timer emulation in user space is a different story to be
> told once we have a patch for it.
> 
>> 
>> Signed-off-by: Liu, Jinsong <address@hidden> ---
>>  target-i386/cpu.h   |    2 ++
>>  target-i386/cpuid.c |    7 ++++++-
>>  target-i386/kvm.c   |   13 +++++++++++++
>>  3 files changed, 21 insertions(+), 1 deletions(-)
>> 
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 177d8aa..f2d0ad5 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -399,6 +399,7 @@
>>  #define CPUID_EXT_X2APIC   (1 << 21)
>>  #define CPUID_EXT_MOVBE    (1 << 22)
>>  #define CPUID_EXT_POPCNT   (1 << 23)
>> +#define CPUID_EXT_TSC_DEADLINE_TIMER (1 << 24)
>>  #define CPUID_EXT_XSAVE    (1 << 26)
>>  #define CPUID_EXT_OSXSAVE  (1 << 27)
>>  #define CPUID_EXT_HYPERVISOR  (1 << 31)
>> @@ -693,6 +694,7 @@ typedef struct CPUX86State {
>> 
>>      uint64_t tsc;
>>      uint64_t tsc_deadline;
>> +    bool tsc_deadline_timer_enabled;
>> 
>>      uint64_t mcg_status;
>>      uint64_t msr_ia32_misc_enable;
>> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
>> index 0b3af90..fe749e0 100644
>> --- a/target-i386/cpuid.c
>> +++ b/target-i386/cpuid.c
>> @@ -48,7 +48,7 @@ static const char *ext_feature_name[] = {
>>      "fma", "cx16", "xtpr", "pdcm",
>>      NULL, NULL, "dca", "sse4.1|sse4_1",
>>      "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
>> -    NULL, "aes", "xsave", "osxsave",
>> +    "tsc_deadline", "aes", "xsave", "osxsave",
>>      "avx", NULL, NULL, "hypervisor",
>>  };
>>  static const char *ext2_feature_name[] = {
>> @@ -225,6 +225,7 @@ typedef struct x86_def_t {
>>      int model;
>>      int stepping;
>>      int tsc_khz;
>> +    bool tsc_deadline_timer_enabled;
>>      uint32_t features, ext_features, ext2_features, ext3_features;
>>      uint32_t kvm_features, svm_features;
>>      uint32_t xlevel;
>> @@ -742,6 +743,9 @@ static int cpu_x86_find_by_name(x86_def_t
>>      *x86_cpu_def, const char *cpu_model) x86_cpu_def->ext3_features
>>      &= ~minus_ext3_features; x86_cpu_def->kvm_features &=
>>      ~minus_kvm_features; x86_cpu_def->svm_features &=
>> ~minus_svm_features; +    /* Defaultly user don't against
>> tsc_deadline_timer */ +    x86_cpu_def->tsc_deadline_timer_enabled =
>> +        !(minus_ext_features & CPUID_EXT_TSC_DEADLINE_TIMER);     
>>          if (check_cpuid) { if
>> (check_features_against_host(x86_cpu_def) && enforce_cpuid)         
>>      goto error; @@ -885,6 +889,7 @@ int cpu_x86_register
>>      (CPUX86State *env, const char *cpu_model)
>>      env->cpuid_ext4_features = def->ext4_features;
>> env->cpuid_xlevel2 = def->xlevel2; env->tsc_khz = def->tsc_khz; +   
>>          env->tsc_deadline_timer_enabled =
>>          def->tsc_deadline_timer_enabled;      if (!kvm_enabled()) {
>> env->cpuid_features &= TCG_FEATURES; env->cpuid_ext_features &=
>> TCG_EXT_FEATURES;  
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index d50de90..79baf0b 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -370,6 +370,19 @@ int kvm_arch_init_vcpu(CPUState *env)
>>      i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
>>      env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1,
>>      0, R_ECX); env->cpuid_ext_features |= i;
>> +    /*
>> +     * 1. Considering live migration, user enable/disable tsc
>> deadline timer; +     * 2. If guest use kvm apic and kvm emulate tsc
>> deadline timer, expose it; +     * 3. If in the future qemu support
>> tsc deadline timer emulation, +     *    and guest use qemu apic,
>> add cpuid exposing case then. +     */
> 
> See above. Also, I don't think this comment applies very well to this
> function.

Yes, the comment is indeed ambiguous. Would elaborate more clear.

> 
>> +    env->cpuid_ext_features &= ~CPUID_EXT_TSC_DEADLINE_TIMER;
> 
> Can that feature possibly be set in cpuid_ext_features? I thought the
> kernel now refrains from this.

Yes, it's possible. Kernel didn't refrain it, just let qemu to make decision.

> 
>> +    if (env->tsc_deadline_timer_enabled) {
>> +        if (kvm_irqchip_in_kernel() &&
>> +            kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
>> +            env->cpuid_ext_features |=
>> CPUID_EXT_TSC_DEADLINE_TIMER; +        } +    }
>> 
>>      env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s,
>>                                                              
>> 0x80000001, 0, R_EDX); 
> 
> Sorry, it remains bogus to expose the tsc deadline timer feature on
> machines < pc-1.1. That's just like we introduced kvmclock only to
> pc-0.14 onward. The reason is that guest OSes so far running on
> qemu-1.0 or older without deadline timer support must not find that
> feature when being migrated to a host with qemu-1.1 in pc-1.0 compat
> mode. Yes, the user can explicitly disable it, but that is not the
> idea of legacy machine models. They should provide the very same
> environment that older qemu versions offered.
> 

Not quite clear about this point.
Per my understanding, if a kvm guest running on an older qemu without tsc 
deadline timer support, 
then after migrate, the guest would still cannot find tsc deadline feature, no 
matter older or newer host/qemu/pc-xx it migrate to.

Thanks,
Jinsong