[Qemu-devel] [PATCH qom-cpu 00/11] disable-kvm_mmu + -cpu check/enforce fixes (v2)

[Qemu-devel] [PATCH qom-cpu 00/11] disable-kvm_mmu + -cpu check/enforce fixes (v2)

[Eduardo Habkost]

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.v2
  https://github.com/ehabkost/qemu-hacks/tree/cpu-enforce-all.v2

Patches 1-2 disable the "kvm_mmu" feature by default on pc-1.4. Host-side
support for the kvm_mmu feature was removed from the kernel since v3.3
(released in March 2012), it was marked for removal since January 2011 and it's
slower than shadow or hardware assisted paging (see kernel commit fb92045843).
It doesn't make sense to keep it enabled by default.

Also, keeping it enabled by default would cause unnecessary hassle when
libvirt start using the "enforce" option.

Patches 3-11 change 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.


Eduardo Habkost (11):
  target-i386: Don't set any KVM flag by default if KVM is disabled
  target-i386: Disable kvm_mmu_op by default on pc-1.4
  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

 hw/pc_piix.c      |  9 +++++-
 target-i386/cpu.c | 93 +++++++++++++++++++++++++++++++++++++++++--------------
 target-i386/cpu.h |  4 +++
 3 files changed, 81 insertions(+), 25 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH qom-cpu 01/11] 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
CONFIG_KVM and 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
---
 target-i386/cpu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 82685dc..e6435da 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -145,15 +145,17 @@ 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;
+#ifdef CONFIG_KVM
+    if (kvm_enabled()) {
+        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
+    }
+#endif
 }
 
 void host_cpuid(uint32_t function, uint32_t count,
-- 
1.7.11.7

[Qemu-devel] [PATCH qom-cpu 02/11] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Eduardo Habkost]

The kvm_mmu_op feature was removed from the kernel since v3.3 (released
in March 2012), it was marked for removal since January 2011 and it's
slower than shadow or hardware assisted paging (see kernel commit
fb92045843). It doesn't make sense to keep it enabled by default.

Also, keeping it enabled by default would cause unnecessary hassle when
libvirt start using the "enforce" option.

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>

I was planning to reverse the logic of the compat init functions and
make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4()
instead. But that would require changing pc_init_pci_no_kvmclock() and
pc_init_isa() as well. So to keep the changes simple, I am keeping the
pattern used when pc_init_pci_1_3() was introduced, making
pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3().

Changes v2:
 - Coding style fix
 - Removed redundant comments above machine init functions
---
 hw/pc_piix.c      | 9 ++++++++-
 target-i386/cpu.c | 9 +++++++++
 target-i386/cpu.h | 1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 99747a7..a32af6a 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory,
     }
 }
 
+/* machine init function for pc-0.14 - pc-1.2 */
 static void pc_init_pci(QEMUMachineInitArgs *args)
 {
     ram_addr_t ram_size = args->ram_size;
@@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
     pc_init_pci(args);
 }
 
+static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
+{
+    disable_kvm_mmu_op();
+    pc_init_pci_1_3(args);
+}
+
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
     ram_addr_t ram_size = args->ram_size;
@@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = {
     .name = "pc-1.4",
     .alias = "pc",
     .desc = "Standard PC",
-    .init = pc_init_pci_1_3,
+    .init = pc_init_pci_1_4,
     .max_cpus = 255,
     .is_default = 1,
 };
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e6435da..c83a566 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void)
 #endif
 }
 
+void disable_kvm_mmu_op(void)
+{
+#ifdef CONFIG_KVM
+    if (kvm_enabled()) {
+        kvm_default_features &= ~(1UL << KVM_FEATURE_MMU_OP);
+    }
+#endif
+}
+
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1283537..27c8d0c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1219,5 +1219,6 @@ void do_smm_enter(CPUX86State *env1);
 void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 
 void enable_kvm_pv_eoi(void);
+void disable_kvm_mmu_op(void);
 
 #endif /* CPU_I386_H */
-- 
1.7.11.7

[Qemu-devel] [PATCH qom-cpu 03/11] 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 c83a566..c49a97c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -908,13 +908,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 qom-cpu 04/11] 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 c49a97c..e916ae0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -911,6 +911,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 qom-cpu 05/11] 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

Changes v2:
 - Coding style fixes
 - Add assert() for invalid register numbers on
   unavailable_host_feature()
---
 target-i386/cpu.c | 42 +++++++++++++++++++++++++++++++++---------
 target-i386/cpu.h |  3 +++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e916ae0..c3e5db8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -124,6 +124,25 @@ 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 +151,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;
@@ -923,10 +943,13 @@ 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);
+            const char *reg = get_register_name_32(f->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);
             break;
         }
     return 0;
@@ -945,13 +968,14 @@ 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 qom-cpu 06/11] 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 c3e5db8..42c4c99 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -970,7 +970,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 qom-cpu 07/11] 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 42c4c99..ce64b98 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -972,7 +972,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 qom-cpu 08/11] 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.

