From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUqWK-0000dA-QJ for qemu-devel@nongnu.org; Wed, 16 May 2012 22:32:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUqWH-0000OT-TQ for qemu-devel@nongnu.org; Wed, 16 May 2012 22:32:52 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:46876) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUqWH-0000OC-Mi for qemu-devel@nongnu.org; Wed, 16 May 2012 22:32:49 -0400 Received: by obbwd20 with SMTP id wd20so2217917obb.4 for ; Wed, 16 May 2012 19:32:48 -0700 (PDT) Message-ID: <4FB4634C.5060909@codemonkey.ws> Date: Wed, 16 May 2012 21:32:44 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <20120516205805.GY4373@otherpad.lan.raisama.net> In-Reply-To: <20120516205805.GY4373@otherpad.lan.raisama.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [QEMU 1.1 PATCH] Expose CPUID leaf 7 only for -cpu host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Gleb Natapov , kvm@vger.kernel.org, "Yang, Wei Y" , Marcelo Tosatti , qemu-devel@nongnu.org, Avi Kivity 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. > 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. 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 > --- > 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];