[Qemu-devel] [PATCH qom-cpu 0/7] disable kvm_mmu + -cpu "enforce" fixes (v3)

[Qemu-devel] [PATCH qom-cpu 0/7] disable kvm_mmu + -cpu "enforce" fixes (v3)

[Eduardo Habkost]

Changes on v3:
 - Patches 3-9 from v2 are now already on qom-cpu tree
 - Remove CONFIG_KVM #ifdefs by declaring fake KVM_* #defines on sysemu/kvm.h
 - Refactor code that uses the feature word arrays
   (to make it easier to add a new feature name array)
 - Add feature name array for CPUID leaf 0xC0000001

Changes on v2:
 - Now both the kvm_mmu-disable and -cpu "enforce" changes are on the same
   series
 - Coding style fixes

Git tree for reference:
  git://github.com/ehabkost/qemu-hacks.git cpu-enforce-all.v3
  https://github.com/ehabkost/qemu-hacks/tree/cpu-enforce-all.v3

The changes are a bit intrusive, but:

 - The longer we take to make "enforce" strict as it should (and make libvirt
   finally use it), more users will have VMs with migration-unsafe unpredictable
   guest ABIs. For this reason, I would like to get this into QEMU 1.4.
 - The changes in this series should affect only users that are already using
   the "enforce" flag, and I believe whoever is using the "enforce" flag really
   want the strict behavior introduced by this series.



Eduardo Habkost (7):
  kvm: Add fake KVM constants to avoid #ifdefs on KVM-specific code
  target-i386: Don't set any KVM flag by default if KVM is disabled
  target-i386: Disable kvm_mmu by default
  target-i386/cpu: Introduce FeatureWord typedefs
  target-i386: kvm_check_features_against_host(): Use feature_word_info
  target-i386/cpu.c: Add feature name array for ext4_features
  target-i386: check/enforce: Check all feature words

 include/sysemu/kvm.h |  14 ++++
 target-i386/cpu.c    | 193 ++++++++++++++++++++++++++++++++-------------------
 target-i386/cpu.h    |  15 ++++
 3 files changed, 150 insertions(+), 72 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH qom-cpu 1/7] kvm: Add fake KVM constants to avoid #ifdefs on KVM-specific code

[Eduardo Habkost]

Any KVM-specific code that use these constants must check if
kvm_enabled() is true before using them.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
---
 include/sysemu/kvm.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 3db19ff..15f9658 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -21,6 +21,20 @@
 #ifdef CONFIG_KVM
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
+#else
+/* These constants must never be used at runtime if kvm_enabled() is false.
+ * They exist so we don't need #ifdefs around KVM-specific code that already
+ * checks kvm_enabled() properly.
+ */
+#define KVM_CPUID_SIGNATURE      0
+#define KVM_CPUID_FEATURES       0
+#define KVM_FEATURE_CLOCKSOURCE  0
+#define KVM_FEATURE_NOP_IO_DELAY 0
+#define KVM_FEATURE_MMU_OP       0
+#define KVM_FEATURE_CLOCKSOURCE2 0
+#define KVM_FEATURE_ASYNC_PF     0
+#define KVM_FEATURE_STEAL_TIME   0
+#define KVM_FEATURE_PV_EOI       0
 #endif
 
 extern int kvm_allowed;
-- 
1.7.11.7

[Qemu-devel] [PATCH qom-cpu 2/7] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

This is a cleanup that tries to solve two small issues:

 - We don't need a separate kvm_pv_eoi_features variable just to keep a
   constant calculated at compile-time, and this style would require
   adding a separate variable (that's declared twice because of the
   CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
   by machine-type compat code.
 - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
   even when KVM is disabled at runtime. This small incosistency in
   the cpuid_kvm_features field isn't a problem today because
   cpuid_kvm_features is ignored by the TCG code, but it may cause
   unexpected problems later when refactoring the CPUID handling code.

This patch eliminates the kvm_pv_eoi_features variable and simply uses
kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
behavior of enable_kvm_pv_eoi() clearer and easier to understand.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Changes v2:
 - Coding style fix

Changes v3:
 - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
---
 target-i386/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 951e206..40400ac 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -164,15 +164,15 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_ASYNC_PF) |
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
 #else
 static uint32_t kvm_default_features = 0;
-static const uint32_t kvm_pv_eoi_features = 0;
 #endif
 
 void enable_kvm_pv_eoi(void)
 {
-    kvm_default_features |= kvm_pv_eoi_features;
+    if (kvm_enabled()) {
+        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
+    }
 }
 
 void host_cpuid(uint32_t function, uint32_t count,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH qom-cpu 2/7] target-i386: Don't set any KVM flag by default if KVM is disabled

[Igor Mammedov]

