> 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 <jinsong.liu@xxxxxxxxx> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