Changes v2:
 - Coding style fix
---
 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 ce64b98..a9dd959 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -974,7 +974,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 qom-cpu 09/11] 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>
---
Changes v2:
 - Coding style fix
---
 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 a9dd959..1c3c7e1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -148,7 +148,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;
@@ -956,8 +955,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.
  */
@@ -968,13 +966,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, 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());
@@ -982,7 +980,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
     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 qom-cpu 10/11] 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 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,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;
@@ -987,6 +988,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)
@@ -1410,10 +1412,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 qom-cpu 11/11] 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
---
 target-i386/cpu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 876b0f6..52727ad 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -955,8 +955,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.
  */
@@ -973,7 +974,15 @@ 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 qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> CONFIG_KVM and 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
> ---
>  target-i386/cpu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 82685dc..e6435da 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -145,15 +145,17 @@ 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;
> +#ifdef CONFIG_KVM
You do not need ifdef here.

> +    if (kvm_enabled()) {
> +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> +    }
> +#endif
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> -- 
> 1.7.11.7

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 02/11] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:03PM -0200, Eduardo Habkost wrote:
> The kvm_mmu_op feature was removed from the kernel since v3.3 (released
> in March 2012), it was marked for removal since January 2011 and it's
> slower than shadow or hardware assisted paging (see kernel commit
> fb92045843). It doesn't make sense to keep it enabled by default.
> 
Actually it was effectively removed Oct 1 2009 by a68a6a7282373. After 3
and a half years of not having it I think we can safely drop it without
trying to preserve it in older machine types.

> Also, keeping it enabled by default would cause unnecessary hassle when
> libvirt start using the "enforce" option.
> 
> 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>
> 
> I was planning to reverse the logic of the compat init functions and
> make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4()
> instead. But that would require changing pc_init_pci_no_kvmclock() and
> pc_init_isa() as well. So to keep the changes simple, I am keeping the
> pattern used when pc_init_pci_1_3() was introduced, making
> pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3().
> 
> Changes v2:
>  - Coding style fix
>  - Removed redundant comments above machine init functions
> ---
>  hw/pc_piix.c      | 9 ++++++++-
>  target-i386/cpu.c | 9 +++++++++
>  target-i386/cpu.h | 1 +
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 99747a7..a32af6a 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory,
>      }
>  }
>  
> +/* machine init function for pc-0.14 - pc-1.2 */
>  static void pc_init_pci(QEMUMachineInitArgs *args)
>  {
>      ram_addr_t ram_size = args->ram_size;
> @@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
>      pc_init_pci(args);
>  }
>  
> +static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
> +{
> +    disable_kvm_mmu_op();
> +    pc_init_pci_1_3(args);
> +}
> +
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      ram_addr_t ram_size = args->ram_size;
> @@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = {
>      .name = "pc-1.4",
>      .alias = "pc",
>      .desc = "Standard PC",
> -    .init = pc_init_pci_1_3,
> +    .init = pc_init_pci_1_4,
>      .max_cpus = 255,
>      .is_default = 1,
>  };
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e6435da..c83a566 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void)
>  #endif
>  }
>  
> +void disable_kvm_mmu_op(void)
> +{
> +#ifdef CONFIG_KVM
No need for ifdef here too.

> +    if (kvm_enabled()) {
> +        kvm_default_features &= ~(1UL << KVM_FEATURE_MMU_OP);
clear_bit()

> +    }
> +#endif
> +}
> +
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>  {
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 1283537..27c8d0c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1219,5 +1219,6 @@ void do_smm_enter(CPUX86State *env1);
>  void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  
>  void enable_kvm_pv_eoi(void);
> +void disable_kvm_mmu_op(void);
>  
>  #endif /* CPU_I386_H */
> -- 
> 1.7.11.7

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 03/11] target-i386: kvm: -cpu host: Use GET_SUPPORTED_CPUID for SVM features

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:04PM -0200, Eduardo Habkost wrote:
> 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>
Reviewed-by: Gleb Natapov <gleb@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 c83a566..c49a97c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -908,13 +908,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

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 04/11] target-i386: kvm: Enable all supported KVM features for -cpu host

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:05PM -0200, Eduardo Habkost wrote:
> 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>
Reviewed-by: Gleb Natapov <gleb@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 c49a97c..e916ae0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -911,6 +911,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

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost 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>
Reviewed-by: Gleb Natapov <gleb@redhat.com>
But see the question below.

> ---
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: kvm@vger.kernel.org
> 
> Changes v2:
>  - Coding style fixes
>  - Add assert() for invalid register numbers on
>    unavailable_host_feature()
> ---
>  target-i386/cpu.c | 42 +++++++++++++++++++++++++++++++++---------
>  target-i386/cpu.h |  3 +++
>  2 files changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e916ae0..c3e5db8 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -124,6 +124,25 @@ 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 +151,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;
> @@ -923,10 +943,13 @@ 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);
> +            const char *reg = get_register_name_32(f->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);
>              break;
>          }
>      return 0;
> @@ -945,13 +968,14 @@ 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}
Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked?