On Mon,  7 Jan 2013 16:20:43 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This is a cleanup that tries to solve two small issues:
> 
>  - We don't need a separate kvm_pv_eoi_features variable just to keep a
>    constant calculated at compile-time, and this style would require
>    adding a separate variable (that's declared twice because of the
>    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
>    by machine-type compat code.
>  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
>    even when KVM is disabled at runtime. This small incosistency in
>    the cpuid_kvm_features field isn't a problem today because
>    cpuid_kvm_features is ignored by the TCG code, but it may cause
>    unexpected problems later when refactoring the CPUID handling code.
> 
> This patch eliminates the kvm_pv_eoi_features variable and simply uses
> kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> behavior of enable_kvm_pv_eoi() clearer and easier to understand.

Subj doesn't match what patch actually does.
Have you meant "Don't set kvm_pv_eoi flag by default if KVM is disabled"?
Although "eliminate kvm_pv_eoi_features variable" might better describe what
patch is doing.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Coding style fix
> 
> Changes v3:
>  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> ---
>  target-i386/cpu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 951e206..40400ac 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -164,15 +164,15 @@ static uint32_t kvm_default_features = (1 <<
> KVM_FEATURE_CLOCKSOURCE) | (1 << KVM_FEATURE_ASYNC_PF) |
>          (1 << KVM_FEATURE_STEAL_TIME) |
>          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
>  #else
>  static uint32_t kvm_default_features = 0;
> -static const uint32_t kvm_pv_eoi_features = 0;
>  #endif
>  
>  void enable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= kvm_pv_eoi_features;
> +    if (kvm_enabled()) {
> +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> +    }
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,

Re: [Qemu-devel] [PATCH qom-cpu 2/7] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

On Wed, Jan 09, 2013 at 10:46:12AM +0100, Igor Mammedov wrote:
> On Mon,  7 Jan 2013 16:20:43 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This is a cleanup that tries to solve two small issues:
> > 
> >  - We don't need a separate kvm_pv_eoi_features variable just to keep a
> >    constant calculated at compile-time, and this style would require
> >    adding a separate variable (that's declared twice because of the
> >    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
> >    by machine-type compat code.
> >  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
> >    even when KVM is disabled at runtime. This small incosistency in
> >    the cpuid_kvm_features field isn't a problem today because
> >    cpuid_kvm_features is ignored by the TCG code, but it may cause
> >    unexpected problems later when refactoring the CPUID handling code.
> > 
> > This patch eliminates the kvm_pv_eoi_features variable and simply uses
> > kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> > enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> > behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> 
> Subj doesn't match what patch actually does.
> Have you meant "Don't set kvm_pv_eoi flag by default if KVM is disabled"?
> Although "eliminate kvm_pv_eoi_features variable" might better describe what
> patch is doing.


True. The other flags in kvm_default_features may be already set and
will be copied to cpuid_kvm_features even if kvm_enabled() is false.

I had a previous version of this patch that also changed the code using
kvm_default_features to check kvm_enabled(), but I removed that part and
kept only the kvm_pv_eoi_features variable removal.

Andreas, please ignore this patch (it is not necessary anymore as this
series doesn't include machine-type compatibility code for kvm_mmu).
Patches 2-7 don't depend on this patch and should apply cleanly without
it.

I will send a new version later, probably with a separate patch to
ignore kvm_default_features if kvm_enabled() is false (so there's no
need to even check kvm_enabled() inside enable_kvm_pv_eoi()).

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Changes v2:
> >  - Coding style fix
> > 
> > Changes v3:
> >  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> > ---
> >  target-i386/cpu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 951e206..40400ac 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -164,15 +164,15 @@ static uint32_t kvm_default_features = (1 <<
> > KVM_FEATURE_CLOCKSOURCE) | (1 << KVM_FEATURE_ASYNC_PF) |
> >          (1 << KVM_FEATURE_STEAL_TIME) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> >  #else
> >  static uint32_t kvm_default_features = 0;
> > -static const uint32_t kvm_pv_eoi_features = 0;
> >  #endif
> >  
> >  void enable_kvm_pv_eoi(void)
> >  {
> > -    kvm_default_features |= kvm_pv_eoi_features;
> > +    if (kvm_enabled()) {
> > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > +    }
> >  }
> >  
> >  void host_cpuid(uint32_t function, uint32_t count,
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 2/7] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

On Wed, Jan 09, 2013 at 09:41:37AM -0200, Eduardo Habkost wrote:
[...]
> Andreas, please ignore this patch (it is not necessary anymore as this
> series doesn't include machine-type compatibility code for kvm_mmu).
> Patches 2-7 don't depend on this patch and should apply cleanly without
> it.

I mean: patches 1 and 3-7 don't depend on this patch and can be applied
cleanly without it.

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu 3/7] target-i386: Disable kvm_mmu by default

[Eduardo Habkost]

KVM_CAP_PV_MMU capability reporting was removed from the kernel since
v2.6.33 (see commit a68a6a7282373), and was completely removed from the
kernel since v3.3 (see commit fb92045843). It doesn't make sense to keep
it enabled by default, as it would cause unnecessary hassle when using
the "enforce" flag.

This disables kvm_mmu on all machine-types. With this fix, the possible
scenarios when migrating from QEMU <= 1.3 to QEMU 1.4 are;

------------+------------+----------------------------------------------------
 src kernel | dst kernel | Result
------------+------------+----------------------------------------------------
 >= 2.6.33  | any        | kvm_mmu was already disabled and will stay disabled
 <= 2.6.32  | >= 3.3     | correct live migration is impossible
 <= 2.6.32  | <= 3.2     | kvm_mmu will be disabled on next guest reboot *
------------+------------+----------------------------------------------------

 * If they are running kernel <= 2.6.32 and want kvm_mmu to be kept
   enabled on guest reboot, they can explicitly add +kvm_mmu to the QEMU
   command-line. Using 2.6.33 and higher, it is not possible to enable
   kvm_mmu explicitly anymore.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: libvir-list@redhat.com
Cc: Jiri Denemark <jdenemar@redhat.com>

Changes v2:
 - Coding style fix
 - Removed redundant comments above machine init functions

Changes v3:
 - Eliminate per-machine-type compatibility code
---
 target-i386/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 40400ac..b09b625 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -159,7 +159,6 @@ int enforce_cpuid = 0;
 #if defined(CONFIG_KVM)
 static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_NOP_IO_DELAY) |
-        (1 << KVM_FEATURE_MMU_OP) |
         (1 << KVM_FEATURE_CLOCKSOURCE2) |
         (1 << KVM_FEATURE_ASYNC_PF) |
         (1 << KVM_FEATURE_STEAL_TIME) |
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH qom-cpu 3/7] target-i386: Disable kvm_mmu by default

[Andreas Färber]

Am 07.01.2013 19:20, schrieb Eduardo Habkost:
> KVM_CAP_PV_MMU capability reporting was removed from the kernel since
> v2.6.33 (see commit a68a6a7282373), and was completely removed from the
> kernel since v3.3 (see commit fb92045843). It doesn't make sense to keep
> it enabled by default, as it would cause unnecessary hassle when using
> the "enforce" flag.
> 
> This disables kvm_mmu on all machine-types. With this fix, the possible
> scenarios when migrating from QEMU <= 1.3 to QEMU 1.4 are;
> 
> ------------+------------+----------------------------------------------------
>  src kernel | dst kernel | Result
> ------------+------------+----------------------------------------------------
>  >= 2.6.33  | any        | kvm_mmu was already disabled and will stay disabled
>  <= 2.6.32  | >= 3.3     | correct live migration is impossible
>  <= 2.6.32  | <= 3.2     | kvm_mmu will be disabled on next guest reboot *
> ------------+------------+----------------------------------------------------

When using ASCII art, please remember to use at most 76 characters, to
avoid linewraps in git-log. Shortening the second column fixes this.

Andreas

> 
>  * If they are running kernel <= 2.6.32 and want kvm_mmu to be kept
>    enabled on guest reboot, they can explicitly add +kvm_mmu to the QEMU
>    command-line. Using 2.6.33 and higher, it is not possible to enable
>    kvm_mmu explicitly anymore.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH qom-cpu 4/7] target-i386/cpu: Introduce FeatureWord typedefs

