Patchwork [5/5] target-i386: fix kvm_default_features overwriting kvm_arch_get_supported_cpuid()

login
register
mail settings
Submitter Igor Mammedov
Date Feb. 5, 2013, 4:39 p.m.
Message ID <1360082364-12475-6-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/218317/
State New
Headers show

Comments

Igor Mammedov - Feb. 5, 2013, 4:39 p.m.
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(-)
Eduardo Habkost - Feb. 5, 2013, 7:04 p.m.
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
Igor Mammedov - Feb. 5, 2013, 7:47 p.m.
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
Eduardo Habkost - Feb. 5, 2013, 9:20 p.m.
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").

> [...]
Igor Mammedov - Feb. 6, 2013, 9:51 a.m.
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().

Patch

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 */