Re: [Qemu-devel] [QEMU 1.1 PATCH v3] Expose CPUID leaf 7 only for -cpu host

[Gleb Natapov]

On Mon, May 21, 2012 at 11:27:02AM -0300, Eduardo Habkost wrote:
> Changes v2 -> v3;
>   - Check for kvm_enabled() before setting cpuid_7_0_ebx_features
> 
> Changes v1 -> v2:
>   - Use kvm_arch_get_supported_cpuid() instead of host_cpuid() on
>     cpu_x86_fill_host().
> 
>   We should use GET_SUPPORTED_CPUID for all bits on "-cpu host"
>   eventually, but I am not changing all the other CPUID leaves because
>   we may not be able to test such an intrusive change in time for 1.1.
> 
> Description of the bug:
> 
> Since QEMU 0.15, the CPUID information on CPUID[EAX=7,ECX=0] is being
> returned unfiltered to the guest, directly from the GET_SUPPORTED_CPUID
> return value.
> 
> The problem is that this makes the resulting CPU feature flags
> unpredictable and dependent on the host CPU and kernel version. This
> breaks live-migration badly if migrating from a host CPU that supports
> some features on that CPUID leaf (running a recent kernel) to a kernel
> or host CPU that doesn't support it.
> 
> Migration also is incorrect (the virtual CPU changes under the guest's
> feet) if you migrate in the opposite direction (from an old CPU/kernel
> to a new CPU/kernel), but with less serious consequences (guests
> normally query CPUID information only once on boot).
> 
> Fortunately, the bug affects only users using cpudefs with level >= 7.
> 
> The right behavior should be to explicitly enable those features on
> [cpudef] config sections or on the "-cpu" command-line arguments. Right
> now there is no predefined CPU model on QEMU that has those features:
> the latest Intel model we have is Sandy Bridge.
> 
> I would like to get this fixed on 1.1, so I am submitting this patch,
> that enables those features only if "-cpu host" is being used (as we
> don't have any pre-defined CPU model that actually have those features).
> After 1.1 is released, we can make those features properly configurable
> on [cpudef] and -cpu configuration.
> 
> One problem is: with this patch, users with the following setup:
> - Running QEMU 1.0;
> - Using a cpudef having level >= 7;
> - Running a kernel that supports the features on CPUID leaf 7; and
> - Running on a CPU that supports some features on CPUID leaf 7
> won't be able to live-migrate to QEMU 1.1. But for these users
> live-migration is already broken (they can't live-migrate to hosts with
> older CPUs or older kernels, already), I don't see how to avoid this
> problem.
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
Looks good to me.

> ---
>  target-i386/cpu.c |   22 +++++++++++++++-------
>  target-i386/cpu.h |    2 ++
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 89b4ac7..388bc5c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -238,6 +238,8 @@ typedef struct x86_def_t {
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;
> +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
> +    uint32_t cpuid_7_0_ebx_features;
>  } x86_def_t;
>  
>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
> @@ -521,6 +523,12 @@ static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
>      x86_cpu_def->ext_features = ecx;
>      x86_cpu_def->features = edx;
>  
> +    if (kvm_enabled() && x86_cpu_def->level >= 7) {
> +        x86_cpu_def->cpuid_7_0_ebx_features = 
> kvm_arch_get_supported_cpuid(kvm_state, 0x7, 0, R_EBX);
> +    } else {
> +        x86_cpu_def->cpuid_7_0_ebx_features = 0;
> +    }
> +
>      host_cpuid(0x80000000, 0, &eax, &ebx, &ecx, &edx);
>      x86_cpu_def->xlevel = eax;
>  
> @@ -1185,6 +1193,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      env->cpuid_kvm_features = def->kvm_features;
>      env->cpuid_svm_features = def->svm_features;
>      env->cpuid_ext4_features = def->ext4_features;
> +    env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
>      env->cpuid_xlevel2 = def->xlevel2;
>      object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
>                              "tsc-frequency", &error);
> @@ -1451,13 +1460,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>          *edx = 0;
>          break;
>      case 7:
> -        if (kvm_enabled()) {
> -            KVMState *s = env->kvm_state;
> -
> -            *eax = kvm_arch_get_supported_cpuid(s, 0x7, count, R_EAX);
> -            *ebx = kvm_arch_get_supported_cpuid(s, 0x7, count, R_EBX);
> -            *ecx = kvm_arch_get_supported_cpuid(s, 0x7, count, R_ECX);
> -            *edx = kvm_arch_get_supported_cpuid(s, 0x7, count, R_EDX);
> +        /* Structured Extended Feature Flags Enumeration Leaf */
> +        if (count == 0) {
> +            *eax = 0; /* Maximum ECX value for sub-leaves */
> +            *ebx = env->cpuid_7_0_ebx; /* Feature flags */
> +            *ecx = 0; /* Reserved */
> +            *edx = 0; /* Reserved */
>          } else {
>              *eax = 0;
>              *ebx = 0;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index b5b9a50..2460f63 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -741,6 +741,8 @@ typedef struct CPUX86State {
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t cpuid_xlevel2;
>      uint32_t cpuid_ext4_features;
> +    /* Flags from CPUID[EAX=7,ECX=0].EBX */
> +    uint32_t cpuid_7_0_ebx;
>  
>      /* MTRRs */
>      uint64_t mtrr_fixed[11];
> -- 
> 1.7.3.2
> 
> 
> -- 
> Eduardo

--
                        Gleb.