[Eduardo Habkost]

This introduces a FeatureWord enum, FeatureWordInfo struct (with
generation information about a feature word), and a FeatureWordArray
typedef, and changes add_flagname_to_bitmaps() code and
cpu_x86_parse_featurestr() to use the new typedefs instead of separate
variables for each feature word.

This will help us keep the code at kvm_check_features_against_host(),
cpu_x86_parse_featurestr() and add_flagname_to_bitmaps() sane while
adding new feature name arrays.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 97 +++++++++++++++++++++++++++----------------------------
 target-i386/cpu.h | 15 +++++++++
 2 files changed, 63 insertions(+), 49 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b09b625..7d62d48 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,20 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+typedef struct FeatureWordInfo {
+    const char **feat_names;
+} FeatureWordInfo;
+
+static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
+    [FEAT_1_EDX] = { .feat_names = feature_name },
+    [FEAT_1_ECX] = { .feat_names = ext_feature_name },
+    [FEAT_8000_0001_EDX] = { .feat_names = ext2_feature_name },
+    [FEAT_8000_0001_ECX] = { .feat_names = ext3_feature_name },
+    [FEAT_KVM]   = { .feat_names = kvm_feature_name },
+    [FEAT_SVM]   = { .feat_names = svm_feature_name },
+    [FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name },
+};
+
 const char *get_register_name_32(unsigned int reg)
 {
     static const char *reg_names[CPU_NB_REGS32] = {
@@ -271,23 +285,20 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
     return found;
 }
 
-static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
-                                    uint32_t *ext_features,
-                                    uint32_t *ext2_features,
-                                    uint32_t *ext3_features,
-                                    uint32_t *kvm_features,
-                                    uint32_t *svm_features,
-                                    uint32_t *cpuid_7_0_ebx_features)
+static void add_flagname_to_bitmaps(const char *flagname,
+                                    FeatureWordArray words)
 {
-    if (!lookup_feature(features, flagname, NULL, feature_name) &&
-        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
-        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
-        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
-        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
-        !lookup_feature(svm_features, flagname, NULL, svm_feature_name) &&
-        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
-                        cpuid_7_0_ebx_feature_name))
-            fprintf(stderr, "CPU feature %s not found\n", flagname);
+    FeatureWord w;
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        FeatureWordInfo *wi = &feature_word_info[w];
+        if (wi->feat_names &&
+            lookup_feature(&words[w], flagname, NULL, wi->feat_names)) {
+            break;
+        }
+    }
+    if (w == FEATURE_WORDS) {
+        fprintf(stderr, "CPU feature %s not found\n", flagname);
+    }
 }
 
 typedef struct x86_def_t {
@@ -1256,35 +1267,23 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
-    uint32_t plus_features = 0, plus_ext_features = 0;
-    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0;
-    uint32_t plus_7_0_ebx_features = 0;
+    FeatureWordArray plus_features = {
+        [FEAT_KVM] = kvm_default_features,
+    };
     /* Features to be removed */
-    uint32_t minus_features = 0, minus_ext_features = 0;
-    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
-    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
-    uint32_t minus_7_0_ebx_features = 0;
+    FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
 
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-            &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-            &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
+    add_flagname_to_bitmaps("hypervisor", plus_features);
 
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
         char *val;
         if (featurestr[0] == '+') {
-            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
-                            &plus_ext_features, &plus_ext2_features,
-                            &plus_ext3_features, &plus_kvm_features,
-                            &plus_svm_features, &plus_7_0_ebx_features);
+            add_flagname_to_bitmaps(featurestr + 1, plus_features);
         } else if (featurestr[0] == '-') {
-            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
-                            &minus_ext_features, &minus_ext2_features,
-                            &minus_ext3_features, &minus_kvm_features,
-                            &minus_svm_features, &minus_7_0_ebx_features);
+            add_flagname_to_bitmaps(featurestr + 1, minus_features);
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             if (!strcmp(featurestr, "family")) {
@@ -1384,20 +1383,20 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
         }
         featurestr = strtok(NULL, ",");
     }
