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

[Eduardo Habkost]

On Wed, May 16, 2012 at 09:32:44PM -0500, Anthony Liguori wrote:
> On 05/16/2012 03:58 PM, Eduardo Habkost wrote:
> >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.
> 
> But isn't this exactly what you get with -cpu host?  I mean, the
> whole point is that you're getting whatever the best set of flags
> that can be exposed by the hardware.

Exactly, this is the point of -cpu host, but not when _not_ using CPU
host. This happens unconditionally today, for all users.

> 
> >This
> >breaks live-migration badly if you try to migrate from a host CPU that
> >supports some features on that CPUID leaf (running a recent kenrel) to a
> >kernel or host CPU that doesn't support it.
> 
> This seems like expected behavior to me.

I am not sure I understand your point. This is expected behavior only
for -cpu host, and this is what this patch does: changes the code so
that this behavior is enabled only if using -cpu host.

> 
> Regards,
> 
> Anthony Liguori
> 
> >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; and
> >- Running a kernel that supports the features on CPUID leaf 7;
> >- 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>
> >---
> >  target-i386/cpu.c |   24 +++++++++++++++++-------
> >  target-i386/cpu.h |    2 ++
> >  2 files changed, 19 insertions(+), 7 deletions(-)
> >
> >diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >index 89b4ac7..c75fa3d 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,13 @@ static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
> >      x86_cpu_def->ext_features = ecx;
> >      x86_cpu_def->features = edx;
> >
> >+    if (x86_cpu_def->level>= 7) {
> >+        host_cpuid(0x7, 0,&eax,&ebx,&ecx,&edx);
> >+        x86_cpu_def->cpuid_7_0_ebx_features = 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 +1194,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 +1461,13 @@ 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];
> 

-- 
Eduardo