> +    };
>  
>      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

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

[Gleb Natapov]

On Sun, Jan 06, 2013 at 04:12:54PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost 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>
> Reviewed-by: Gleb Natapov <gleb@redhat.com>
> But see the question below.
Never mind. I found the answer in the following patches :)

> 
> > ---
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Cc: kvm@vger.kernel.org
> > 
> > Changes v2:
> >  - Coding style fixes
> >  - Add assert() for invalid register numbers on
> >    unavailable_host_feature()
> > ---
> >  target-i386/cpu.c | 42 +++++++++++++++++++++++++++++++++---------
> >  target-i386/cpu.h |  3 +++
> >  2 files changed, 36 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e916ae0..c3e5db8 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -124,6 +124,25 @@ 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 +151,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;
> > @@ -923,10 +943,13 @@ 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);
> > +            const char *reg = get_register_name_32(f->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);
> >              break;
> >          }
> >      return 0;
> > @@ -945,13 +968,14 @@ 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}
> Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked?
> 
> > +    };
> >  
> >      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
> 
> --
> 			Gleb.

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 06/11] target-i386: check/enforce: Do not ignore "hypervisor" flag

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:07PM -0200, Eduardo Habkost wrote:
> 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>
Reviewed-by: Gleb Natapov <gleb@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 c3e5db8..42c4c99 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -970,7 +970,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 07/11] target-i386: check/enforce: Check all CPUID.80000001H.EDX bits

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:08PM -0200, Eduardo Habkost wrote:
> 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>
Reviewed-by: Gleb Natapov <gleb@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 42c4c99..ce64b98 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -972,7 +972,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
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 08/11] target-i386: check/enforce: Check SVM flag support as well

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:09PM -0200, Eduardo Habkost wrote:
> 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>
Reviewed-by: Gleb Natapov <gleb@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.
> 
> Changes v2:
>  - Coding style fix
> ---
>  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 ce64b98..a9dd959 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -974,7 +974,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
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 09/11] target-i386: check/enforce: Eliminate check_feat field

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:10PM -0200, Eduardo Habkost wrote:
> 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>
Reviewed-by: Gleb Natapov <gleb@redhat.com>

> ---
> Changes v2:
>  - Coding style fix
> ---
>  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 a9dd959..1c3c7e1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -148,7 +148,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;
> @@ -956,8 +955,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.
>   */
> @@ -968,13 +966,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, 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());
> @@ -982,7 +980,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
>      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
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
> 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 1c3c7e1..876b0f6 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -936,6 +936,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;
> @@ -987,6 +988,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)
> @@ -1410,10 +1412,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
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.

>      return 0;
>  
>  error:
> -- 
> 1.7.11.7
> 

--
			Gleb.

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

[Gleb Natapov]

On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
> 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
> ---
>  target-i386/cpu.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 876b0f6..52727ad 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -955,8 +955,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.
>   */
> @@ -973,7 +974,15 @@ 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},
Since there is not name array for ext4_features they cannot be added or
removed on the command line hence no need to check them, no?

> +        {&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

--
			Gleb.

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

[Eduardo Habkost]

On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > CONFIG_KVM and 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
> > ---
> >  target-i386/cpu.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 82685dc..e6435da 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -145,15 +145,17 @@ 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;
> > +#ifdef CONFIG_KVM
> You do not need ifdef here.

We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
set.

I could also write it as:

    if (kvm_enabled()) {
#ifdef CONFIG_KVM
        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
#endif
    }

But I find it less readable.


> 
> > +    if (kvm_enabled()) {
> > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > +    }
> > +#endif
> >  }
> >  
> >  void host_cpuid(uint32_t function, uint32_t count,
> > -- 
> > 1.7.11.7
> 
> --
> 			Gleb.

-- 
Eduardo

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > > CONFIG_KVM and 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
> > > ---
> > >  target-i386/cpu.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 82685dc..e6435da 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -145,15 +145,17 @@ 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;
> > > +#ifdef CONFIG_KVM
> > You do not need ifdef here.
> 
> We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> set.
> 
> I could also write it as:
> 
>     if (kvm_enabled()) {
> #ifdef CONFIG_KVM
>         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> #endif
>     }
> 
> But I find it less readable.
> 
> 
Why not define KVM_FEATURE_PV_EOI unconditionally?

> > 
> > > +    if (kvm_enabled()) {
> > > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > +    }
> > > +#endif
> > >  }
> > >  
> > >  void host_cpuid(uint32_t function, uint32_t count,
> > > -- 
> > > 1.7.11.7
> > 
> > --
> > 			Gleb.
> 
> -- 
> Eduardo

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 02/11] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Eduardo Habkost]