-    x86_cpu_def->features |= plus_features;
-    x86_cpu_def->ext_features |= plus_ext_features;
-    x86_cpu_def->ext2_features |= plus_ext2_features;
-    x86_cpu_def->ext3_features |= plus_ext3_features;
-    x86_cpu_def->kvm_features |= plus_kvm_features;
-    x86_cpu_def->svm_features |= plus_svm_features;
-    x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_features;
-    x86_cpu_def->features &= ~minus_features;
-    x86_cpu_def->ext_features &= ~minus_ext_features;
-    x86_cpu_def->ext2_features &= ~minus_ext2_features;
-    x86_cpu_def->ext3_features &= ~minus_ext3_features;
-    x86_cpu_def->kvm_features &= ~minus_kvm_features;
-    x86_cpu_def->svm_features &= ~minus_svm_features;
-    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
+    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
+    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
+    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
+    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
+    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
+    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
+    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
+    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
+    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
+    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
+    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
+    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
+    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
+    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
     if (check_cpuid && kvm_enabled()) {
         if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
             goto error;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e56921b..e4a7c50 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -361,6 +361,21 @@
 
 #define MSR_VM_HSAVE_PA                 0xc0010117
 
+/* CPUID feature words */
+typedef enum FeatureWord {
+    FEAT_1_EDX,         /* CPUID[1].EDX */
+    FEAT_1_ECX,         /* CPUID[1].ECX */
+    FEAT_7_0_EBX,       /* CPUID[EAX=7,ECX=0].EBX */
+    FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
+    FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
+    FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */
+    FEAT_KVM,           /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */
+    FEAT_SVM,           /* CPUID[8000_000A].EDX */
+    FEATURE_WORDS,
+} FeatureWord;
+
+typedef uint32_t FeatureWordArray[FEATURE_WORDS];
+
 /* cpuid_features bits */
 #define CPUID_FP87 (1 << 0)
 #define CPUID_VME  (1 << 1)
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH qom-cpu 4/7] target-i386/cpu: Introduce FeatureWord typedefs

[Gleb Natapov]

