[Qemu-devel] [PATCH 0/9] target-i386: make "enforce" flag work as it should

[Qemu-devel] [PATCH 0/9] target-i386: make "enforce" flag work as it should

[Eduardo Habkost]

This changes the -cpu check/enforce code to work as it should: it will
check every single CPUID bit to make sure it is supported by the host.

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.

This series is based on Andreas' qom-cpu branch, and depends on the series:
  Subject: [PATCH 0/2] Disable kvm_mmu_op by default on pc-1.4
  Message-Id: <1357311145-16410-1-git-send-email-ehabkost@redhat.com>

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


Eduardo Habkost (9):
  target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features
  target-i386: kvm: Enable all supported KVM features for -cpu host
  target-i386: check/enforce: Fix CPUID leaf numbers on error messages
  target-i386: check/enforce: Do not ignore "hypervisor" flag
  target-i386: check/enforce: Check all CPUID.80000001H.EDX bits
  target-i386: check/enforce: Check SVM flag support as well
  target-i386: check/enforce: Eliminate check_feat field
  target-i386: Call kvm_check_features_against_host() only if
    CONFIG_KVM is set
  target-i386: check/enforce: Check all feature words

 target-i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++++----------------
 target-i386/cpu.h |  3 +++
 2 files changed, 55 insertions(+), 21 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/9] target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features

[Eduardo Habkost]

The existing -cpu host code simply set every bit inside svm_features
(initializing it to -1), and that makes it impossible to make the
enforce/check options work properly when the user asks for SVM features
explicitly in the command-line.

So, instead of initializing svm_features to -1, use GET_SUPPORTED_CPUID
to fill only the bits that are supported by the host (just like we do
for all other CPUID feature words inside kvm_cpu_fill_host()).

This will keep the existing behavior (as filter_features_for_kvm()
already uses GET_SUPPORTED_CPUID to filter svm_features), but will allow
us to properly check for KVM features inside
kvm_check_features_against_host() later.

For example, we will be able to make this:

  $ qemu-system-x86_64 -cpu ...,+pfthreshold,enforce

refuse to start if the SVM "pfthreshold" feature is not supported by the
host (after we fix kvm_check_features_against_host() to check SVM flags
as well).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Coding style (indentation) fix

Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: kvm@vger.kernel.org
---
 target-i386/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ec877c7..649cfb2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -906,13 +906,10 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
         }
     }
 
-    /*
-     * Every SVM feature requires emulation support in KVM - so we can't just
-     * read the host features here. KVM might even support SVM features not
-     * available on the host hardware. Just set all bits and mask out the
-     * unsupported ones later.
-     */
-    x86_cpu_def->svm_features = -1;
+    /* Other KVM-specific feature fields: */
+    x86_cpu_def->svm_features =
+        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
+
 #endif /* CONFIG_KVM */
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/9] target-i386: kvm: Enable all supported KVM features for -cpu host

[Eduardo Habkost]

When using -cpu host, we don't need to use the kvm_default_features
variable, as the user is explicitly asking QEMU to enable all feature
supported by the host.

This changes the kvm_cpu_fill_host() code to use GET_SUPPORTED_CPUID to
initialize the kvm_features field, so we get all host KVM features
enabled.

This will also allow use to properly check/enforce KVM features inside
kvm_check_features_against_host() later. For example, we will be able to
make this:

  $ qemu-system-x86_64 -cpu ...,+kvm_pv_eoi,enforce

refuse to start if kvm_pv_eoi is not supported by the host (after we fix
kvm_check_features_against_host() to check KVM flags as well).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Coding style (indentation) fix

Cc: Gleb Natapov <gleb@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org
---
 target-i386/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 649cfb2..4e26b11 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -909,6 +909,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     /* Other KVM-specific feature fields: */
     x86_cpu_def->svm_features =
         kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
+    x86_cpu_def->kvm_features =
+        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
 
 #endif /* CONFIG_KVM */
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/9] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

