[Qemu-devel] [PATCH v3 0/6] target-i386: Make most CPU models work with "enforce" out of the box

[Qemu-devel] [PATCH v3 0/6] target-i386: Make most CPU models work with "enforce" out of the box

[Eduardo Habkost]

Changes v2 -> v3:
 * None. This is just a rebase against latest qemu.git master (commit b00a0dd)

Changes v1 -> v2:
 * Commit message and comment changes.
 * Update compat code to change pc-*-2.1, not pc-*-2.0.
 * Added patch to disable SVM by default in KVM mode.

Most of the bits that make "enforce" breaks were introduced in 2010 by commit
8560efed6a72a816c0115f41ddb9d79f7ce63f28. The intention behind that commit made
sense, the only problem is that we can't guarantee guest ABI stability across
hosts if we simply rely on trimming of CPU features based on host capabilities.

So, this series remove CPUID bits from the CPU model definitions so they become
defaults that: 1) won't unexpectly stop working when we start using the
"enforce" flag; 2) won't silently break the guest ABI when TCG or KVM start
supporting new features.

There's only one non-trivial case left: the qemu32/qemu64 models. The problem
with them is that we have conflicting expectations about it, from different
users:

TCG users expect the default CPU model to contain most TCG-supported features
(and it makes sense). See, for example, commit
f1e00a9cf326acc1f2386a72525af8859852e1df.

KVM users expect the default CPU model to be a conservative choice which will
work on most host CPUs (and will only contain features that are supported by
KVM).

We could solve the qemu32/qemu64 issue by having different defaults for TCG and
KVM. But we have existing management code (libvirt) that already expects qemu32
or qemu64 to be the default, and changing the default would break that code. I
will send an RFC to address that later.

Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org

Eduardo Habkost (6):
  pc: Create pc_compat_2_1() functions
  target-i386: Rename KVM auto-feature-enable compat function
  target-i386: Disable CPUID_ACPI by default on KVM mode
  target-i386: Remove unsupported bits from all CPU models
  target-i386: Don't enable nested VMX by default
  target-i386: Disable SVM by default in KVM mode

 hw/i386/pc_piix.c | 22 ++++++++++++++++++----
 hw/i386/pc_q35.c  | 18 ++++++++++++++++--
 target-i386/cpu.c | 42 ++++++++++++++++++++++++++++--------------
 target-i386/cpu.h |  3 ++-
 4 files changed, 64 insertions(+), 21 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/6] pc: Create pc_compat_2_1() functions

[Eduardo Habkost]

We will need new compat code for the 2.1 machine-types.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c | 13 ++++++++++++-
 hw/i386/pc_q35.c  | 13 ++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 103d756..0a4f4d3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -302,8 +302,13 @@ static void pc_init_pci(MachineState *machine)
     pc_init1(machine, 1, 1);
 }
 
+static void pc_compat_2_1(MachineState *machine)
+{
+}
+
 static void pc_compat_2_0(MachineState *machine)
 {
+    pc_compat_2_1(machine);
     /* This value depends on the actual DSDT and SSDT compiled into
      * the source QEMU; unfortunately it depends on the binary and
      * not on the machine type, so we cannot make pc-i440fx-1.7 work on
@@ -368,6 +373,12 @@ static void pc_compat_1_2(MachineState *machine)
     x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
 }
 
+static void pc_init_pci_2_1(MachineState *machine)
+{
+    pc_compat_2_1(machine);
+    pc_init_pci(machine);
+}
+
 static void pc_init_pci_2_0(MachineState *machine)
 {
     pc_compat_2_0(machine);
@@ -471,7 +482,7 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
 static QEMUMachine pc_i440fx_machine_v2_1 = {
     PC_I440FX_2_1_MACHINE_OPTIONS,
     .name = "pc-i440fx-2.1",
-    .init = pc_init_pci,
+    .init = pc_init_pci_2_1,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_2_1,
         { /* end of list */ }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d4a907c..8f46bf7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -276,8 +276,13 @@ static void pc_q35_init(MachineState *machine)
     }
 }
 
+static void pc_compat_2_1(MachineState *machine)
+{
+}
+
 static void pc_compat_2_0(MachineState *machine)
 {
+    pc_compat_2_1(machine);
     smbios_legacy_mode = true;
     has_reserved_memory = false;
     pc_set_legacy_acpi_data_size();
@@ -311,6 +316,12 @@ static void pc_compat_1_4(MachineState *machine)
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
 }
 
