| Message ID | 20120516205805.GY4373@otherpad.lan.raisama.net |
|---|---|
| State | New |
| Headers | show |
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<ehabkost@redhat.com> > --- > 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];
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<ehabkost@redhat.com> > >--- > > 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]; >
On Wed, May 16, 2012 at 05:58:05PM -0300, Eduardo Habkost wrote: [...] > @@ -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; > + } > + Ouch: it looks like -cpu host isn't using GET_SUPPORTED_CPUID at all, host_cpuid() is a raw cpuid instruction. I am surprised -cpu host works at all (probably it breaks in interesting ways if running on a new CPU and a not-very-new kernel). I will send v2 of this patch, to use GET_SUPPORTED_CPUID at least for cpuid_7_0_ebx_features. It is still a bug to not filter the remaining CPUID leaves using GET_SUPPORTED_CPUID, but I am not sure we would have enough time for testing if we try to fix all that in 1.1.
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];
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 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. 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 <ehabkost@redhat.com> --- target-i386/cpu.c | 24 +++++++++++++++++------- target-i386/cpu.h | 2 ++ 2 files changed, 19 insertions(+), 7 deletions(-)