On Mon, Jan 07, 2013 at 04:20:45PM -0200, Eduardo Habkost wrote:
> This introduces a FeatureWord enum, FeatureWordInfo struct (with
> generation information about a feature word), and a FeatureWordArray
> typedef, and changes add_flagname_to_bitmaps() code and
> cpu_x86_parse_featurestr() to use the new typedefs instead of separate
> variables for each feature word.
> 
> This will help us keep the code at kvm_check_features_against_host(),
> cpu_x86_parse_featurestr() and add_flagname_to_bitmaps() sane while
> adding new feature name arrays.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 97 +++++++++++++++++++++++++++----------------------------
>  target-i386/cpu.h | 15 +++++++++
>  2 files changed, 63 insertions(+), 49 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b09b625..7d62d48 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -124,6 +124,20 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
>      NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>  };
>  
> +typedef struct FeatureWordInfo {
> +    const char **feat_names;
> +} FeatureWordInfo;
> +
> +static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> +    [FEAT_1_EDX] = { .feat_names = feature_name },
> +    [FEAT_1_ECX] = { .feat_names = ext_feature_name },
> +    [FEAT_8000_0001_EDX] = { .feat_names = ext2_feature_name },
> +    [FEAT_8000_0001_ECX] = { .feat_names = ext3_feature_name },
> +    [FEAT_KVM]   = { .feat_names = kvm_feature_name },
> +    [FEAT_SVM]   = { .feat_names = svm_feature_name },
> +    [FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name },
> +};
> +
>  const char *get_register_name_32(unsigned int reg)
>  {
>      static const char *reg_names[CPU_NB_REGS32] = {
> @@ -271,23 +285,20 @@ static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
>      return found;
>  }
>  
> -static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
> -                                    uint32_t *ext_features,
> -                                    uint32_t *ext2_features,
> -                                    uint32_t *ext3_features,
> -                                    uint32_t *kvm_features,
> -                                    uint32_t *svm_features,
> -                                    uint32_t *cpuid_7_0_ebx_features)
> +static void add_flagname_to_bitmaps(const char *flagname,
> +                                    FeatureWordArray words)
>  {
> -    if (!lookup_feature(features, flagname, NULL, feature_name) &&
> -        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
> -        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
> -        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
> -        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
> -        !lookup_feature(svm_features, flagname, NULL, svm_feature_name) &&
> -        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
> -                        cpuid_7_0_ebx_feature_name))
> -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> +    FeatureWord w;
> +    for (w = 0; w < FEATURE_WORDS; w++) {
> +        FeatureWordInfo *wi = &feature_word_info[w];
> +        if (wi->feat_names &&
I would move patch 6 before that one and drop this check here, but if
you disagree it is your call.

> +            lookup_feature(&words[w], flagname, NULL, wi->feat_names)) {
> +            break;
> +        }
> +    }
> +    if (w == FEATURE_WORDS) {
> +        fprintf(stderr, "CPU feature %s not found\n", flagname);
> +    }
>  }
>  
>  typedef struct x86_def_t {
> @@ -1256,35 +1267,23 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      unsigned int i;
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
> -    uint32_t plus_features = 0, plus_ext_features = 0;
> -    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> -    uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0;
> -    uint32_t plus_7_0_ebx_features = 0;
> +    FeatureWordArray plus_features = {
> +        [FEAT_KVM] = kvm_default_features,
> +    };
>      /* Features to be removed */
> -    uint32_t minus_features = 0, minus_ext_features = 0;
> -    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
> -    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> -    uint32_t minus_7_0_ebx_features = 0;
> +    FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
>  
> -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> -            &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -            &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
> +    add_flagname_to_bitmaps("hypervisor", plus_features);
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
>          char *val;
>          if (featurestr[0] == '+') {
> -            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> -                            &plus_ext_features, &plus_ext2_features,
> -                            &plus_ext3_features, &plus_kvm_features,
> -                            &plus_svm_features, &plus_7_0_ebx_features);
> +            add_flagname_to_bitmaps(featurestr + 1, plus_features);
>          } else if (featurestr[0] == '-') {
> -            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> -                            &minus_ext_features, &minus_ext2_features,
> -                            &minus_ext3_features, &minus_kvm_features,
> -                            &minus_svm_features, &minus_7_0_ebx_features);
> +            add_flagname_to_bitmaps(featurestr + 1, minus_features);
>          } else if ((val = strchr(featurestr, '='))) {
>              *val = 0; val++;
>              if (!strcmp(featurestr, "family")) {
> @@ -1384,20 +1383,20 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>          }
>          featurestr = strtok(NULL, ",");
>      }
> -    x86_cpu_def->features |= plus_features;
> -    x86_cpu_def->ext_features |= plus_ext_features;
> -    x86_cpu_def->ext2_features |= plus_ext2_features;
> -    x86_cpu_def->ext3_features |= plus_ext3_features;
> -    x86_cpu_def->kvm_features |= plus_kvm_features;
> -    x86_cpu_def->svm_features |= plus_svm_features;
> -    x86_cpu_def->cpuid_7_0_ebx_features |= plus_7_0_ebx_features;
> -    x86_cpu_def->features &= ~minus_features;
> -    x86_cpu_def->ext_features &= ~minus_ext_features;
> -    x86_cpu_def->ext2_features &= ~minus_ext2_features;
> -    x86_cpu_def->ext3_features &= ~minus_ext3_features;
> -    x86_cpu_def->kvm_features &= ~minus_kvm_features;
> -    x86_cpu_def->svm_features &= ~minus_svm_features;
> -    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> +    x86_cpu_def->features |= plus_features[FEAT_1_EDX];
> +    x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
> +    x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
> +    x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
> +    x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
> +    x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
> +    x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
> +    x86_cpu_def->features &= ~minus_features[FEAT_1_EDX];
> +    x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
> +    x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
> +    x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
> +    x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
> +    x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
> +    x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
>      if (check_cpuid && kvm_enabled()) {
>          if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
>              goto error;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index e56921b..e4a7c50 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -361,6 +361,21 @@
>  
>  #define MSR_VM_HSAVE_PA                 0xc0010117
>  
> +/* CPUID feature words */
> +typedef enum FeatureWord {
> +    FEAT_1_EDX,         /* CPUID[1].EDX */
> +    FEAT_1_ECX,         /* CPUID[1].ECX */
> +    FEAT_7_0_EBX,       /* CPUID[EAX=7,ECX=0].EBX */
> +    FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
> +    FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
> +    FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */
> +    FEAT_KVM,           /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */
> +    FEAT_SVM,           /* CPUID[8000_000A].EDX */
> +    FEATURE_WORDS,
> +} FeatureWord;
> +
> +typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> +
>  /* cpuid_features bits */
>  #define CPUID_FP87 (1 << 0)
>  #define CPUID_VME  (1 << 1)
> -- 
> 1.7.11.7

--
			Gleb.

[Qemu-devel] [PATCH qom-cpu 5/7] target-i386: kvm_check_features_against_host(): Use feature_word_info

[Eduardo Habkost]

Instead of carrying the CPUID leaf/register and feature name array on
the model_features_t struct, move that information into
feature_word_info so it can be reused by other functions.

The goal is to eventually kill model_features_t entirely, but to do that
we have to either convert x86_def_t.features to an array or use
offsetof() inside FeatureWordInfo (to replace the pointers inside
model_features_t). So by now just move most of the model_features_t
fields to FeatureWordInfo except for the two pointers to local
arguments.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 24 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7d62d48..4b3ee63 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -126,16 +126,39 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
 
 typedef struct FeatureWordInfo {
     const char **feat_names;
+    uint32_t cpuid_eax; /* Input EAX for CPUID */
+    int cpuid_reg;      /* R_* register constant */
 } FeatureWordInfo;
 
 static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
-    [FEAT_1_EDX] = { .feat_names = feature_name },
-    [FEAT_1_ECX] = { .feat_names = ext_feature_name },
-    [FEAT_8000_0001_EDX] = { .feat_names = ext2_feature_name },
-    [FEAT_8000_0001_ECX] = { .feat_names = ext3_feature_name },
-    [FEAT_KVM]   = { .feat_names = kvm_feature_name },
-    [FEAT_SVM]   = { .feat_names = svm_feature_name },
-    [FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name },
+    [FEAT_1_EDX] = {
+        .feat_names = feature_name,
+        .cpuid_eax = 1, .cpuid_reg = R_EDX,
+    },
+    [FEAT_1_ECX] = {
+        .feat_names = ext_feature_name,
+        .cpuid_eax = 1, .cpuid_reg = R_ECX,
+    },
+    [FEAT_8000_0001_EDX] = {
+        .feat_names = ext2_feature_name,
+        .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX,
+    },
+    [FEAT_8000_0001_ECX] = {
+        .feat_names = ext3_feature_name,
+        .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX,
+    },
+    [FEAT_KVM] = {
+        .feat_names = kvm_feature_name,
+        .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX,
+    },
+    [FEAT_SVM] = {
+        .feat_names = svm_feature_name,
+        .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX,
+    },
+    [FEAT_7_0_EBX] = {
+        .feat_names = cpuid_7_0_ebx_feature_name,
+        .cpuid_eax = 7, .cpuid_reg = R_EBX,
+    },
 };
 
 const char *get_register_name_32(unsigned int reg)