On Sun, Jan 06, 2013 at 03:38:28PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:03PM -0200, Eduardo Habkost wrote:
> > The kvm_mmu_op feature was removed from the kernel since v3.3 (released
> > in March 2012), it was marked for removal since January 2011 and it's
> > slower than shadow or hardware assisted paging (see kernel commit
> > fb92045843). It doesn't make sense to keep it enabled by default.
> > 
> Actually it was effectively removed Oct 1 2009 by a68a6a7282373. After 3
> and a half years of not having it I think we can safely drop it without
> trying to preserve it in older machine types.

Agreed. Especially considering that the check/enforce code for KVM flags
is currently broken. So probably people using pc-1.0, pc-1.1, pc-1.2 are
probably _not_ getting the kvm_mmu feature exposed to the guest.

> 
> > Also, keeping it enabled by default would cause unnecessary hassle when
> > libvirt start using the "enforce" option.
> > 
> > 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>
> > 
> > I was planning to reverse the logic of the compat init functions and
> > make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4()
> > instead. But that would require changing pc_init_pci_no_kvmclock() and
> > pc_init_isa() as well. So to keep the changes simple, I am keeping the
> > pattern used when pc_init_pci_1_3() was introduced, making
> > pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3().
> > 
> > Changes v2:
> >  - Coding style fix
> >  - Removed redundant comments above machine init functions
> > ---
> >  hw/pc_piix.c      | 9 ++++++++-
> >  target-i386/cpu.c | 9 +++++++++
> >  target-i386/cpu.h | 1 +
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 99747a7..a32af6a 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory,
> >      }
> >  }
> >  
> > +/* machine init function for pc-0.14 - pc-1.2 */
> >  static void pc_init_pci(QEMUMachineInitArgs *args)
> >  {
> >      ram_addr_t ram_size = args->ram_size;
> > @@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
> >      pc_init_pci(args);
> >  }
> >  
> > +static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
> > +{
> > +    disable_kvm_mmu_op();
> > +    pc_init_pci_1_3(args);
> > +}
> > +
> >  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> >  {
> >      ram_addr_t ram_size = args->ram_size;
> > @@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = {
> >      .name = "pc-1.4",
> >      .alias = "pc",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci_1_3,
> > +    .init = pc_init_pci_1_4,
> >      .max_cpus = 255,
> >      .is_default = 1,
> >  };
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e6435da..c83a566 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void)
> >  #endif
> >  }
> >  
> > +void disable_kvm_mmu_op(void)
> > +{
> > +#ifdef CONFIG_KVM
> No need for ifdef here too.

Same case of the previous patch: KVM_FEATURE_MMU_OP is available only if
CONFIG_KVM is set.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages

[Eduardo Habkost]

On Sun, Jan 06, 2013 at 04:12:54PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost 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>
> Reviewed-by: Gleb Natapov <gleb@redhat.com>
> But see the question below.
> 
> > ---
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Cc: kvm@vger.kernel.org
> > 
> > Changes v2:
> >  - Coding style fixes
> >  - Add assert() for invalid register numbers on
> >    unavailable_host_feature()
> > ---
> >  target-i386/cpu.c | 42 +++++++++++++++++++++++++++++++++---------
> >  target-i386/cpu.h |  3 +++
> >  2 files changed, 36 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e916ae0..c3e5db8 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -124,6 +124,25 @@ 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 +151,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;
> > @@ -923,10 +943,13 @@ 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);
> > +            const char *reg = get_register_name_32(f->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);
> >              break;
> >          }
> >      return 0;
> > @@ -945,13 +968,14 @@ 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}
> Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked?

I simply have no idea. I just have a few guesses:
 - PPRO_FEATURES was supposed to be CPUID_EXT2_AMD_ALIASES, as
   the user could want to expose vendor=AMD on a non-AMD host;
 - Maybe CPUID_EXT3_SVM was being excluded because the old code checked
   the host CPUID directly (instead of using GET_SUPPORTED_CPUID), so
   the SVM bit in the host CPU wouldn't tell us anything about nested
   SVM support.

They are removed on in separate patches that follow, and then the
check_feat field is removed completely.

> 
> > +    };
> >  
> >      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
> 
> --
> 			Gleb.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Eduardo Habkost]

On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
> > 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 1c3c7e1..876b0f6 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -936,6 +936,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;
> > @@ -987,6 +988,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)
> > @@ -1410,10 +1412,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
> Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
> ifdef here.

I will do. Igor probably will have to change his "target-i386: move
kvm_check_features_against_host() check to realize time" patch to use
the same approach, too.

-- 
Eduardo

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

[Eduardo Habkost]

On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
> > 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
> > ---
> >  target-i386/cpu.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 876b0f6..52727ad 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -955,8 +955,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.
> >   */
> > @@ -973,7 +974,15 @@ 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},
> Since there is not name array for ext4_features they cannot be added or
> removed on the command line hence no need to check them, no?

In theory, yes. But it won't hurt to check it, and it will be useful to
unify the list of feature words in a single place, so we can be sure the
checking/filtering/setting code at
kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
will all check/filter/set exactly the same feature words.

> 
> > +        {&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
> 
> --
> 			Gleb.

-- 
Eduardo

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote:
> On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
> > On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
> > > 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
> > > ---
> > >  target-i386/cpu.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 876b0f6..52727ad 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -955,8 +955,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.
> > >   */
> > > @@ -973,7 +974,15 @@ 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},
> > Since there is not name array for ext4_features they cannot be added or
> > removed on the command line hence no need to check them, no?
> 
> In theory, yes. But it won't hurt to check it, and it will be useful to
> unify the list of feature words in a single place, so we can be sure the
> checking/filtering/setting code at
> kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
> will all check/filter/set exactly the same feature words.
> 
May be add a name array for the leaf? :)

--
			Gleb.

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

[Eduardo Habkost]

On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > > > CONFIG_KVM and 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
> > > > ---
> > > >  target-i386/cpu.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 82685dc..e6435da 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -145,15 +145,17 @@ 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;
> > > > +#ifdef CONFIG_KVM
> > > You do not need ifdef here.
> > 
> > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> > set.
> > 
> > I could also write it as:
> > 
> >     if (kvm_enabled()) {
> > #ifdef CONFIG_KVM
> >         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > #endif
> >     }
> > 
> > But I find it less readable.
> > 
> > 
> Why not define KVM_FEATURE_PV_EOI unconditionally?

It comes from the KVM kernel headers, that are included only if
CONFIG_KVM is set, and probably won't even compile in non-Linux systems.

I have a dejavu feeling. I believe we had this exact problem before,
maybe about some other #defines that come from the Linux KVM headers and
won't be available in non-Linux systems.

> 
> > > 
> > > > +    if (kvm_enabled()) {
> > > > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > > +    }
> > > > +#endif
> > > >  }
> > > >  
> > > >  void host_cpuid(uint32_t function, uint32_t count,
> > > > -- 
> > > > 1.7.11.7
> > > 
> > > --
> > > 			Gleb.
> > 
> > -- 
> > Eduardo
> 
> --
> 			Gleb.

-- 
Eduardo

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
> On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > > > > CONFIG_KVM and 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
> > > > > ---
> > > > >  target-i386/cpu.c | 8 +++++---
> > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > index 82685dc..e6435da 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -145,15 +145,17 @@ 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;
> > > > > +#ifdef CONFIG_KVM
> > > > You do not need ifdef here.
> > > 
> > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> > > set.
> > > 
> > > I could also write it as:
> > > 
> > >     if (kvm_enabled()) {
> > > #ifdef CONFIG_KVM
> > >         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > #endif
> > >     }
> > > 
> > > But I find it less readable.
> > > 
> > > 
> > Why not define KVM_FEATURE_PV_EOI unconditionally?
> 
> It comes from the KVM kernel headers, that are included only if
> CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
> 
> I have a dejavu feeling. I believe we had this exact problem before,
> maybe about some other #defines that come from the Linux KVM headers and
> won't be available in non-Linux systems.
>
It is better to hide all KVM related differences somewhere in the
headers where no one sees them instead of sprinkle them all over the
code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
part. Or have one ifdef CONFIG_KVM at the beginning of the file and
define enable_kvm_pv_eoi() there and provide empty stub otherwise.

--
			Gleb.

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

[Eduardo Habkost]

On Mon, Jan 07, 2013 at 02:06:38PM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote:
> > On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
> > > On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
> > > > 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
> > > > ---
> > > >  target-i386/cpu.c | 15 ++++++++++++---
> > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 876b0f6..52727ad 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -955,8 +955,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.
> > > >   */
> > > > @@ -973,7 +974,15 @@ 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},
> > > Since there is not name array for ext4_features they cannot be added or
> > > removed on the command line hence no need to check them, no?
> > 
> > In theory, yes. But it won't hurt to check it, and it will be useful to
> > unify the list of feature words in a single place, so we can be sure the
> > checking/filtering/setting code at
> > kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
> > will all check/filter/set exactly the same feature words.
> > 
> May be add a name array for the leaf? :)

If anybody find reliable documentation about the 0xC0000001 CPUID bits,
I would happily do it.  :-)

While we don't have the docs and feature names, I still believe that
having the complete list of feature words in the
kvm_check_features_against_host() code will save us trouble later, for
the same reason we already filter the 0xC0000001 leaf in
filter_features_for_kvm() today.

