| Message ID | 1360082364-12475-6-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 05, 2013 at 07:46:09PM +0100, Igor Mammedov wrote: > On Tue, 5 Feb 2013 15:53:04 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Feb 05, 2013 at 05:39:24PM +0100, Igor Mammedov wrote: > > > ORing kvm_default_features to def->kvm_features might set unsupported > > > bits if kvm_arch_get_supported_cpuid() returns less bits set than in > > > kvm_default_features. Fix it by removing kvm_default_features and using > > > only what kvm_arch_get_supported_cpuid() returns. > > > > This is exactly what we _must not_ do! We can't change CPUID bits in a > > machine-type or we will change the guest ABI. We must keep them stable > > for a machine-type and: > > > > * Not change if QEMU is upgraded; > > * Not change if the host kernel is upgraded; > > * Not change if running on different host hardware. > Is all of this applicable to CPU 'host' or only to built-in cpu_models? This applies only to the built-in CPU models and the stable (versioned) machine-types. -cpu host is special and is expected to break the rules above. We could also create a "pc-next" machine-type where those rules could be broken. > > > > > The only way we are allowed to change the guest ABI is on a new > > machine-type. This was the whole point of the bug fix at commit > > ef8621b1a3, and this patch reintroduces the bug. > > > > If you are starting QEMU in a host that doesn't support the set of KVM > > features QEMU is configured to expose (through the command-line or by > > choosing a machine-type), it _should_ fail to start if using -cpu > > enforce. This is a feature. > > > > We could enable this behavior on the "pc-next" machine, if we create it > > one day, but not on the stable machine-types. > > > > > > > > > > Mark disable_kvm_pv_eoi() to be removed in favor of compat_props > > > when available. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > target-i386/cpu.c | 20 ++++++++------------ > > > target-i386/cpu.h | 2 ++ > > > 2 files changed, 10 insertions(+), 12 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 62fdc84..e66362e 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -220,17 +220,12 @@ typedef struct model_features_t { > > > int check_cpuid = 0; > > > int enforce_cpuid = 0; > > > > > > -static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > > > - (1 << KVM_FEATURE_NOP_IO_DELAY) | > > > - (1 << KVM_FEATURE_CLOCKSOURCE2) | > > > - (1 << KVM_FEATURE_ASYNC_PF) | > > > - (1 << KVM_FEATURE_STEAL_TIME) | > > > - (1 << KVM_FEATURE_PV_EOI) | > > > - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > > > - > > > +/* TODO: replace it with compat properties when feature bits > > > + * are converted into static properties */ > > > +bool x86_cpu_no_pv_eoi; > > > void disable_kvm_pv_eoi(void) > > > { > > > - kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI); > > > + x86_cpu_no_pv_eoi = true; > > > } > > > > > > void host_cpuid(uint32_t function, uint32_t count, > > > @@ -2061,9 +2056,6 @@ static void x86_cpu_initfn(Object *obj) > > > env->cpuid_ext3_features = def->ext3_features; > > > object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL); > > > env->cpuid_kvm_features = def->kvm_features; > > > - if (kvm_enabled()) { > > > - env->cpuid_kvm_features |= kvm_default_features; > > > - } > > > env->cpuid_svm_features = def->svm_features; > > > env->cpuid_ext4_features = def->ext4_features; > > > env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features; > > > @@ -2107,6 +2099,10 @@ static void x86_cpu_def_class_init(ObjectClass *oc, void *data) > > > */ > > > memcpy(xcc->info.vendor, hostcc->info.vendor, > > > sizeof(xcc->info.vendor)); > > > + xcc->info.kvm_features = xcc->info.kvm_features; > > > + if (x86_cpu_no_pv_eoi) { > > > + xcc->info.kvm_features &= ~(1UL << KVM_FEATURE_PV_EOI); > > > + } > > > } > > > > > > /* Look for specific models that have the QEMU version in .model_id */ > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > > index 11ef942..51f20b6 100644 > > > --- a/target-i386/cpu.h > > > +++ b/target-i386/cpu.h > > > @@ -1246,6 +1246,8 @@ void do_smm_enter(CPUX86State *env1); > > > > > > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); > > > > > > +/* TODO: convert to compat_props */ > > > +extern bool x86_cpu_no_pv_eoi; > > > void disable_kvm_pv_eoi(void); > > > > > > /* Return name of 32-bit register, from a R_* constant */ > > > -- > > > 1.7.1 > > > > > > > -- > > Eduardo > > > -- > Regards, > Igor
On Tue, 5 Feb 2013 17:04:26 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 05, 2013 at 07:46:09PM +0100, Igor Mammedov wrote: > > On Tue, 5 Feb 2013 15:53:04 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Feb 05, 2013 at 05:39:24PM +0100, Igor Mammedov wrote: > > > > ORing kvm_default_features to def->kvm_features might set unsupported > > > > bits if kvm_arch_get_supported_cpuid() returns less bits set than in > > > > kvm_default_features. Fix it by removing kvm_default_features and using > > > > only what kvm_arch_get_supported_cpuid() returns. > > > > > > This is exactly what we _must not_ do! We can't change CPUID bits in a > > > machine-type or we will change the guest ABI. We must keep them stable > > > for a machine-type and: > > > > > > * Not change if QEMU is upgraded; > > > * Not change if the host kernel is upgraded; > > > * Not change if running on different host hardware. > > Is all of this applicable to CPU 'host' or only to built-in cpu_models? > > This applies only to the built-in CPU models and the stable (versioned) > machine-types. > > -cpu host is special and is expected to break the rules above. We could > also create a "pc-next" machine-type where those rules could be broken. Isn't default machine like pc-next? > > > > > > > > > > The only way we are allowed to change the guest ABI is on a new > > > machine-type. This was the whole point of the bug fix at commit > > > ef8621b1a3, and this patch reintroduces the bug. > > > > > > If you are starting QEMU in a host that doesn't support the set of KVM > > > features QEMU is configured to expose (through the command-line or by > > > choosing a machine-type), it _should_ fail to start if using -cpu > > > enforce. This is a feature. > > > > > > We could enable this behavior on the "pc-next" machine, if we create it > > > one day, but not on the stable machine-types. > > > > > > > > > > > > > > Mark disable_kvm_pv_eoi() to be removed in favor of compat_props > > > > when available. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > target-i386/cpu.c | 20 ++++++++------------ > > > > target-i386/cpu.h | 2 ++ > > > > 2 files changed, 10 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > index 62fdc84..e66362e 100644 > > > > --- a/target-i386/cpu.c > > > > +++ b/target-i386/cpu.c > > > > @@ -220,17 +220,12 @@ typedef struct model_features_t { > > > > int check_cpuid = 0; > > > > int enforce_cpuid = 0; > > > > > > > > -static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > > > > - (1 << KVM_FEATURE_NOP_IO_DELAY) | > > > > - (1 << KVM_FEATURE_CLOCKSOURCE2) | > > > > - (1 << KVM_FEATURE_ASYNC_PF) | > > > > - (1 << KVM_FEATURE_STEAL_TIME) | > > > > - (1 << KVM_FEATURE_PV_EOI) | > > > > - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > > > > - > > > > +/* TODO: replace it with compat properties when feature bits > > > > + * are converted into static properties */ > > > > +bool x86_cpu_no_pv_eoi; > > > > void disable_kvm_pv_eoi(void) > > > > { > > > > - kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI); > > > > + x86_cpu_no_pv_eoi = true; > > > > } > > > > > > > > void host_cpuid(uint32_t function, uint32_t count, > > > > @@ -2061,9 +2056,6 @@ static void x86_cpu_initfn(Object *obj) > > > > env->cpuid_ext3_features = def->ext3_features; > > > > object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL); > > > > env->cpuid_kvm_features = def->kvm_features; > > > > - if (kvm_enabled()) { > > > > - env->cpuid_kvm_features |= kvm_default_features; > > > > - } > > > > env->cpuid_svm_features = def->svm_features; > > > > env->cpuid_ext4_features = def->ext4_features; > > > > env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features; > > > > @@ -2107,6 +2099,10 @@ static void x86_cpu_def_class_init(ObjectClass *oc, void *data) > > > > */ > > > > memcpy(xcc->info.vendor, hostcc->info.vendor, > > > > sizeof(xcc->info.vendor)); > > > > + xcc->info.kvm_features = xcc->info.kvm_features; > > > > + if (x86_cpu_no_pv_eoi) { > > > > + xcc->info.kvm_features &= ~(1UL << KVM_FEATURE_PV_EOI); > > > > + } > > > > } > > > > > > > > /* Look for specific models that have the QEMU version in .model_id */ > > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > > > index 11ef942..51f20b6 100644 > > > > --- a/target-i386/cpu.h > > > > +++ b/target-i386/cpu.h > > > > @@ -1246,6 +1246,8 @@ void do_smm_enter(CPUX86State *env1); > > > > > > > > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); > > > > > > > > +/* TODO: convert to compat_props */ > > > > +extern bool x86_cpu_no_pv_eoi; > > > > void disable_kvm_pv_eoi(void); > > > > > > > > /* Return name of 32-bit register, from a R_* constant */ > > > > -- > > > > 1.7.1 > > > > > > > > > > -- > > > Eduardo > > > > > > -- > > Regards, > > Igor > > -- > Eduardo
On Tue, Feb 05, 2013 at 08:47:29PM +0100, Igor Mammedov wrote: > On Tue, 5 Feb 2013 17:04:26 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Feb 05, 2013 at 07:46:09PM +0100, Igor Mammedov wrote: > > > On Tue, 5 Feb 2013 15:53:04 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Tue, Feb 05, 2013 at 05:39:24PM +0100, Igor Mammedov wrote: > > > > > ORing kvm_default_features to def->kvm_features might set unsupported > > > > > bits if kvm_arch_get_supported_cpuid() returns less bits set than in > > > > > kvm_default_features. Fix it by removing kvm_default_features and using > > > > > only what kvm_arch_get_supported_cpuid() returns. > > > > > > > > This is exactly what we _must not_ do! We can't change CPUID bits in a > > > > machine-type or we will change the guest ABI. We must keep them stable > > > > for a machine-type and: > > > > > > > > * Not change if QEMU is upgraded; > > > > * Not change if the host kernel is upgraded; > > > > * Not change if running on different host hardware. > > > Is all of this applicable to CPU 'host' or only to built-in cpu_models? > > > > This applies only to the built-in CPU models and the stable (versioned) > > machine-types. > > > > -cpu host is special and is expected to break the rules above. We could > > also create a "pc-next" machine-type where those rules could be broken. > Isn't default machine like pc-next? No, the default machine is versioned (today it is "pc-i440fx-1.4"). > [...]
On Tue, 5 Feb 2013 19:20:04 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Feb 05, 2013 at 08:47:29PM +0100, Igor Mammedov wrote: > > On Tue, 5 Feb 2013 17:04:26 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Feb 05, 2013 at 07:46:09PM +0100, Igor Mammedov wrote: > > > > On Tue, 5 Feb 2013 15:53:04 -0200 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Tue, Feb 05, 2013 at 05:39:24PM +0100, Igor Mammedov wrote: > > > > > > ORing kvm_default_features to def->kvm_features might set > > > > > > unsupported bits if kvm_arch_get_supported_cpuid() returns less > > > > > > bits set than in kvm_default_features. Fix it by removing > > > > > > kvm_default_features and using only what > > > > > > kvm_arch_get_supported_cpuid() returns. > > > > > > > > > > This is exactly what we _must not_ do! We can't change CPUID bits > > > > > in a machine-type or we will change the guest ABI. We must keep > > > > > them stable for a machine-type and: > > > > > > > > > > * Not change if QEMU is upgraded; > > > > > * Not change if the host kernel is upgraded; > > > > > * Not change if running on different host hardware. > > > > Is all of this applicable to CPU 'host' or only to built-in > > > > cpu_models? > > > > > > This applies only to the built-in CPU models and the stable (versioned) > > > machine-types. > > > > > > -cpu host is special and is expected to break the rules above. We could > > > also create a "pc-next" machine-type where those rules could be broken. > > Isn't default machine like pc-next? > > No, the default machine is versioned (today it is "pc-i440fx-1.4"). > > > [...] > Ok, lets drop this patch. I'll prepare another one that moves default setting to class_init().
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 62fdc84..e66362e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -220,17 +220,12 @@ typedef struct model_features_t { int check_cpuid = 0; int enforce_cpuid = 0; -static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | - (1 << KVM_FEATURE_NOP_IO_DELAY) | - (1 << KVM_FEATURE_CLOCKSOURCE2) | - (1 << KVM_FEATURE_ASYNC_PF) | - (1 << KVM_FEATURE_STEAL_TIME) | - (1 << KVM_FEATURE_PV_EOI) | - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); - +/* TODO: replace it with compat properties when feature bits + * are converted into static properties */ +bool x86_cpu_no_pv_eoi; void disable_kvm_pv_eoi(void) { - kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI); + x86_cpu_no_pv_eoi = true; } void host_cpuid(uint32_t function, uint32_t count, @@ -2061,9 +2056,6 @@ static void x86_cpu_initfn(Object *obj) env->cpuid_ext3_features = def->ext3_features; object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL); env->cpuid_kvm_features = def->kvm_features; - if (kvm_enabled()) { - env->cpuid_kvm_features |= kvm_default_features; - } env->cpuid_svm_features = def->svm_features; env->cpuid_ext4_features = def->ext4_features; env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features; @@ -2107,6 +2099,10 @@ static void x86_cpu_def_class_init(ObjectClass *oc, void *data) */ memcpy(xcc->info.vendor, hostcc->info.vendor, sizeof(xcc->info.vendor)); + xcc->info.kvm_features = xcc->info.kvm_features; + if (x86_cpu_no_pv_eoi) { + xcc->info.kvm_features &= ~(1UL << KVM_FEATURE_PV_EOI); + } } /* Look for specific models that have the QEMU version in .model_id */ diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 11ef942..51f20b6 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1246,6 +1246,8 @@ void do_smm_enter(CPUX86State *env1); void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); +/* TODO: convert to compat_props */ +extern bool x86_cpu_no_pv_eoi; void disable_kvm_pv_eoi(void); /* Return name of 32-bit register, from a R_* constant */
ORing kvm_default_features to def->kvm_features might set unsupported bits if kvm_arch_get_supported_cpuid() returns less bits set than in kvm_default_features. Fix it by removing kvm_default_features and using only what kvm_arch_get_supported_cpuid() returns. Mark disable_kvm_pv_eoi() to be removed in favor of compat_props when available. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 20 ++++++++------------ target-i386/cpu.h | 2 ++ 2 files changed, 10 insertions(+), 12 deletions(-)