[Eduardo Habkost]

The -cpu check/enforce warnings are printing incorrect information about the
missing flags. There are no feature flags on CPUID leaves 0 and 0x80000000, but
there were references to 0 and 0x80000000 in the table at
kvm_check_features_against_host().

This changes the model_features_t struct to contain the register number as
well, so the error messages print the correct CPUID leaf+register information,
instead of wrong CPUID leaf numbers.

This also changes the format of the error messages, so they follow the
"CPUID.<leaf>.<register>.<name> [bit <offset>]" convention used on Intel
documentation. Example output:

    $ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu Opteron_G4,+ia64,enforce
    warning: host doesn't support requested feature: CPUID.01H:EDX.ia64 [bit 30]
    warning: host doesn't support requested feature: CPUID.01H:ECX.xsave [bit 26]
    warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit 28]
    warning: host doesn't support requested feature: CPUID.80000001H:ECX.abm [bit 5]
    warning: host doesn't support requested feature: CPUID.80000001H:ECX.sse4a [bit 6]
    warning: host doesn't support requested feature: CPUID.80000001H:ECX.misalignsse [bit 7]
    warning: host doesn't support requested feature: CPUID.80000001H:ECX.3dnowprefetch [bit 8]
    warning: host doesn't support requested feature: CPUID.80000001H:ECX.xop [bit 11]
    warning: host doesn't support requested feature: CPUID.80000001H:ECX.fma4 [bit 16]
    Unable to find x86 CPU definition
    $

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
---
 target-i386/cpu.c | 38 +++++++++++++++++++++++++++++---------
 target-i386/cpu.h |  3 +++
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4e26b11..6c43ace 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,24 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+const char *get_register_name_32(unsigned int reg)
+{
+    static const char *reg_names[CPU_NB_REGS32] = {
+        [R_EAX] = "EAX",
+        [R_ECX] = "ECX",
+        [R_EDX] = "EDX",
+        [R_EBX] = "EBX",
+        [R_ESP] = "ESP",
+        [R_EBP] = "EBP",
+        [R_ESI] = "ESI",
+        [R_EDI] = "EDI",
+    };
+
+    if (reg > CPU_NB_REGS32)
+        return NULL;
+    return reg_names[reg];
+}
+
 /* collects per-function cpuid data
  */
 typedef struct model_features_t {
@@ -132,7 +150,8 @@ typedef struct model_features_t {
     uint32_t check_feat;
     const char **flag_names;
     uint32_t cpuid;
-    } model_features_t;
+    int reg;
+} model_features_t;
 
 int check_cpuid = 0;
 int enforce_cpuid = 0;
@@ -921,10 +940,11 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
 
     for (i = 0; i < 32; ++i)
         if (1 << i & mask) {
-            fprintf(stderr, "warning: host cpuid %04x_%04x lacks requested"
-                " flag '%s' [0x%08x]\n",
-                f->cpuid >> 16, f->cpuid & 0xffff,
-                f->flag_names[i] ? f->flag_names[i] : "[reserved]", mask);
+            fprintf(stderr, "warning: host doesn't support requested feature: "
+                "CPUID.%02XH:%s%s%s [bit %d]\n",
+                f->cpuid, get_register_name_32(f->reg),
+                f->flag_names[i] ? "." : "",
+                f->flag_names[i] ? f->flag_names[i] : "", i);
             break;
         }
     return 0;
@@ -943,13 +963,13 @@ 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,
-            ~0, feature_name, 0x00000000},
+            ~0, feature_name, 0x00000001, R_EDX},
         {&guest_def->ext_features, &host_def.ext_features,
-            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
+            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX},
         {&guest_def->ext2_features, &host_def.ext2_features,
-            ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
+            ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX},
         {&guest_def->ext3_features, &host_def.ext3_features,
-            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
+            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}};
 
     assert(kvm_enabled());
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 27c8d0c..ab81a5c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1221,4 +1221,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 void enable_kvm_pv_eoi(void);
 void disable_kvm_mmu_op(void);
 