-- 
Eduardo

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 10:19:15AM -0200, Eduardo Habkost wrote:
> On Mon, Jan 07, 2013 at 02:06:38PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote:
> > > On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
> > > > On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
> > > > > 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
> > > > > ---
> > > > >  target-i386/cpu.c | 15 ++++++++++++---
> > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > index 876b0f6..52727ad 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -955,8 +955,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.
> > > > >   */
> > > > > @@ -973,7 +974,15 @@ 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},
> > > > Since there is not name array for ext4_features they cannot be added or
> > > > removed on the command line hence no need to check them, no?
> > > 
> > > In theory, yes. But it won't hurt to check it, and it will be useful to
> > > unify the list of feature words in a single place, so we can be sure the
> > > checking/filtering/setting code at
> > > kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
> > > will all check/filter/set exactly the same feature words.
> > > 
> > May be add a name array for the leaf? :)
> 
> If anybody find reliable documentation about the 0xC0000001 CPUID bits,
> I would happily do it.  :-)
> 
That's easy :) Just check the kernel:

        /* cpuid 0xC0000001.edx */
        const u32 kvm_supported_word5_x86_features =
                F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
                F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
                F(PMM) | F(PMM_EN);

> While we don't have the docs and feature names, I still believe that
> having the complete list of feature words in the
> kvm_check_features_against_host() code will save us trouble later, for
> the same reason we already filter the 0xC0000001 leaf in
> filter_features_for_kvm() today.
> 
> -- 
> Eduardo

--
			Gleb.

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

[Eduardo Habkost]

On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
> > On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> > > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > > > > > CONFIG_KVM and 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
> > > > > > ---
> > > > > >  target-i386/cpu.c | 8 +++++---
> > > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > index 82685dc..e6435da 100644
> > > > > > --- a/target-i386/cpu.c
> > > > > > +++ b/target-i386/cpu.c
> > > > > > @@ -145,15 +145,17 @@ 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;
> > > > > > +#ifdef CONFIG_KVM
> > > > > You do not need ifdef here.
> > > > 
> > > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> > > > set.
> > > > 
> > > > I could also write it as:
> > > > 
> > > >     if (kvm_enabled()) {
> > > > #ifdef CONFIG_KVM
> > > >         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > > #endif
> > > >     }
> > > > 
> > > > But I find it less readable.
> > > > 
> > > > 
> > > Why not define KVM_FEATURE_PV_EOI unconditionally?
> > 
> > It comes from the KVM kernel headers, that are included only if
> > CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
> > 
> > I have a dejavu feeling. I believe we had this exact problem before,
> > maybe about some other #defines that come from the Linux KVM headers and
> > won't be available in non-Linux systems.
> >
> It is better to hide all KVM related differences somewhere in the
> headers where no one sees them instead of sprinkle them all over the
> code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
> part. Or have one ifdef CONFIG_KVM at the beginning of the file and
> define enable_kvm_pv_eoi() there and provide empty stub otherwise.

If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
around the real implementation. I mean, I don't think this:

  #ifdef CONFIG_KVM
  int enable_kvm_pv_eoi() {
    [...]
  }
  #endif

is any better than this:

  int enable_kvm_pv_eoi() {
  #ifdef CONFIG_KVM
    [...]
  #endif
  }

So this is probably a good reason to duplicate the KVM_FEATURE_*
#defines in the QEMU code, instead?

-- 
Eduardo

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

[Gleb Natapov]

On Mon, Jan 07, 2013 at 10:30:40AM -0200, Eduardo Habkost wrote:
> On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
> > > On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
> > > > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> > > > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > > > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > > > > > > CONFIG_KVM and 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
> > > > > > > ---
> > > > > > >  target-i386/cpu.c | 8 +++++---
> > > > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > > index 82685dc..e6435da 100644
> > > > > > > --- a/target-i386/cpu.c
> > > > > > > +++ b/target-i386/cpu.c
> > > > > > > @@ -145,15 +145,17 @@ 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;
> > > > > > > +#ifdef CONFIG_KVM
> > > > > > You do not need ifdef here.
> > > > > 
> > > > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> > > > > set.
> > > > > 
> > > > > I could also write it as:
> > > > > 
> > > > >     if (kvm_enabled()) {
> > > > > #ifdef CONFIG_KVM
> > > > >         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > > > #endif
> > > > >     }
> > > > > 
> > > > > But I find it less readable.
> > > > > 
> > > > > 
> > > > Why not define KVM_FEATURE_PV_EOI unconditionally?
> > > 
> > > It comes from the KVM kernel headers, that are included only if
> > > CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
> > > 
> > > I have a dejavu feeling. I believe we had this exact problem before,
> > > maybe about some other #defines that come from the Linux KVM headers and
> > > won't be available in non-Linux systems.
> > >
> > It is better to hide all KVM related differences somewhere in the
> > headers where no one sees them instead of sprinkle them all over the
> > code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
> > part. Or have one ifdef CONFIG_KVM at the beginning of the file and
> > define enable_kvm_pv_eoi() there and provide empty stub otherwise.
> 
> If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
> around the real implementation. I mean, I don't think this:
> 
>   #ifdef CONFIG_KVM
>   int enable_kvm_pv_eoi() {
>     [...]
>   }
>   #endif
> 
You already have #ifdef CONFIG_KVM just above enable_kvm_pv_eoi(). Put
everything KVM related there instead of adding #ifdef CONFIG_KVM all
over the file.