+static void pc_q35_init_2_1(MachineState *machine)
+{
+    pc_compat_2_1(machine);
+    pc_q35_init(machine);
+}
+
 static void pc_q35_init_2_0(MachineState *machine)
 {
     pc_compat_2_0(machine);
@@ -362,7 +373,7 @@ static QEMUMachine pc_q35_machine_v2_2 = {
 static QEMUMachine pc_q35_machine_v2_1 = {
     PC_Q35_2_1_MACHINE_OPTIONS,
     .name = "pc-q35-2.1",
-    .init = pc_q35_init,
+    .init = pc_q35_init_2_1,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_2_1,
         { /* end of list */ }
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/6] target-i386: Rename KVM auto-feature-enable compat function

[Eduardo Habkost]

The x86_cpu_compat_disable_kvm_features() name was a bit confusing, as
it won't forcibly disable the feature for all CPU models (i.e. add it to
kvm_default_unset_features), but it will instead turn off the KVM
auto-enabling of the feature (i.e. remove it from kvm_default_features),
meaning the feature may still be enabled by default in some CPU models).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c | 6 +++---
 hw/i386/pc_q35.c  | 2 +-
 target-i386/cpu.c | 2 +-
 target-i386/cpu.h | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0a4f4d3..d2d34d7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -338,7 +338,7 @@ static void pc_compat_1_7(MachineState *machine)
     gigabyte_align = false;
     option_rom_has_mr = true;
     legacy_acpi_table_size = 6414;
-    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
+    x86_cpu_compat_kvm_no_autoenable(FEAT_1_ECX, CPUID_EXT_X2APIC);
 }
 
 static void pc_compat_1_6(MachineState *machine)
@@ -370,7 +370,7 @@ static void pc_compat_1_3(MachineState *machine)
 static void pc_compat_1_2(MachineState *machine)
 {
     pc_compat_1_3(machine);
-    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
+    x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, KVM_FEATURE_PV_EOI);
 }
 
 static void pc_init_pci_2_1(MachineState *machine)
@@ -441,7 +441,7 @@ static void pc_init_isa(MachineState *machine)
     if (!machine->cpu_model) {
         machine->cpu_model = "486";
     }
-    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
+    x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, KVM_FEATURE_PV_EOI);
     enable_compat_apic_id_mode();
     pc_init1(machine, 0, 1);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8f46bf7..d06481c 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -294,7 +294,7 @@ static void pc_compat_1_7(MachineState *machine)
     smbios_defaults = false;
     gigabyte_align = false;
     option_rom_has_mr = true;
-    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
+    x86_cpu_compat_kvm_no_autoenable(FEAT_1_ECX, CPUID_EXT_X2APIC);
 }
 
 static void pc_compat_1_6(MachineState *machine)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7bf9de..b55950b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -464,7 +464,7 @@ static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
     [FEAT_1_ECX] = CPUID_EXT_MONITOR,
 };
 
-void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features)
+void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features)
 {
     kvm_default_features[w] &= ~features;
 }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 2968749..8d932ae 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1304,7 +1304,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
                                  uint32_t feat_add, uint32_t feat_remove);
 
-void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features);
+void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features);
 
 
 /* Return name of 32-bit register, from a R_* constant */
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/6] target-i386: Disable CPUID_ACPI by default on KVM mode

[Eduardo Habkost]

KVM never supported the CPUID_ACPI flag, so it doesn't make sense to
have it enabled by default when KVM is enabled.

The motivation here is exactly the same we had for the MONITOR flag
(disabled by commit 136a7e9a85d7047461f8153f7d12c514a3d68f69).

And like on the MONITOR flag case, we don't need machine-type compat code
because it is currently impossible to run a KVM VM with the ACPI flag set.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b55950b..4119fca 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -461,6 +461,7 @@ static uint32_t kvm_default_features[FEATURE_WORDS] = {
 /* Features that are not added by default to any CPU model when KVM is enabled.
  */
 static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
+    [FEAT_1_EDX] = CPUID_ACPI,
     [FEAT_1_ECX] = CPUID_EXT_MONITOR,
 };
 