+/* Return name of 32-bit register, from a R_* constant */
+const char *get_register_name_32(unsigned int reg);
+
 #endif /* CPU_I386_H */
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 3/9] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

[Blue Swirl]

On Fri, Jan 4, 2013 at 3:37 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The -cpu check/enforce warnings are printing incorrect information about the
> missing flags. There are no feature flags on CPUID leaves 0 and 0x80000000, but
> there were references to 0 and 0x80000000 in the table at
> kvm_check_features_against_host().
>
> This changes the model_features_t struct to contain the register number as
> well, so the error messages print the correct CPUID leaf+register information,
> instead of wrong CPUID leaf numbers.
>
> This also changes the format of the error messages, so they follow the
> "CPUID.<leaf>.<register>.<name> [bit <offset>]" convention used on Intel
> documentation. Example output:
>
>     $ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu Opteron_G4,+ia64,enforce
>     warning: host doesn't support requested feature: CPUID.01H:EDX.ia64 [bit 30]
>     warning: host doesn't support requested feature: CPUID.01H:ECX.xsave [bit 26]
>     warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit 28]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.abm [bit 5]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.sse4a [bit 6]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.misalignsse [bit 7]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.3dnowprefetch [bit 8]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.xop [bit 11]
>     warning: host doesn't support requested feature: CPUID.80000001H:ECX.fma4 [bit 16]
>     Unable to find x86 CPU definition
>     $
>
> 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
> ---
>  target-i386/cpu.c | 38 +++++++++++++++++++++++++++++---------
>  target-i386/cpu.h |  3 +++
>  2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4e26b11..6c43ace 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -124,6 +124,24 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
>      NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>  };
>
> +const char *get_register_name_32(unsigned int reg)
> +{
> +    static const char *reg_names[CPU_NB_REGS32] = {
> +        [R_EAX] = "EAX",
> +        [R_ECX] = "ECX",
> +        [R_EDX] = "EDX",
> +        [R_EBX] = "EBX",
> +        [R_ESP] = "ESP",
> +        [R_EBP] = "EBP",
> +        [R_ESI] = "ESI",
> +        [R_EDI] = "EDI",
> +    };
> +
> +    if (reg > CPU_NB_REGS32)

Missing braces.

> +        return NULL;
> +    return reg_names[reg];
> +}
> +
>  /* collects per-function cpuid data
>   */
>  typedef struct model_features_t {
> @@ -132,7 +150,8 @@ typedef struct model_features_t {
>      uint32_t check_feat;
>      const char **flag_names;
>      uint32_t cpuid;
> -    } model_features_t;
> +    int reg;
> +} model_features_t;
>
>  int check_cpuid = 0;
>  int enforce_cpuid = 0;
> @@ -921,10 +940,11 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
>
>      for (i = 0; i < 32; ++i)
>          if (1 << i & mask) {
> -            fprintf(stderr, "warning: host cpuid %04x_%04x lacks requested"
> -                " flag '%s' [0x%08x]\n",
> -                f->cpuid >> 16, f->cpuid & 0xffff,
> -                f->flag_names[i] ? f->flag_names[i] : "[reserved]", mask);
> +            fprintf(stderr, "warning: host doesn't support requested feature: "
> +                "CPUID.%02XH:%s%s%s [bit %d]\n",
> +                f->cpuid, get_register_name_32(f->reg),

This could attempt to print NULL via %s format, which is not OK with
all C libraries. If we trust that the callers always pass valid
numbers, the check above could be turned into assert().

> +                f->flag_names[i] ? "." : "",
> +                f->flag_names[i] ? f->flag_names[i] : "", i);
>              break;
>          }
>      return 0;
> @@ -943,13 +963,13 @@ 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,
> -            ~0, feature_name, 0x00000000},
> +            ~0, feature_name, 0x00000001, R_EDX},
>          {&guest_def->ext_features, &host_def.ext_features,
> -            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
> +            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX},
>          {&guest_def->ext2_features, &host_def.ext2_features,
> -            ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
> +            ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX},
>          {&guest_def->ext3_features, &host_def.ext3_features,
> -            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
> +            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}};
>
>      assert(kvm_enabled());
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 27c8d0c..ab81a5c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1221,4 +1221,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  void enable_kvm_pv_eoi(void);
>  void disable_kvm_mmu_op(void);
>
> +/* Return name of 32-bit register, from a R_* constant */
> +const char *get_register_name_32(unsigned int reg);
> +
>  #endif /* CPU_I386_H */
> --
> 1.7.11.7
>
>

