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

[Jan Kiszka]

On 2011-12-28 19:55, Liu, Jinsong wrote:
>>From 3a78adf8006ec6189bfe2f55f7ae213e75bf3815 Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <address@hidden>
> Date: Thu, 29 Dec 2011 05:28:12 +0800
> Subject: [PATCH 2/2] Expose tsc deadline timer cpuid to guest
> 
> Depend on several factors:
> 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.

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
- 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.

> +    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.

> +    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.

Jan

signature.asc
Description: OpenPGP digital signature