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

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

[Eduardo Habkost]

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 existinting 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 (3):
  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

 hw/i386/pc_piix.c |  2 ++
 hw/i386/pc_q35.c  |  2 ++
 target-i386/cpu.c | 34 +++++++++++++++++++++-------------
 3 files changed, 25 insertions(+), 13 deletions(-)

-- 
1.9.3

[Qemu-devel] [qom-cpu PATCH 1/3] 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.

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 de09ca2..8de1566 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] [qom-cpu PATCH 2/3] 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>
---
 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 8de1566..2f32d29 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -680,10 +680,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,
@@ -699,8 +700,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"
     },
@@ -711,15 +713,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] =
@@ -794,13 +797,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,
@@ -873,14 +878,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] [qom-cpu PATCH 2/3] target-i386: Remove unsupported bits from all CPU models

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On 06/18/2014 01:55 PM, Eduardo Habkost wrote:
> The following CPU features were never supported by neither TCG or KVM,
> so they are useless on the CPU model definitions, today:
> 

The overall idea of this series makes sense to me (yes, I'd love to get
libvirt to the point that we can use enforce mode), but I decline to
review the actual contents (it's a bit over my head how all the models
work) and leave it to the experts.  But here's a trivial finding:

> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8de1566..2f32d29 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -680,10 +680,11 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .family = 16,
>          .model = 2,
>          .stepping = 3,
> +        /* MIssing: CPUID_HT */

s/MIssing/Missing/


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [qom-cpu PATCH 3/3] 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 a48e263..61882d5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -267,6 +267,8 @@ static void pc_init_pci(MachineState *machine)
 static void pc_compat_2_0(MachineState *machine)
 {
     smbios_legacy_mode = true;
+    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_1_7(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b3c02c1..3949267 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -245,6 +245,8 @@ static void pc_q35_init(MachineState *machine)
 static void pc_compat_2_0(MachineState *machine)
 {
     smbios_legacy_mode = true;
+    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_1_7(MachineState *machine)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2f32d29..6bd44e1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -719,10 +719,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] =
@@ -803,9 +803,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] [qom-cpu PATCH 3/3] target-i386: Don't enable nested VMX by default

[Paolo Bonzini]

Il 18/06/2014 21:55, Eduardo Habkost ha scritto:
> 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 a48e263..61882d5 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -267,6 +267,8 @@ static void pc_init_pci(MachineState *machine)
>  static void pc_compat_2_0(MachineState *machine)
>  {
>      smbios_legacy_mode = true;
> +    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_1_7(MachineState *machine)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b3c02c1..3949267 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -245,6 +245,8 @@ static void pc_q35_init(MachineState *machine)
>  static void pc_compat_2_0(MachineState *machine)
>  {
>      smbios_legacy_mode = true;
> +    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_1_7(MachineState *machine)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 2f32d29..6bd44e1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -719,10 +719,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] =
> @@ -803,9 +803,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,
>

Could you please do the same for SVM, perhaps with the exception of 
qemu64?  Nested SVM is enabled by default upstream, but right now it is 
probably less stable than nested VMX.

Paolo