[Qemu-devel] [PATCH 4/9] target-i386: check/enforce: Do not ignore "hypervisor" flag

[Eduardo Habkost]

We don't need any hack to ignore CPUID_EXT_HYPERVISOR anymore, because
kvm_arch_get_supported_cpuid() now set CPUID_EXT_HYPERVISOR properly.
So, this shouldn't introduce any behavior change, but it makes the code
simpler.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
My goal is to eliminate the check_feat field completely, as
kvm_arch_get_supported_cpuid() should now really return all the bits we
can set on all CPUID leaves.
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6c43ace..f1a21cf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -965,7 +965,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
         {&guest_def->features, &host_def.features,
             ~0, feature_name, 0x00000001, R_EDX},
         {&guest_def->ext_features, &host_def.ext_features,
-            ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX},
+            ~0, ext_feature_name, 0x00000001, R_ECX},
         {&guest_def->ext2_features, &host_def.ext2_features,
             ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX},
         {&guest_def->ext3_features, &host_def.ext3_features,
-- 
1.7.11.7

[Qemu-devel] [PATCH 5/9] target-i386: check/enforce: Check all CPUID.80000001H.EDX bits

[Eduardo Habkost]

I have no idea why PPRO_FEATURES was being ignored on the check of the
CPUID.80000001H.EDX bits. I believe it was a mistake, and it was
supposed to be ~(PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) or just
~CPUID_EXT2_AMD_ALIASES, because some time ago kvm_cpu_fill_host() used
the CPUID instruction directly (instead of
kvm_arch_get_supported_cpuid()).

But now kvm_cpu_fill_host() use kvm_arch_get_supported_cpuid(), and
kvm_arch_get_supported_cpuid() returns all supported bits for
CPUID.80000001H.EDX, even the AMD aliases (that are explicitly copied
from CPUID.01H.EDX), so we can make the code check/enforce all the
CPUID.80000001H.EDX bits.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f1a21cf..13075c7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -967,7 +967,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
         {&guest_def->ext_features, &host_def.ext_features,
             ~0, ext_feature_name, 0x00000001, R_ECX},
         {&guest_def->ext2_features, &host_def.ext2_features,
-            ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX},
+            ~0, ext2_feature_name, 0x80000001, R_EDX},
         {&guest_def->ext3_features, &host_def.ext3_features,
             ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}};
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 6/9] target-i386: check/enforce: Check SVM flag support as well

[Eduardo Habkost]

When nested SVM is supported, the kernel returns the SVM flag on
GET_SUPPORTED_CPUID[1], so we can check the SVM flag safely on
kvm_check_features_against_host().

I don't know why the original code ignored the SVM flag. Maybe it was
because kvm_cpu_fill_host() used the CPUID instruction directly instead
of GET_SUPPORTED_CPUID

[1] Older kernels (before v2.6.37) returned the SVM flag even if nested
    SVM was _not_ supported. So the only cases where this patch should
    change behavior is when SVM is being requested by the user or the
    CPU model, but not supported by the host. And on these cases we
    really want QEMU to abort if the "enforce" option is set.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Joerg Roedel <joro@8bytes.org>