@@ -162,9 +185,7 @@ const char *get_register_name_32(unsigned int reg)
 typedef struct model_features_t {
     uint32_t *guest_feat;
     uint32_t *host_feat;
-    const char **flag_names;
-    uint32_t cpuid;
-    int reg;
+    FeatureWord feat_word;
 } model_features_t;
 
 int check_cpuid = 0;
@@ -935,19 +956,19 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 #endif /* CONFIG_KVM */
 }
 
-static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
+static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
 {
     int i;
 
     for (i = 0; i < 32; ++i)
         if (1 << i & mask) {
-            const char *reg = get_register_name_32(f->reg);
+            const char *reg = get_register_name_32(f->cpuid_reg);
             assert(reg);
             fprintf(stderr, "warning: host doesn't support requested feature: "
                 "CPUID.%02XH:%s%s%s [bit %d]\n",
-                f->cpuid, reg,
-                f->flag_names[i] ? "." : "",
-                f->flag_names[i] ? f->flag_names[i] : "", i);
+                f->cpuid_eax, reg,
+                f->feat_names[i] ? "." : "",
+                f->feat_names[i] ? f->feat_names[i] : "", i);
             break;
         }
     return 0;
@@ -965,25 +986,29 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
     int rv, i;
     struct model_features_t ft[] = {
         {&guest_def->features, &host_def.features,
-            feature_name, 0x00000001, R_EDX},
+            FEAT_1_EDX },
         {&guest_def->ext_features, &host_def.ext_features,
-            ext_feature_name, 0x00000001, R_ECX},
+            FEAT_1_ECX },
         {&guest_def->ext2_features, &host_def.ext2_features,
-            ext2_feature_name, 0x80000001, R_EDX},
+            FEAT_8000_0001_EDX },
         {&guest_def->ext3_features, &host_def.ext3_features,
-            ext3_feature_name, 0x80000001, R_ECX}
+            FEAT_8000_0001_ECX },
     };
 
     assert(kvm_enabled());
 
     kvm_cpu_fill_host(&host_def);