> is any better than this:
> 
>   int enable_kvm_pv_eoi() {
>   #ifdef CONFIG_KVM
>     [...]
>   #endif
>   }
> 
> So this is probably a good reason to duplicate the KVM_FEATURE_*
> #defines in the QEMU code, instead?
> 
Not even duplicate, they can be fake just to keep compiler happy.

--
			Gleb.

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

[Eduardo Habkost]

On Mon, Jan 07, 2013 at 02:33:25PM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 10:30:40AM -0200, Eduardo Habkost wrote:
> > On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
> > > > On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
> > > > > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> > > > > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > > > > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost 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
> > > > > > > > CONFIG_KVM and 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
> > > > > > > > ---
> > > > > > > >  target-i386/cpu.c | 8 +++++---
> > > > > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > > > index 82685dc..e6435da 100644
> > > > > > > > --- a/target-i386/cpu.c
> > > > > > > > +++ b/target-i386/cpu.c
> > > > > > > > @@ -145,15 +145,17 @@ 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;
> > > > > > > > +#ifdef CONFIG_KVM
> > > > > > > You do not need ifdef here.
> > > > > > 
> > > > > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> > > > > > set.
> > > > > > 
> > > > > > I could also write it as:
> > > > > > 
> > > > > >     if (kvm_enabled()) {
> > > > > > #ifdef CONFIG_KVM
> > > > > >         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > > > > #endif
> > > > > >     }
> > > > > > 
> > > > > > But I find it less readable.
> > > > > > 
> > > > > > 
> > > > > Why not define KVM_FEATURE_PV_EOI unconditionally?
> > > > 
> > > > It comes from the KVM kernel headers, that are included only if
> > > > CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
> > > > 
> > > > I have a dejavu feeling. I believe we had this exact problem before,
> > > > maybe about some other #defines that come from the Linux KVM headers and
> > > > won't be available in non-Linux systems.
> > > >
> > > It is better to hide all KVM related differences somewhere in the
> > > headers where no one sees them instead of sprinkle them all over the
> > > code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
> > > part. Or have one ifdef CONFIG_KVM at the beginning of the file and
> > > define enable_kvm_pv_eoi() there and provide empty stub otherwise.
> > 
> > If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
> > around the real implementation. I mean, I don't think this:
> > 
> >   #ifdef CONFIG_KVM
> >   int enable_kvm_pv_eoi() {
> >     [...]
> >   }
> >   #endif
> > 
> You already have #ifdef CONFIG_KVM just above enable_kvm_pv_eoi(). Put
> everything KVM related there instead of adding #ifdef CONFIG_KVM all
> over the file.

But it also creates the need to write a separate stub function somewhere
else, while we could have a ready-to-use stub function automatically by
simply #ifdefing the whole function body. But anyway: this won't matter
if we choose the duplicate/fake #defines approach mentioned below.


> 
> > is any better than this:
> > 
> >   int enable_kvm_pv_eoi() {
> >   #ifdef CONFIG_KVM
> >     [...]
> >   #endif
> >   }
> > 
> > So this is probably a good reason to duplicate the KVM_FEATURE_*
> > #defines in the QEMU code, instead?
> > 
> Not even duplicate, they can be fake just to keep compiler happy.

I believe this would be even better. I will try that in the next version
of this series.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Igor Mammedov]

On Mon, 7 Jan 2013 10:00:09 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
> > On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
> > > 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 1c3c7e1..876b0f6 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -936,6 +936,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;
> > > @@ -987,6 +988,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)
> > > @@ -1410,10 +1412,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
> > Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
> > ifdef here.
> 
> I will do. Igor probably will have to change his "target-i386: move
> kvm_check_features_against_host() check to realize time" patch to use
> the same approach, too.


Gleb,

Why do stub here? As result we will be adding more ifdef-s just in other
places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
kvm_check_features_against_host() are bundled together in cpu.c so we could
instead ifdef whole block. Like here:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html

For me code looks more readable with ifdef here, if we have stub, a reader
would have to look at kvm_check_features_against_host() body to see if it does
anything.

> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Gleb Natapov]

On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
> On Mon, 7 Jan 2013 10:00:09 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
> > > On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
> > > > 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 1c3c7e1..876b0f6 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -936,6 +936,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;
> > > > @@ -987,6 +988,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)
> > > > @@ -1410,10 +1412,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
> > > Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
> > > ifdef here.
> > 
> > I will do. Igor probably will have to change his "target-i386: move
> > kvm_check_features_against_host() check to realize time" patch to use
> > the same approach, too.
> 
> 
> Gleb,
> 
> Why do stub here? As result we will be adding more ifdef-s just in other
> places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
Why will we be adding more ifdef-s in other places?