Cc: kvm@vger.kernel.org
Cc: libvir-list@redhat.com
Cc: Jiri Denemark <jdenemar@redhat.com>

I'm CCing libvirt people in case having SVM enabled by default may cause
trouble when libvirt starts using the "enforce" flag. I don't know if
libvirt expects most of the QEMU CPU models to have nested SVM enabled.
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 13075c7..21aceed 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -969,7 +969,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
         {&guest_def->ext2_features, &host_def.ext2_features,
             ~0, ext2_feature_name, 0x80000001, R_EDX},
         {&guest_def->ext3_features, &host_def.ext3_features,
-            ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}};
+            ~0, ext3_feature_name, 0x80000001, R_ECX}};
 
     assert(kvm_enabled());
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 7/9] target-i386: check/enforce: Eliminate check_feat field

[Eduardo Habkost]

Now that all entries have check_feat=~0 on
kvm_check_features_against_host(), we can eliminate check_feat entirely
and make the code check all bits.

This patch shouldn't introduce any behavior change, as check_feat is set
to ~0 on all entries.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 21aceed..05134a8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -147,7 +147,6 @@ const char *get_register_name_32(unsigned int reg)
 typedef struct model_features_t {
     uint32_t *guest_feat;
     uint32_t *host_feat;
-    uint32_t check_feat;
     const char **flag_names;
     uint32_t cpuid;
     int reg;
@@ -951,8 +950,7 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
 }
 
 /* best effort attempt to inform user requested cpu flags aren't making
- * their way to the guest.  Note: ft[].check_feat ideally should be
- * specified via a guest_def field to suppress report of extraneous flags.
+ * their way to the guest.
  *
  * This function may be called only if KVM is enabled.
  */
@@ -963,20 +961,20 @@ 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,
-            ~0, feature_name, 0x00000001, R_EDX},
+            feature_name, 0x00000001, R_EDX},
         {&guest_def->ext_features, &host_def.ext_features,
-            ~0, ext_feature_name, 0x00000001, R_ECX},
+            ext_feature_name, 0x00000001, R_ECX},
         {&guest_def->ext2_features, &host_def.ext2_features,
-            ~0, ext2_feature_name, 0x80000001, R_EDX},
+            ext2_feature_name, 0x80000001, R_EDX},
         {&guest_def->ext3_features, &host_def.ext3_features,
-            ~0, ext3_feature_name, 0x80000001, R_ECX}};
+            ext3_feature_name, 0x80000001, R_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)
-            if (ft[i].check_feat & mask && *ft[i].guest_feat & mask &&
+            if (*ft[i].guest_feat & mask &&
                 !(*ft[i].host_feat & mask)) {
                     unavailable_host_feature(&ft[i], mask);
                     rv = 1;
-- 
1.7.11.7

[Qemu-devel] [PATCH 8/9] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Eduardo Habkost]

This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 05134a8..02c00bf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -933,6 +933,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 #endif /* CONFIG_KVM */
 }
 
+#ifdef CONFIG_KVM
 static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
 {
     int i;
@@ -981,6 +982,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
                 }
     return rv;
 }
+#endif
 
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
@@ -1404,10 +1406,12 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *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;
+#ifdef CONFIG_KVM
     if (check_cpuid && kvm_enabled()) {
         if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
             goto error;
     }
+#endif
     return 0;
 
 error:
-- 
1.7.11.7

[Qemu-devel] [PATCH 9/9] 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.
---
 target-i386/cpu.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 02c00bf..543ca34 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -950,8 +950,9 @@ static int unavailable_host_feature(struct model_features_t *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.
  */
@@ -968,7 +969,16 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
         {&guest_def->ext2_features, &host_def.ext2_features,
             ext2_feature_name, 0x80000001, R_EDX},
         {&guest_def->ext3_features, &host_def.ext3_features,
-            ext3_feature_name, 0x80000001, R_ECX}};
+            ext3_feature_name, 0x80000001, R_ECX},
+        {&guest_def->ext4_features, &host_def.ext4_features,
+            NULL, 0xC0000001, R_EDX},
+        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
+            cpuid_7_0_ebx_feature_name, 7, R_EBX},
+        {&guest_def->svm_features, &host_def.svm_features,
+            svm_feature_name, 0x8000000A, R_EDX},
+        {&guest_def->kvm_features, &host_def.kvm_features,
+            kvm_feature_name, KVM_CPUID_FEATURES, R_EAX},
+    };
 
     assert(kvm_enabled());
 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 0/9] target-i386: make "enforce" flag work as it should

[Eduardo Habkost]

The Cc header of the message looks broken. It contained:

  Cc: =?utf-8?q?kvm=40vger=2Ekernel=2Eorg=2C_Gleb_Natapov_=3Cgleb=40redhat=2E?=.=?utf-8?q?com=3E=2C_libvir-list=40redhat=2Ecom=2C_Marcelo_Tosatti_=3Cmtosat?=.=?utf-8?q?ti=40redhat=2Ecom=3E=2C_Igor_Mammedov_=3Cimammedo=40redhat=2Ecom?=.=?utf-8?q?=3E=2C_Jiri_Den

On Fri, Jan 04, 2013 at 08:21:22PM -0000, Anthony Liguori wrote:
> Hi,
> 
> This is an automated message generated from the QEMU Patches.
> Thank you for submitting this patch.  This patch no longer applies to qemu.git.

Cool.  :-)

> 
> This may have occurred due to:
>  
>   1) Changes in mainline requiring your patch to be rebased and re-tested.
> 
>   2) Sending the mail using a tool other than git-send-email.  Please use
>      git-send-email to send patches to QEMU.
> 
>   3) Basing this patch off of a branch that isn't tracking the QEMU
>      master branch.  If that was done purposefully, please include the name
>      of the tree in the subject line in the future to prevent this message.
> 
>      For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature"

What should I do if the patch requires other patches that are on the
mailing list but no git tree yet? That was the case, here (the series
depends on the disable-kvm_mmu_op series I sent previously today).

> 
>   4) You no longer wish for this patch to be applied to QEMU.  No additional
>      action is required on your part.
> 
> Nacked-by: QEMU Patches <aliguori@us.ibm.com>
> 
> Below is the output from git-am:
> 
>     Applying: target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features
>     Applying: target-i386: kvm: Enable all supported KVM features for -cpu host
>     Applying: target-i386: check/enforce: Fix CPUID leaf numbers on error messages
>     fatal: sha1 information is lacking or useless (target-i386/cpu.c).
>     Repository lacks necessary blobs to fall back on 3-way merge.
>     Cannot fall back to three-way merge.
>     Patch failed at 0003 target-i386: check/enforce: Fix CPUID leaf numbers on error messages
>     The copy of the patch that failed is found in:
>        /home/aliguori/.patches/qemu-working/.git/rebase-apply/patch
>     When you have resolved this problem run "git am --resolved".
>     If you would prefer to skip this patch, instead run "git am --skip".
>     To restore the original branch and stop patching run "git am --abort".
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/9] target-i386: make "enforce" flag work as it should

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> The Cc header of the message looks broken. It contained:
>
>   Cc:
>   =?utf-8?q?kvm=40vger=2Ekernel=2Eorg=2C_Gleb_Natapov_=3Cgleb=40redhat=2E?=.=?utf-8?q?com=3E=2C_libvir-list=40redhat=2Ecom=2C_Marcelo_Tosatti_=3Cmtosat?=.=?utf-8?q?ti=40redhat=2Ecom=3E=2C_Igor_Mammedov_=3Cimammedo=40redhat=2Ecom?=.=?utf-8?q?=3E=2C_Jiri_Den

Yes, email sucks :-(

I'm working to fix it... sorry for the noise.

>
> On Fri, Jan 04, 2013 at 08:21:22PM -0000, Anthony Liguori wrote:
>> Hi,
>> 
>> This is an automated message generated from the QEMU Patches.
>> Thank you for submitting this patch.  This patch no longer applies to qemu.git.
>
> Cool.  :-)
>
>> 
>> This may have occurred due to:
>>  
>>   1) Changes in mainline requiring your patch to be rebased and re-tested.
>> 
>>   2) Sending the mail using a tool other than git-send-email.  Please use
>>      git-send-email to send patches to QEMU.
>> 
>>   3) Basing this patch off of a branch that isn't tracking the QEMU
>>      master branch.  If that was done purposefully, please include the name
>>      of the tree in the subject line in the future to prevent this message.
>> 
>>      For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature"
>
> What should I do if the patch requires other patches that are on the
> mailing list but no git tree yet? That was the case, here (the series
> depends on the disable-kvm_mmu_op series I sent previously today).

Good question.  In this case, just say [PATCH qom-cpu...] since this is
against the qom-cpu tree.

Or even an RFC tag if that's appropriate.

I wouldn't recommend sending patches meant for merging that depend on
other series still on the list without an RFC tag.  For no other reason
that you'll likely confuse someone trying to apply it.

Regards,

Anthony Liguori

>
>> 
>>   4) You no longer wish for this patch to be applied to QEMU.  No additional
>>      action is required on your part.
>> 
>> Nacked-by: QEMU Patches <aliguori@us.ibm.com>
>> 
>> Below is the output from git-am:
>> 
>>     Applying: target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features
>>     Applying: target-i386: kvm: Enable all supported KVM features for -cpu host
>>     Applying: target-i386: check/enforce: Fix CPUID leaf numbers on error messages
>>     fatal: sha1 information is lacking or useless (target-i386/cpu.c).
>>     Repository lacks necessary blobs to fall back on 3-way merge.
>>     Cannot fall back to three-way merge.
>>     Patch failed at 0003 target-i386: check/enforce: Fix CPUID leaf numbers on error messages
>>     The copy of the patch that failed is found in:
>>        /home/aliguori/.patches/qemu-working/.git/rebase-apply/patch
>>     When you have resolved this problem run "git am --resolved".
>>     If you would prefer to skip this patch, instead run "git am --skip".
>>     To restore the original branch and stop patching run "git am --abort".
>> 
>
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH 0/9] target-i386: make "enforce" flag work as it should

[Anthony Liguori]

Hi,

This is an automated message generated from the QEMU Patches.
Thank you for submitting this patch.  This patch no longer applies to qemu.git.

This may have occurred due to:
 
  1) Changes in mainline requiring your patch to be rebased and re-tested.

  2) Sending the mail using a tool other than git-send-email.  Please use
     git-send-email to send patches to QEMU.

  3) Basing this patch off of a branch that isn't tracking the QEMU
     master branch.  If that was done purposefully, please include the name
     of the tree in the subject line in the future to prevent this message.

     For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature"

  4) You no longer wish for this patch to be applied to QEMU.  No additional
     action is required on your part.

Nacked-by: QEMU Patches <aliguori@us.ibm.com>

Below is the output from git-am:

    Applying: target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features
    Applying: target-i386: kvm: Enable all supported KVM features for -cpu host
    Applying: target-i386: check/enforce: Fix CPUID leaf numbers on error messages
    fatal: sha1 information is lacking or useless (target-i386/cpu.c).
    Repository lacks necessary blobs to fall back on 3-way merge.
    Cannot fall back to three-way merge.
    Patch failed at 0003 target-i386: check/enforce: Fix CPUID leaf numbers on error messages
    The copy of the patch that failed is found in:
       /home/aliguori/.patches/git-working/.git/rebase-apply/patch
    When you have resolved this problem run "git am --resolved".
    If you would prefer to skip this patch, instead run "git am --skip".
    To restore the original branch and stop patching run "git am --abort".