-    for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i)
-        for (mask = 1; mask; mask <<= 1)
+    for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
+        FeatureWord w = ft[i].feat_word;
+        FeatureWordInfo *wi = &feature_word_info[w];
+        for (mask = 1; mask; mask <<= 1) {
             if (*ft[i].guest_feat & mask &&
                 !(*ft[i].host_feat & mask)) {
-                    unavailable_host_feature(&ft[i], mask);
-                    rv = 1;
-                }
+                unavailable_host_feature(wi, mask);
+                rv = 1;
+            }
+        }
+    }
     return rv;
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH qom-cpu 6/7] target-i386/cpu.c: Add feature name array for ext4_features

[Eduardo Habkost]

Feature names were taken from the X86_FEATURE_* constants in the Linux
kernel code.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Gleb Natapov <gleb@redhat.com>
---
 target-i386/cpu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4b3ee63..a54c464 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -95,6 +95,17 @@ static const char *ext3_feature_name[] = {
     NULL, NULL, NULL, NULL,
 };
 
+static const char *ext4_feature_name[] = {
+    NULL, NULL, "xstore","xstore-en",
+    NULL, NULL, "xcrypt","xcrypt-en",
+    "ace2", "ace2-en", "phe", "phe-en",
+    "pmm", "pmm-en", NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+};
+
 static const char *kvm_feature_name[] = {
     "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
     "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL,
@@ -147,6 +158,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .feat_names = ext3_feature_name,
         .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX,
     },
+    [FEAT_C000_0001_EDX] = {
+        .feat_names = ext4_feature_name,
+        .cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX,
+    },
     [FEAT_KVM] = {
         .feat_names = kvm_feature_name,
         .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX,
@@ -1412,6 +1427,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX];
     x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX];
     x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX];
+    x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX];
     x86_cpu_def->kvm_features |= plus_features[FEAT_KVM];
     x86_cpu_def->svm_features |= plus_features[FEAT_SVM];
     x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX];
@@ -1419,6 +1435,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX];
     x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX];
     x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX];
+    x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX];
     x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM];
     x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM];
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX];
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH qom-cpu 6/7] target-i386/cpu.c: Add feature name array for ext4_features

[Andreas Färber]

Am 07.01.2013 19:20, schrieb Eduardo Habkost:
> Feature names were taken from the X86_FEATURE_* constants in the Linux
> kernel code.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Gleb Natapov <gleb@redhat.com>
> ---
>  target-i386/cpu.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4b3ee63..a54c464 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -95,6 +95,17 @@ static const char *ext3_feature_name[] = {
>      NULL, NULL, NULL, NULL,
>  };
>  
> +static const char *ext4_feature_name[] = {
> +    NULL, NULL, "xstore","xstore-en",
> +    NULL, NULL, "xcrypt","xcrypt-en",

Missing spaces, fixed.

Andreas

> +    "ace2", "ace2-en", "phe", "phe-en",
> +    "pmm", "pmm-en", NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};
> +
>  static const char *kvm_feature_name[] = {
>      "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
>      "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL,
[snip]

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH qom-cpu 7/7] target-i386: check/enforce: Check all feature words

[Eduardo Habkost]

This adds the following feature words to the list of flags to be checked
by kvm_check_features_against_host():

 - cpuid_7_0_ebx_features
 - ext4_features
 - kvm_features
 - svm_features

This will ensure the "enforce" flag works as it should: it won't allow
QEMU to be started unless every flag that was requested by the user or
defined in the CPU model is supported by the host.

This patch may cause existing configurations where "enforce" wasn't
preventing QEMU from being started to abort QEMU. But that's exactly the
point of this patch: if a flag was not supported by the host and QEMU
wasn't aborting, it was a bug in the "enforce" code.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org
Cc: libvir-list@redhat.com
Cc: Jiri Denemark <jdenemar@redhat.com>

CCing libvirt people, as this is directly related to the planned usage
of the "enforce" flag by libvirt.

The libvirt team probably has a problem in their hands: libvirt should
use "enforce" to make sure all requested flags are making their way into
the guest (so the resulting CPU is always the same, on any host), but
users may have existing working configurations where a flag is not
supported by the guest and the user really doesn't care about it. Those
configurations will necessarily break when libvirt starts using
"enforce".