-- 
1.9.3

[Qemu-devel] [PATCH v3 4/6] target-i386: Remove unsupported bits from all CPU models

[Eduardo Habkost]

The following CPU features were never supported by neither TCG or KVM,
so they are useless on the CPU model definitions, today:

 * CPUID_DTS (DS)
 * CPUID_HT
 * CPUID_TM
 * CPUID_PBE
 * CPUID_EXT_DTES64
 * CPUID_EXT_DSCPL
 * CPUID_EXT_EST
 * CPUID_EXT_TM2
 * CPUID_EXT_XTPR
 * CPUID_EXT_PDCM
 * CPUID_SVM_LBRV

As using "enforce" mode is the only way to ensure guest ABI doesn't
change when moving to a different host, we should make "enforce" mode
the default or at least encourage management software to always use it.

In turn, to make "enforce" usable, we need CPU models that work without
always requiring some features to be explicitly disabled. This patch
removes the above features from all CPU model definitions.

We won't need any machine-type compat code for those changes, because it
is impossible to have existing VMs with those features enabled.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
---
Changes v1 -> v2:
* Trivial typo fix in comment
---
 target-i386/cpu.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4119fca..1e9fff9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -681,10 +681,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .family = 16,
         .model = 2,
         .stepping = 3,
+        /* Missing: CPUID_HT */
         .features[FEAT_1_EDX] =
             PPRO_FEATURES |
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
-            CPUID_PSE36 | CPUID_VME | CPUID_HT,
+            CPUID_PSE36 | CPUID_VME,
         .features[FEAT_1_ECX] =
             CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_CX16 |
             CPUID_EXT_POPCNT,
@@ -700,8 +701,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
             CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
+        /* Missing: CPUID_SVM_LBRV */
         .features[FEAT_SVM] =
-            CPUID_SVM_NPT | CPUID_SVM_LBRV,
+            CPUID_SVM_NPT,
         .xlevel = 0x8000001A,
         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
     },
@@ -712,15 +714,16 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .family = 6,
         .model = 15,
         .stepping = 11,
+        /* Missing: CPUID_DTS, CPUID_HT, CPUID_TM, CPUID_PBE */
         .features[FEAT_1_EDX] =
             PPRO_FEATURES |
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
-            CPUID_PSE36 | CPUID_VME | CPUID_DTS | CPUID_ACPI | CPUID_SS |
-            CPUID_HT | CPUID_TM | CPUID_PBE,
+            CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS,
+        /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST,
+         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
         .features[FEAT_1_ECX] =
             CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
-            CPUID_EXT_DTES64 | CPUID_EXT_DSCPL | CPUID_EXT_VMX | CPUID_EXT_EST |
-            CPUID_EXT_TM2 | CPUID_EXT_CX16 | CPUID_EXT_XTPR | CPUID_EXT_PDCM,
+            CPUID_EXT_VMX | CPUID_EXT_CX16,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .features[FEAT_8000_0001_ECX] =
@@ -795,13 +798,15 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .family = 6,
         .model = 14,
         .stepping = 8,
+        /* Missing: CPUID_DTS, CPUID_HT, CPUID_TM, CPUID_PBE */
         .features[FEAT_1_EDX] =
             PPRO_FEATURES | CPUID_VME |
-            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_DTS | CPUID_ACPI |
-            CPUID_SS | CPUID_HT | CPUID_TM | CPUID_PBE,
+            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_ACPI |
+            CPUID_SS,
+        /* Missing: CPUID_EXT_EST, CPUID_EXT_TM2 , CPUID_EXT_XTPR,
+         * CPUID_EXT_PDCM */
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_VMX |
-            CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR | CPUID_EXT_PDCM,
+            CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_VMX,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_NX,
         .xlevel = 0x80000008,
@@ -874,14 +879,16 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .family = 6,
         .model = 28,
         .stepping = 2,
+        /* Missing: CPUID_DTS, CPUID_HT, CPUID_TM, CPUID_PBE */
         .features[FEAT_1_EDX] =
             PPRO_FEATURES |
-            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_VME | CPUID_DTS |
-            CPUID_ACPI | CPUID_SS | CPUID_HT | CPUID_TM | CPUID_PBE,
+            CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_VME |
+            CPUID_ACPI | CPUID_SS,
             /* Some CPUs got no CPUID_SEP */
+        /* Missing: CPUID_EXT_DSCPL, CPUID_EXT_EST, CPUID_EXT_TM2,
+         * CPUID_EXT_XTPR */
         .features[FEAT_1_ECX] =
             CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
-            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR |
             CPUID_EXT_MOVBE,
         .features[FEAT_8000_0001_EDX] =
             (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 4/6] target-i386: Remove unsupported bits from all CPU models

[Andreas Färber]

Am 03.10.2014 um 21:39 schrieb Eduardo Habkost:
> The following CPU features were never supported by neither TCG or KVM,
> so they are useless on the CPU model definitions, today:
> 
>  * CPUID_DTS (DS)
>  * CPUID_HT
>  * CPUID_TM
>  * CPUID_PBE
>  * CPUID_EXT_DTES64
>  * CPUID_EXT_DSCPL
>  * CPUID_EXT_EST
>  * CPUID_EXT_TM2
>  * CPUID_EXT_XTPR
>  * CPUID_EXT_PDCM
>  * CPUID_SVM_LBRV
> 
> As using "enforce" mode is the only way to ensure guest ABI doesn't
> change when moving to a different host, we should make "enforce" mode
> the default or at least encourage management software to always use it.
> 
> In turn, to make "enforce" usable, we need CPU models that work without
> always requiring some features to be explicitly disabled. This patch
> removes the above features from all CPU model definitions.
> 
> We won't need any machine-type compat code for those changes, because it
> is impossible to have existing VMs with those features enabled.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> ---
> Changes v1 -> v2:
> * Trivial typo fix in comment
> ---
>  target-i386/cpu.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4119fca..1e9fff9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -681,10 +681,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .family = 16,
>          .model = 2,
>          .stepping = 3,
> +        /* Missing: CPUID_HT */
>          .features[FEAT_1_EDX] =
>              PPRO_FEATURES |
>              CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> -            CPUID_PSE36 | CPUID_VME | CPUID_HT,
> +            CPUID_PSE36 | CPUID_VME,
>          .features[FEAT_1_ECX] =
>              CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_CX16 |
>              CPUID_EXT_POPCNT,
[snip]

I'm okay with retaining these as comments. Anyone any objections?

Andreas

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

[Qemu-devel] [PATCH v3 5/6] target-i386: Don't enable nested VMX by default

[Eduardo Habkost]

TCG doesn't support VMX, and nested VMX is not enabled by default on the
KVM kernel module.

So, there's no reason to have VMX enabled by default on the core2duo and
coreduo CPU models, today. Even the newer Intel CPU model definitions
don't have it enabled.

In this case, we need machine-type compat code, as people may be running
the older machine-types on hosts that had VMX nesting enabled.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c | 2 ++
 hw/i386/pc_q35.c  | 2 ++
 target-i386/cpu.c | 8 ++++----
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index d2d34d7..1d5d57e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -304,6 +304,8 @@ static void pc_init_pci(MachineState *machine)
 
 static void pc_compat_2_1(MachineState *machine)
 {
+    x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
+    x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
 }
 
 static void pc_compat_2_0(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d06481c..c8f7a88 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -278,6 +278,8 @@ static void pc_q35_init(MachineState *machine)
 
 static void pc_compat_2_1(MachineState *machine)
 {
+    x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
+    x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
 }
 
 static void pc_compat_2_0(MachineState *machine)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1e9fff9..c336003 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -720,10 +720,10 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
             CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS,
         /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST,
-         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
+         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_VMX */
         .features[FEAT_1_ECX] =
             CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
-            CPUID_EXT_VMX | CPUID_EXT_CX16,
+            CPUID_EXT_CX16,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .features[FEAT_8000_0001_ECX] =
@@ -804,9 +804,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_ACPI |
             CPUID_SS,
         /* Missing: CPUID_EXT_EST, CPUID_EXT_TM2 , CPUID_EXT_XTPR,
-         * CPUID_EXT_PDCM */
+         * CPUID_EXT_PDCM, CPUID_EXT_VMX */
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_VMX,
+            CPUID_EXT_SSE3 | CPUID_EXT_MONITOR,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_NX,
         .xlevel = 0x80000008,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 5/6] target-i386: Don't enable nested VMX by default

[Andreas Färber]

Am 03.10.2014 um 21:39 schrieb Eduardo Habkost:
> TCG doesn't support VMX, and nested VMX is not enabled by default on the
> KVM kernel module.
> 
> So, there's no reason to have VMX enabled by default on the core2duo and
> coreduo CPU models, today. Even the newer Intel CPU model definitions
> don't have it enabled.
> 
> In this case, we need machine-type compat code, as people may be running
> the older machine-types on hosts that had VMX nesting enabled.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc_piix.c | 2 ++
>  hw/i386/pc_q35.c  | 2 ++
>  target-i386/cpu.c | 8 ++++----
>  3 files changed, 8 insertions(+), 4 deletions(-)
[...]
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1e9fff9..c336003 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -720,10 +720,10 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
>              CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS,
>          /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST,
> -         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
> +         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_VMX */
>          .features[FEAT_1_ECX] =
>              CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
> -            CPUID_EXT_VMX | CPUID_EXT_CX16,
> +            CPUID_EXT_CX16,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>          .features[FEAT_8000_0001_ECX] =
[snip]

Here I'm less certain what the best approach is. As you point out,
there's an inconsistency that I agree should be fixed. I wonder however
whether an approach similar to 3/6 for KVM only would be better? I.e.,
have VMX as a sometimes-KVM-supported feature be listed in the model and
filter it out for accel=kvm so that -cpu enforce works, but let
accel=tcg fail with features not implemented.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v3 5/6] target-i386: Don't enable nested VMX by default

[Eduardo Habkost]

On Wed, Oct 29, 2014 at 06:40:48PM +0100, Andreas Färber wrote:
> Am 03.10.2014 um 21:39 schrieb Eduardo Habkost:
> > TCG doesn't support VMX, and nested VMX is not enabled by default on the
> > KVM kernel module.
> > 
> > So, there's no reason to have VMX enabled by default on the core2duo and
> > coreduo CPU models, today. Even the newer Intel CPU model definitions
> > don't have it enabled.
> > 
> > In this case, we need machine-type compat code, as people may be running
> > the older machine-types on hosts that had VMX nesting enabled.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/i386/pc_piix.c | 2 ++
> >  hw/i386/pc_q35.c  | 2 ++
> >  target-i386/cpu.c | 8 ++++----
> >  3 files changed, 8 insertions(+), 4 deletions(-)
> [...]
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 1e9fff9..c336003 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -720,10 +720,10 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >              CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
> >              CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS,
> >          /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST,
> > -         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
> > +         * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_VMX */
> >          .features[FEAT_1_ECX] =
> >              CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
> > -            CPUID_EXT_VMX | CPUID_EXT_CX16,
> > +            CPUID_EXT_CX16,
> >          .features[FEAT_8000_0001_EDX] =
> >              CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> >          .features[FEAT_8000_0001_ECX] =
> [snip]
> 
> Here I'm less certain what the best approach is. As you point out,
> there's an inconsistency that I agree should be fixed. I wonder however
> whether an approach similar to 3/6 for KVM only would be better? I.e.,
> have VMX as a sometimes-KVM-supported feature be listed in the model and
> filter it out for accel=kvm so that -cpu enforce works, but let
> accel=tcg fail with features not implemented.

I don't mind either way, this is up for TCG maintainers. But I don't
understand what would be the benefit in keeping it enabled by default
for TCG.

Fortunately the answer for KVM and TCG are completely independent from
each other, now. We just need to follow this depending on the defaults
we choose:

  | Default on |
  |  TCG | KVM | Implementation
  +------+-----+-------------------------------------------------
A |  NO  | NO  | Unset it on CPU model table (builtin_x86_defs)
B |  NO  | YES | Unset on table, set on kvm_default_features
C |  YES | NO  | Set on table, set on kvm_default_unset_features
B |  YES | YES | Set it on CPU model table
  +------+-----+-------------------------------------------------

This patch implements row A.

If you tell me you really want VMX to be enabled by default on TCG mode,
then we can implement row C like we already did for CPUID_ACPI,
CPUID_EXT_MONITOR, and CPUID_EXT3_SVM.

But, are you really sure you do?

Considering accel=tcg behavior only (ignore KVM by now): why is this
different from all the features listed on patch 1/6? VMX was never
supported by TCG, so (like all the features from 1/6) it is not useful
to have it enabled by default when accel=tcg.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 5/6] target-i386: Don't enable nested VMX by default

[Paolo Bonzini]

> Here I'm less certain what the best approach is. As you point out,
> there's an inconsistency that I agree should be fixed. I wonder however
> whether an approach similar to 3/6 for KVM only would be better? I.e.,
> have VMX as a sometimes-KVM-supported feature be listed in the model and
> filter it out for accel=kvm so that -cpu enforce works, but let
> accel=tcg fail with features not implemented.

This would mean that -cpu coreduo,enforce doesn't work on TCG, but -cpu
Nehalem,enforce works.  This does not make much sense to me.

In fact, I would even omit the x86_cpu_compat_set_features altogether.
The inclusion of vmx in these models was a mistake, and nested VMX is
not really useful with anything but "-cpu host" because there are too
many capabilities communicated via MSRs rather than CPUID.

Paolo

[Qemu-devel] [PATCH v3 6/6] target-i386: Disable SVM by default in KVM mode

[Eduardo Habkost]

Make SVM be disabled by default on all CPU models when in KVM mode.
Nested SVM is enabled by default in the KVM kernel module, but it is
probably less stable than nested VMX (which is already disabled by
default).

Add a new compat function, x86_cpu_compat_kvm_no_autodisable(), to keep
compatibility on previous machine-types.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c | 1 +
 hw/i386/pc_q35.c  | 1 +
 target-i386/cpu.c | 6 ++++++
 target-i386/cpu.h | 1 +
 4 files changed, 9 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1d5d57e..70027b9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -306,6 +306,7 @@ static void pc_compat_2_1(MachineState *machine)
 {
     x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
     x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
+    x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
 }
 
 static void pc_compat_2_0(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c8f7a88..f923a9e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -280,6 +280,7 @@ static void pc_compat_2_1(MachineState *machine)
 {
     x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
     x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
+    x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
 }
 
 static void pc_compat_2_0(MachineState *machine)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c336003..986a94f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -463,6 +463,7 @@ static uint32_t kvm_default_features[FEATURE_WORDS] = {
 static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
     [FEAT_1_EDX] = CPUID_ACPI,
     [FEAT_1_ECX] = CPUID_EXT_MONITOR,
+    [FEAT_8000_0001_ECX] = CPUID_EXT3_SVM,
 };
 
 void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features)
@@ -470,6 +471,11 @@ void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features)
     kvm_default_features[w] &= ~features;
 }
 
+void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features)
+{
+    kvm_default_unset_features[w] &= ~features;
+}
+
 /*
  * Returns the set of feature flags that are supported and migratable by
  * QEMU, for a given FeatureWord.
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8d932ae..8a9b70f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1305,6 +1305,7 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
                                  uint32_t feat_add, uint32_t feat_remove);
 
 void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features);
+void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features);
 
 
 /* Return name of 32-bit register, from a R_* constant */
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 0/6] target-i386: Make most CPU models work with "enforce" out of the box

[Paolo Bonzini]

Il 03/10/2014 21:39, Eduardo Habkost ha scritto:
> Changes v2 -> v3:
>  * None. This is just a rebase against latest qemu.git master (commit b00a0dd)
> 
> Changes v1 -> v2:
>  * Commit message and comment changes.
>  * Update compat code to change pc-*-2.1, not pc-*-2.0.
>  * Added patch to disable SVM by default in KVM mode.
> 
> Most of the bits that make "enforce" breaks were introduced in 2010 by commit
> 8560efed6a72a816c0115f41ddb9d79f7ce63f28. The intention behind that commit made
> sense, the only problem is that we can't guarantee guest ABI stability across
> hosts if we simply rely on trimming of CPU features based on host capabilities.
> 
> So, this series remove CPUID bits from the CPU model definitions so they become
> defaults that: 1) won't unexpectly stop working when we start using the
> "enforce" flag; 2) won't silently break the guest ABI when TCG or KVM start
> supporting new features.
> 
> There's only one non-trivial case left: the qemu32/qemu64 models. The problem
> with them is that we have conflicting expectations about it, from different
> users:
> 
> TCG users expect the default CPU model to contain most TCG-supported features
> (and it makes sense). See, for example, commit
> f1e00a9cf326acc1f2386a72525af8859852e1df.
> 
> KVM users expect the default CPU model to be a conservative choice which will
> work on most host CPUs (and will only contain features that are supported by
> KVM).
> 
> We could solve the qemu32/qemu64 issue by having different defaults for TCG and
> KVM. But we have existing management code (libvirt) that already expects qemu32
> or qemu64 to be the default, and changing the default would break that code. I
> will send an RFC to address that later.
> 
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org
> 
> Eduardo Habkost (6):
>   pc: Create pc_compat_2_1() functions
>   target-i386: Rename KVM auto-feature-enable compat function
>   target-i386: Disable CPUID_ACPI by default on KVM mode
>   target-i386: Remove unsupported bits from all CPU models
>   target-i386: Don't enable nested VMX by default
>   target-i386: Disable SVM by default in KVM mode
> 
>  hw/i386/pc_piix.c | 22 ++++++++++++++++++----
>  hw/i386/pc_q35.c  | 18 ++++++++++++++++--
>  target-i386/cpu.c | 42 ++++++++++++++++++++++++++++--------------
>  target-i386/cpu.h |  3 ++-
>  4 files changed, 64 insertions(+), 21 deletions(-)
> 

Andreas, are you picking up this?

Paolo

Re: [Qemu-devel] [PATCH v3 0/6] target-i386: Make most CPU models work with "enforce" out of the box

[Andreas Färber]

Am 04.10.2014 um 00:16 schrieb Paolo Bonzini:
> Il 03/10/2014 21:39, Eduardo Habkost ha scritto:
>> Changes v2 -> v3:
>>  * None. This is just a rebase against latest qemu.git master (commit b00a0dd)
>>
>> Changes v1 -> v2:
>>  * Commit message and comment changes.
>>  * Update compat code to change pc-*-2.1, not pc-*-2.0.
>>  * Added patch to disable SVM by default in KVM mode.
>>
>> Most of the bits that make "enforce" breaks were introduced in 2010 by commit
>> 8560efed6a72a816c0115f41ddb9d79f7ce63f28. The intention behind that commit made
>> sense, the only problem is that we can't guarantee guest ABI stability across
>> hosts if we simply rely on trimming of CPU features based on host capabilities.
>>
>> So, this series remove CPUID bits from the CPU model definitions so they become
>> defaults that: 1) won't unexpectly stop working when we start using the
>> "enforce" flag; 2) won't silently break the guest ABI when TCG or KVM start
>> supporting new features.
>>
>> There's only one non-trivial case left: the qemu32/qemu64 models. The problem
>> with them is that we have conflicting expectations about it, from different
>> users:
>>
>> TCG users expect the default CPU model to contain most TCG-supported features
>> (and it makes sense). See, for example, commit
>> f1e00a9cf326acc1f2386a72525af8859852e1df.
>>
>> KVM users expect the default CPU model to be a conservative choice which will
>> work on most host CPUs (and will only contain features that are supported by
>> KVM).
>>
>> We could solve the qemu32/qemu64 issue by having different defaults for TCG and
>> KVM. But we have existing management code (libvirt) that already expects qemu32
>> or qemu64 to be the default, and changing the default would break that code. I
>> will send an RFC to address that later.
>>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: kvm@vger.kernel.org
>>
>> Eduardo Habkost (6):
>>   pc: Create pc_compat_2_1() functions
>>   target-i386: Rename KVM auto-feature-enable compat function
>>   target-i386: Disable CPUID_ACPI by default on KVM mode
>>   target-i386: Remove unsupported bits from all CPU models
>>   target-i386: Don't enable nested VMX by default
>>   target-i386: Disable SVM by default in KVM mode
>>
>>  hw/i386/pc_piix.c | 22 ++++++++++++++++++----
>>  hw/i386/pc_q35.c  | 18 ++++++++++++++++--
>>  target-i386/cpu.c | 42 ++++++++++++++++++++++++++++--------------
>>  target-i386/cpu.h |  3 ++-
>>  4 files changed, 64 insertions(+), 21 deletions(-)
>>
> 
> Andreas, are you picking up this?

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

I do still have some comments, but let's get this in first.

Regards,
Andreas

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