> kvm_check_features_against_host() are bundled together in cpu.c so we could
> instead ifdef whole block. Like here:
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
> 
That's fine, but you can avoid things like:

     if (kvm_enabled() && name && strcmp(name, "host") == 0) {
+#ifdef CONFIG_KVM
         kvm_cpu_fill_host(x86_cpu_def);
+#endif

in your patch by providing stub for kvm_cpu_fill_host() for !CONFIG_KVM
case. This is common practice really. Avoid ifdefs in the code.

> For me code looks more readable with ifdef here, if we have stub, a reader
> would have to look at kvm_check_features_against_host() body to see if it does
> anything.
> 
If reader cares about kvm it has to anyway. If he does not, there is
friendly kvm_enabled() (which is stub in case of !CONFIG_KVM BTW) to
tell him that he does not care. No need additional ifdef there.

--
			Gleb.

Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Eduardo Habkost]

On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
> On Mon, 7 Jan 2013 10:00:09 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
> > > On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
> > > > 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 1c3c7e1..876b0f6 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -936,6 +936,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;
> > > > @@ -987,6 +988,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)
> > > > @@ -1410,10 +1412,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
> > > Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
> > > ifdef here.
> > 
> > I will do. Igor probably will have to change his "target-i386: move
> > kvm_check_features_against_host() check to realize time" patch to use
> > the same approach, too.
> 
> 
> Gleb,
> 
> Why do stub here? As result we will be adding more ifdef-s just in other
> places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
> kvm_check_features_against_host() are bundled together in cpu.c so we could
> instead ifdef whole block. Like here:
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
> 
> For me code looks more readable with ifdef here, if we have stub, a reader
> would have to look at kvm_check_features_against_host() body to see if it does
> anything.

If CONFIG_KVM is not set, kvm_enabled() is always zero, so the function
would never be called, so I find the ifdef-less code more readable and
obvious.

What I don't know is if we should do this:

  #ifdef CONFIG_KVM
  
  static int kvm_check_features_against_host(...)
  {
      /* real implementation here */
  }
  
  static int kvm_do_something_else(...)
  {
      /* real implementation here */
  }
  
  /* Other kvm_* functions here */
  
  #else
  
  static int kvm_check_features_against_host(...)
  {
  }
  
  static int kvm_do_something_else(...)
  {
  }
  
  /* Other kvm_* stubs here */
  
  #endif /* CONFIG_KVM */


Or this:

  static int kvm_check_features_against_host(...)
  {
  #ifdef CONFIG_KVM
      /* real implementation here */
  #endif /* CONFIG_KVM */
  }
  
  static int kvm_do_something_else(...)
  {
  #ifdef CONFIG_KVM
      /* real implementation here */
  #endif /* CONFIG_KVM */
  }


I believe the latter is better, but based on Gleb's comments about
enable_kvm_pv_eoi(), he seems to prefer the former.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set

[Igor Mammedov]

On Mon, 7 Jan 2013 15:30:26 +0200
Gleb Natapov <gleb@redhat.com> wrote:

> On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
> > On Mon, 7 Jan 2013 10:00:09 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
> > > > On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
> > > > > 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 1c3c7e1..876b0f6 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -936,6 +936,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;
> > > > > @@ -987,6 +988,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)
> > > > > @@ -1410,10 +1412,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
> > > > Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
> > > > ifdef here.
> > > 
> > > I will do. Igor probably will have to change his "target-i386: move
> > > kvm_check_features_against_host() check to realize time" patch to use
> > > the same approach, too.
> > 
> > 
> > Gleb,
> > 
> > Why do stub here? As result we will be adding more ifdef-s just in other
> > places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
> Why will we be adding more ifdef-s in other places?
unavailable_host_feature() is being ifdef-ed above

> 
> > kvm_check_features_against_host() are bundled together in cpu.c so we could
> > instead ifdef whole block. Like here:
> > http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
> > 
> That's fine, but you can avoid things like:
> 
>      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> +#ifdef CONFIG_KVM
>          kvm_cpu_fill_host(x86_cpu_def);
> +#endif
> 
> in your patch by providing stub for kvm_cpu_fill_host() for !CONFIG_KVM
> case. This is common practice really. Avoid ifdefs in the code.
This ifdef could be eliminated later when cpus are converted into sub-classes.
Then we would put host subclass close to kvm_cpu_fill_host inside of the same
ifdef. that would leave ifdef around kvm_check_features_against_host() in
cpu_x86_parse_featurestr().

> 
> > For me code looks more readable with ifdef here, if we have stub, a reader
> > would have to look at kvm_check_features_against_host() body to see if it does
> > anything.
> > 
> If reader cares about kvm it has to anyway. If he does not, there is
> friendly kvm_enabled() (which is stub in case of !CONFIG_KVM BTW) to
> tell him that he does not care. No need additional ifdef there.

both ways would work, but if stubs are preferred style then there is no
point arguing.

> 
> --
> 			Gleb.


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH qom-cpu 00/11] disable-kvm_mmu + -cpu check/enforce fixes (v2)

[Andreas Färber]

Am 04.01.2013 23:01, schrieb Eduardo Habkost:
> Eduardo Habkost (11):
[...]
>   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
[snip]

Thanks, applied patches 3-9 to qom-cpu queue (fixing some typos in
commit messages):
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