One example where it may cause trouble for common setups: pc-1.3 wants
the kvm_pv_eoi flag enabled by default (so "enforce" will make sure it
is enabled), but the user may have an existing VM running on a host
without pv_eoi support. That setup is unsafe today because
live-migration between different host kernel versions may enable/disable
pv_eoi silently (that's why we need the "enforce" flag to be used by
libvirt), but the user probably would like to be able to live-migrate
that VM anyway (and have libvirt to "just do the right thing").

One possible solution to libvirt is to use "enforce" only on newer
machine-types, so existing machines with older machine-types will keep
the unsafe host-dependent-ABI behavior, but at least would keep
live-migration working in case the user is careful.

I really don't know what the libvirt team prefers, but that's the
situation today. The longer we take to make "enforce" strict as it
should and make libvirt finally use it, more users will have VMs with
migration-unsafe unpredictable guest ABIs.

Changes v2:
 - Coding style fix

Changes v3:
 - Added ext4_feature_name array
---
 target-i386/cpu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a54c464..68cabcf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -989,8 +989,9 @@ static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
     return 0;
 }
 
-/* best effort attempt to inform user requested cpu flags aren't making
- * their way to the guest.
+/* Check if all requested cpu flags are making their way to the guest
+ *
+ * Returns 0 if all flags are supported by the host, non-zero otherwise.
  *
  * This function may be called only if KVM is enabled.
  */
@@ -1008,6 +1009,14 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
             FEAT_8000_0001_EDX },
         {&guest_def->ext3_features, &host_def.ext3_features,
             FEAT_8000_0001_ECX },
+        {&guest_def->ext4_features, &host_def.ext4_features,
+            FEAT_C000_0001_EDX },
+        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
+            FEAT_7_0_EBX },
+        {&guest_def->svm_features, &host_def.svm_features,
+            FEAT_SVM },
+        {&guest_def->kvm_features, &host_def.kvm_features,
+            FEAT_KVM },
     };
 
     assert(kvm_enabled());
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH qom-cpu 0/7] disable kvm_mmu + -cpu "enforce" fixes (v3)

[Gleb Natapov]

On Mon, Jan 07, 2013 at 04:20:41PM -0200, Eduardo Habkost wrote:
> Changes on v3:
>  - Patches 3-9 from v2 are now already on qom-cpu tree
>  - Remove CONFIG_KVM #ifdefs by declaring fake KVM_* #defines on sysemu/kvm.h
>  - Refactor code that uses the feature word arrays
>    (to make it easier to add a new feature name array)
>  - Add feature name array for CPUID leaf 0xC0000001
> 
> Changes on v2:
>  - Now both the kvm_mmu-disable and -cpu "enforce" changes are on the same
>    series
>  - Coding style fixes
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git cpu-enforce-all.v3
>   https://github.com/ehabkost/qemu-hacks/tree/cpu-enforce-all.v3
> 
> The changes are a bit intrusive, but:
> 
>  - The longer we take to make "enforce" strict as it should (and make libvirt
>    finally use it), more users will have VMs with migration-unsafe unpredictable
>    guest ABIs. For this reason, I would like to get this into QEMU 1.4.
>  - The changes in this series should affect only users that are already using
>    the "enforce" flag, and I believe whoever is using the "enforce" flag really
>    want the strict behavior introduced by this series.
> 
> 
> 
Reviewed-by: Gleb Natapov <gleb@redhat.com>

Small comment on patch 4. Fill free to ignore.

> Eduardo Habkost (7):
>   kvm: Add fake KVM constants to avoid #ifdefs on KVM-specific code
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   target-i386: Disable kvm_mmu by default
>   target-i386/cpu: Introduce FeatureWord typedefs
>   target-i386: kvm_check_features_against_host(): Use feature_word_info
>   target-i386/cpu.c: Add feature name array for ext4_features
>   target-i386: check/enforce: Check all feature words
> 
>  include/sysemu/kvm.h |  14 ++++
>  target-i386/cpu.c    | 193 ++++++++++++++++++++++++++++++++-------------------
>  target-i386/cpu.h    |  15 ++++
>  3 files changed, 150 insertions(+), 72 deletions(-)
> 
> -- 
> 1.7.11.7

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 0/7] disable kvm_mmu + -cpu "enforce" fixes (v3)

[Andreas Färber]

Am 07.01.2013 19:20, schrieb Eduardo Habkost:
> Eduardo Habkost (7):
>   kvm: Add fake KVM constants to avoid #ifdefs on KVM-specific code
[...]
>   target-i386: Disable kvm_mmu by default
>   target-i386/cpu: Introduce FeatureWord typedefs
>   target-i386: kvm_check_features_against_host(): Use feature_word_info
>   target-i386/cpu.c: Add feature name array for ext4_features
>   target-i386: check/enforce: Check all feature words

Thanks, applied these to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg