[Qemu-devel] [PATCH] target-i386: Enable x2apic by default on more recent CPU models

[Qemu-devel] [PATCH] target-i386: Enable x2apic by default on more recent CPU models

[Eduardo Habkost]

This enables x2apic on the following CPU models: Conroe, Penryn,
Nehalem, Westmere, Opteron_G[12345].

Normally we try to keep the CPU model definitions as close as the real
CPUs as possible, but x2apic can be emulated by KVM without host CPU
support for x2apic, and it improves performance by reducing APIC access
overhead. x2apic emulation is available on KVM since 2009 (Linux
2.6.32-rc1), there's no reason for not enabling x2apic by default when
running KVM.

About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
on those CPU models got lots of testing in the last few years. I want to
eventually enable x2apic on all other CPU models as well, but it will
require some testing to ensure it won't confuse guests.

This shouldn't affect TCG at all because features not supported by TCG
are automatically and silently disabled by QEMU when initializing the
CPU.

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 907792b..0af00ef 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -240,6 +240,15 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
     rom_file_in_ram = false;
+    x86_cpu_compat_set_features("Conroe", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Penryn", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Nehalem", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G1", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G2", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G3", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G4", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G5", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
 }
 
 static void pc_compat_1_5(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ca84e1c..82e7a23 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -224,6 +224,15 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
     rom_file_in_ram = false;
+    x86_cpu_compat_set_features("Conroe", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Penryn", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Nehalem", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G1", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G2", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G3", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G4", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G5", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
 }
 
 static void pc_compat_1_5(QEMUMachineInitArgs *args)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9abb73f..23a44c3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -791,7 +791,7 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
         .features[FEAT_8000_0001_ECX] =
@@ -813,8 +813,8 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
-             CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
+            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
         .features[FEAT_8000_0001_ECX] =
@@ -836,8 +836,8 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
-             CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
+            CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
+            CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .features[FEAT_8000_0001_ECX] =
@@ -859,9 +859,9 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 |
-             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
-             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
+            CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_X2APIC |
+            CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
+            CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .features[FEAT_8000_0001_ECX] =
@@ -943,7 +943,7 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
              CPUID_EXT2_NX | CPUID_EXT2_PSE36 | CPUID_EXT2_PAT |
@@ -968,7 +968,7 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_CX16 | CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_CX16 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_FXSR |
              CPUID_EXT2_MMX | CPUID_EXT2_NX | CPUID_EXT2_PSE36 |
@@ -996,8 +996,8 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_POPCNT | CPUID_EXT_CX16 | CPUID_EXT_MONITOR |
-             CPUID_EXT_SSE3,
+            CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_CX16 |
+            CPUID_EXT_MONITOR | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_FXSR |
              CPUID_EXT2_MMX | CPUID_EXT2_NX | CPUID_EXT2_PSE36 |
@@ -1027,9 +1027,9 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
             CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
-             CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
-             CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ |
-             CPUID_EXT_SSE3,
+             CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
+             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
+             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP |
              CPUID_EXT2_PDPE1GB | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
@@ -1061,9 +1061,10 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
             CPUID_EXT_F16C | CPUID_EXT_AVX | CPUID_EXT_XSAVE |
-             CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 |
-             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_FMA |
-             CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
+             CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_X2APIC |
+             CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
+             CPUID_EXT_FMA | CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ |
+             CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP |
              CPUID_EXT2_PDPE1GB | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] target-i386: Enable x2apic by default on more recent CPU models

[Gleb Natapov]

On Fri, Sep 20, 2013 at 04:15:17PM -0300, Eduardo Habkost wrote:
> This enables x2apic on the following CPU models: Conroe, Penryn,
> Nehalem, Westmere, Opteron_G[12345].
> 
> Normally we try to keep the CPU model definitions as close as the real
> CPUs as possible, but x2apic can be emulated by KVM without host CPU
> support for x2apic, and it improves performance by reducing APIC access
> overhead. x2apic emulation is available on KVM since 2009 (Linux
> 2.6.32-rc1), there's no reason for not enabling x2apic by default when
> running KVM.
> 
> About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
> have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
> on those CPU models got lots of testing in the last few years. I want to
> eventually enable x2apic on all other CPU models as well, but it will
> require some testing to ensure it won't confuse guests.
> 
> This shouldn't affect TCG at all because features not supported by TCG
> are automatically and silently disabled by QEMU when initializing the
> CPU.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Gleb Natapov <gleb@redhat.com>

> ---
>  hw/i386/pc_piix.c |  9 +++++++++
>  hw/i386/pc_q35.c  |  9 +++++++++
>  target-i386/cpu.c | 37 +++++++++++++++++++------------------
>  3 files changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 907792b..0af00ef 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -240,6 +240,15 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
>      rom_file_in_ram = false;
> +    x86_cpu_compat_set_features("Conroe", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Penryn", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Nehalem", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G1", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G2", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G3", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G4", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G5", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
>  }
>  
>  static void pc_compat_1_5(QEMUMachineInitArgs *args)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index ca84e1c..82e7a23 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -224,6 +224,15 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
>      rom_file_in_ram = false;
> +    x86_cpu_compat_set_features("Conroe", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Penryn", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Nehalem", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G1", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G2", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G3", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G4", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
> +    x86_cpu_compat_set_features("Opteron_G5", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
>  }
>  
>  static void pc_compat_1_5(QEMUMachineInitArgs *args)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9abb73f..23a44c3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -791,7 +791,7 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
> +            CPUID_EXT_X2APIC | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
>          .features[FEAT_8000_0001_ECX] =
> @@ -813,8 +813,8 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
> -             CPUID_EXT_SSE3,
> +            CPUID_EXT_X2APIC | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
> +            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
>          .features[FEAT_8000_0001_ECX] =
> @@ -836,8 +836,8 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
> -             CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
> +            CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
> +            CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>          .features[FEAT_8000_0001_ECX] =
> @@ -859,9 +859,9 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 |
> -             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
> -             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
> +            CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_X2APIC |
> +            CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
> +            CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>          .features[FEAT_8000_0001_ECX] =
> @@ -943,7 +943,7 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_SSE3,
> +            CPUID_EXT_X2APIC | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
>               CPUID_EXT2_NX | CPUID_EXT2_PSE36 | CPUID_EXT2_PAT |
> @@ -968,7 +968,7 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_CX16 | CPUID_EXT_SSE3,
> +            CPUID_EXT_X2APIC | CPUID_EXT_CX16 | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_FXSR |
>               CPUID_EXT2_MMX | CPUID_EXT2_NX | CPUID_EXT2_PSE36 |
> @@ -996,8 +996,8 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_POPCNT | CPUID_EXT_CX16 | CPUID_EXT_MONITOR |
> -             CPUID_EXT_SSE3,
> +            CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_CX16 |
> +            CPUID_EXT_MONITOR | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_FXSR |
>               CPUID_EXT2_MMX | CPUID_EXT2_NX | CPUID_EXT2_PSE36 |
> @@ -1027,9 +1027,9 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
>              CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
> -             CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
> -             CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ |
> -             CPUID_EXT_SSE3,
> +             CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
> +             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
> +             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_RDTSCP |
>               CPUID_EXT2_PDPE1GB | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
> @@ -1061,9 +1061,10 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
>              CPUID_EXT_F16C | CPUID_EXT_AVX | CPUID_EXT_XSAVE |
> -             CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 |
> -             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_FMA |
> -             CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
> +             CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_X2APIC |
> +             CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
> +             CPUID_EXT_FMA | CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ |
> +             CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_RDTSCP |
>               CPUID_EXT2_PDPE1GB | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
> -- 
> 1.8.3.1

--
			Gleb.

[Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

This enables x2apic on the following CPU models: Conroe, Penryn,
Nehalem, Westmere, Opteron_G[12345].

Normally we try to keep the CPU model definitions as close as the real
CPUs as possible, but x2apic can be emulated by KVM without host CPU
support for x2apic, and it improves performance by reducing APIC access
overhead. x2apic emulation is available on KVM since 2009 (Linux
2.6.32-rc1), there's no reason for not enabling x2apic by default when
running KVM.

About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
on those CPU models got lots of testing in the last few years. I want to
eventually enable x2apic on all other CPU models as well, but it will
require some testing to ensure it won't confuse guests.

This shouldn't affect TCG at all because features not supported by TCG
are automatically and silently disabled by QEMU when initializing the
CPU.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v1 was sent in September 2013:
  Message-Id: <1379704517-19177-1-git-send-email-ehabkost@redhat.com>
  http://article.gmane.org/gmane.comp.emulators.qemu/234541

It got an Acked-by from Gleb but it was ignored by all maintainers.
---
 hw/i386/pc_piix.c |  9 +++++++++
 hw/i386/pc_q35.c  |  9 +++++++++
 target-i386/cpu.c | 37 +++++++++++++++++++------------------
 3 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2766414..9438565 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -260,6 +260,15 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
     gigabyte_align = false;
+    x86_cpu_compat_set_features("Conroe", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Penryn", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Nehalem", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G1", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G2", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G3", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G4", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G5", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 07f38ff..f6a0316 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -228,6 +228,15 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
+    x86_cpu_compat_set_features("Conroe", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Penryn", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Nehalem", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G1", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G2", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G3", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G4", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
+    x86_cpu_compat_set_features("Opteron_G5", FEAT_1_ECX, 0, CPUID_EXT_X2APIC);
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0eea8c7..8f4dcfd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -798,7 +798,7 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
         .features[FEAT_8000_0001_ECX] =
@@ -820,8 +820,8 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
-             CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
+            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
         .features[FEAT_8000_0001_ECX] =
@@ -843,8 +843,8 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
-             CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
+            CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
+            CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .features[FEAT_8000_0001_ECX] =
@@ -866,9 +866,9 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 |
-             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
-             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
+            CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_X2APIC |
+            CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
+            CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         .features[FEAT_8000_0001_ECX] =
@@ -950,7 +950,7 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
              CPUID_EXT2_NX | CPUID_EXT2_PSE36 | CPUID_EXT2_PAT |
@@ -975,7 +975,7 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_CX16 | CPUID_EXT_SSE3,
+            CPUID_EXT_X2APIC | CPUID_EXT_CX16 | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_FXSR |
              CPUID_EXT2_MMX | CPUID_EXT2_NX | CPUID_EXT2_PSE36 |
@@ -1003,8 +1003,8 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
-            CPUID_EXT_POPCNT | CPUID_EXT_CX16 | CPUID_EXT_MONITOR |
-             CPUID_EXT_SSE3,
+            CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_CX16 |
+            CPUID_EXT_MONITOR | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_FXSR |
              CPUID_EXT2_MMX | CPUID_EXT2_NX | CPUID_EXT2_PSE36 |
@@ -1034,9 +1034,9 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
             CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
-             CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
-             CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ |
-             CPUID_EXT_SSE3,
+             CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
+             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
+             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP |
              CPUID_EXT2_PDPE1GB | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
@@ -1068,9 +1068,10 @@ static x86_def_t builtin_x86_defs[] = {
              CPUID_DE | CPUID_FP87,
         .features[FEAT_1_ECX] =
             CPUID_EXT_F16C | CPUID_EXT_AVX | CPUID_EXT_XSAVE |
-             CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 |
-             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_FMA |
-             CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3,
+             CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_X2APIC |
+             CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 |
+             CPUID_EXT_FMA | CPUID_EXT_SSSE3 | CPUID_EXT_PCLMULQDQ |
+             CPUID_EXT_SSE3,
         .features[FEAT_8000_0001_EDX] =
             CPUID_EXT2_LM | CPUID_EXT2_RDTSCP |
              CPUID_EXT2_PDPE1GB | CPUID_EXT2_FXSR | CPUID_EXT2_MMX |
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 20.01.2014 15:36, schrieb Eduardo Habkost:
> This enables x2apic on the following CPU models: Conroe, Penryn,
> Nehalem, Westmere, Opteron_G[12345].
> 
> Normally we try to keep the CPU model definitions as close as the real
> CPUs as possible, but x2apic can be emulated by KVM without host CPU
> support for x2apic, and it improves performance by reducing APIC access
> overhead. x2apic emulation is available on KVM since 2009 (Linux
> 2.6.32-rc1), there's no reason for not enabling x2apic by default when
> running KVM.
> 
> About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
> have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
> on those CPU models got lots of testing in the last few years. I want to
> eventually enable x2apic on all other CPU models as well, but it will
> require some testing to ensure it won't confuse guests.
> 
> This shouldn't affect TCG at all because features not supported by TCG
> are automatically and silently disabled by QEMU when initializing the
> CPU.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v1 was sent in September 2013:
>   Message-Id: <1379704517-19177-1-git-send-email-ehabkost@redhat.com>
>   http://article.gmane.org/gmane.comp.emulators.qemu/234541
> 
> It got an Acked-by from Gleb but it was ignored by all maintainers.

Sorry, was still unread in my inbox. What changed between this version
and said v1? Just moving to _1_7? I.e. should I add back Gleb's Acked-by
before your Sob when applying?

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] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Mon, Jan 20, 2014 at 05:27:18PM +0100, Andreas Färber wrote:
> Am 20.01.2014 15:36, schrieb Eduardo Habkost:
> > This enables x2apic on the following CPU models: Conroe, Penryn,
> > Nehalem, Westmere, Opteron_G[12345].
> > 
> > Normally we try to keep the CPU model definitions as close as the real
> > CPUs as possible, but x2apic can be emulated by KVM without host CPU
> > support for x2apic, and it improves performance by reducing APIC access
> > overhead. x2apic emulation is available on KVM since 2009 (Linux
> > 2.6.32-rc1), there's no reason for not enabling x2apic by default when
> > running KVM.
> > 
> > About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
> > have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
> > on those CPU models got lots of testing in the last few years. I want to
> > eventually enable x2apic on all other CPU models as well, but it will
> > require some testing to ensure it won't confuse guests.
> > 
> > This shouldn't affect TCG at all because features not supported by TCG
> > are automatically and silently disabled by QEMU when initializing the
> > CPU.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > v1 was sent in September 2013:
> >   Message-Id: <1379704517-19177-1-git-send-email-ehabkost@redhat.com>
> >   http://article.gmane.org/gmane.comp.emulators.qemu/234541
> > 
> > It got an Acked-by from Gleb but it was ignored by all maintainers.
> 
> Sorry, was still unread in my inbox. What changed between this version
> and said v1? Just moving to _1_7? I.e. should I add back Gleb's Acked-by
> before your Sob when applying?

Yes, the only change from v1 was on the compat code. I didn't think I
should have included Gleb's Acked-by because he didn't review the compat
code changes (and I could have made mistakes when rebasing), but if you
think it is appropriate, you can pull Gleb's Acked-by.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 20.01.2014 15:36, schrieb Eduardo Habkost:
> This enables x2apic on the following CPU models: Conroe, Penryn,
> Nehalem, Westmere, Opteron_G[12345].
> 
> Normally we try to keep the CPU model definitions as close as the real
> CPUs as possible, but x2apic can be emulated by KVM without host CPU
> support for x2apic, and it improves performance by reducing APIC access
> overhead. x2apic emulation is available on KVM since 2009 (Linux
> 2.6.32-rc1), there's no reason for not enabling x2apic by default when
> running KVM.
> 
> About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
> have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
> on those CPU models got lots of testing in the last few years. I want to
> eventually enable x2apic on all other CPU models as well, but it will
> require some testing to ensure it won't confuse guests.
> 
> This shouldn't affect TCG at all because features not supported by TCG
> are automatically and silently disabled by QEMU when initializing the
> CPU.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v1 was sent in September 2013:
>   Message-Id: <1379704517-19177-1-git-send-email-ehabkost@redhat.com>
>   http://article.gmane.org/gmane.comp.emulators.qemu/234541
> 
> It got an Acked-by from Gleb but it was ignored by all maintainers.
> ---
>  hw/i386/pc_piix.c |  9 +++++++++
>  hw/i386/pc_q35.c  |  9 +++++++++
>  target-i386/cpu.c | 37 +++++++++++++++++++------------------
>  3 files changed, 37 insertions(+), 18 deletions(-)
[...]
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0eea8c7..8f4dcfd 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -798,7 +798,7 @@ static x86_def_t builtin_x86_defs[] = {
>               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
>               CPUID_DE | CPUID_FP87,
>          .features[FEAT_1_ECX] =
> -            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
> +            CPUID_EXT_X2APIC | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
>          .features[FEAT_8000_0001_EDX] =
>              CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
>          .features[FEAT_8000_0001_ECX] =
[snip]

I remember discussing about this before, but don't see an email reply,
so maybe it was on IRC?

I don't like the argument that we can put arbitrary stuff in our model
definitions and rely on TCG not having implemented it to make it
correct. Is x2apic something that TCG can never implement for some
reason? Then that needs a better explanation. Otherwise, is there no
criteria we can add this flag for when kvm_enabled()?

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] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 20.01.2014 17:50, schrieb Eduardo Habkost:
> On Mon, Jan 20, 2014 at 05:27:18PM +0100, Andreas Färber wrote:
>> Am 20.01.2014 15:36, schrieb Eduardo Habkost:
>>> v1 was sent in September 2013:
>>>   Message-Id: <1379704517-19177-1-git-send-email-ehabkost@redhat.com>
>>>   http://article.gmane.org/gmane.comp.emulators.qemu/234541
>>>
>>> It got an Acked-by from Gleb but it was ignored by all maintainers.
>>
>> Sorry, was still unread in my inbox. What changed between this version
>> and said v1? Just moving to _1_7? I.e. should I add back Gleb's Acked-by
>> before your Sob when applying?
> 
> Yes, the only change from v1 was on the compat code. I didn't think I
> should have included Gleb's Acked-by because he didn't review the compat
> code changes (and I could have made mistakes when rebasing), but if you
> think it is appropriate, you can pull Gleb's Acked-by.

I've inserted it, staging it on qom-cpu-next for now. It is a matter of
ordering the *-by before your Sob rather than after it and, if changes
were more than stylistic, adding a [you: ...] line.

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] target-i386: enable x2apic by default on more recent CPU models

[Paolo Bonzini]

Il 20/01/2014 23:13, Andreas Färber ha scritto:
> I don't like the argument that we can put arbitrary stuff in our model
> definitions and rely on TCG not having implemented it to make it
> correct. Is x2apic something that TCG can never implement for some
> reason? Then that needs a better explanation. Otherwise, is there no
> criteria we can add this flag for when kvm_enabled()?

We already do that for other bits (e.g. XSAVE/OSXSAVE), and in fact it
is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
used to trim the generic feature bits.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Mon, Jan 20, 2014 at 11:13:11PM +0100, Andreas Färber wrote:
> Am 20.01.2014 15:36, schrieb Eduardo Habkost:
> > This enables x2apic on the following CPU models: Conroe, Penryn,
> > Nehalem, Westmere, Opteron_G[12345].
> > 
> > Normally we try to keep the CPU model definitions as close as the real
> > CPUs as possible, but x2apic can be emulated by KVM without host CPU
> > support for x2apic, and it improves performance by reducing APIC access
> > overhead. x2apic emulation is available on KVM since 2009 (Linux
> > 2.6.32-rc1), there's no reason for not enabling x2apic by default when
> > running KVM.
> > 
> > About testing: Conroe, Penryn, Nehalem, Westemere and Opteron_G[123]
> > have x2apic enabled on RHEL-6 since RHEL-6.0, so the presence of x2apic
> > on those CPU models got lots of testing in the last few years. I want to
> > eventually enable x2apic on all other CPU models as well, but it will
> > require some testing to ensure it won't confuse guests.
> > 
> > This shouldn't affect TCG at all because features not supported by TCG
> > are automatically and silently disabled by QEMU when initializing the
> > CPU.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > v1 was sent in September 2013:
> >   Message-Id: <1379704517-19177-1-git-send-email-ehabkost@redhat.com>
> >   http://article.gmane.org/gmane.comp.emulators.qemu/234541
> > 
> > It got an Acked-by from Gleb but it was ignored by all maintainers.
> > ---
> >  hw/i386/pc_piix.c |  9 +++++++++
> >  hw/i386/pc_q35.c  |  9 +++++++++
> >  target-i386/cpu.c | 37 +++++++++++++++++++------------------
> >  3 files changed, 37 insertions(+), 18 deletions(-)
> [...]
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 0eea8c7..8f4dcfd 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -798,7 +798,7 @@ static x86_def_t builtin_x86_defs[] = {
> >               CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
> >               CPUID_DE | CPUID_FP87,
> >          .features[FEAT_1_ECX] =
> > -            CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
> > +            CPUID_EXT_X2APIC | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3,
> >          .features[FEAT_8000_0001_EDX] =
> >              CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL,
> >          .features[FEAT_8000_0001_ECX] =
> [snip]
> 
> I remember discussing about this before, but don't see an email reply,
> so maybe it was on IRC?
> 
> I don't like the argument that we can put arbitrary stuff in our model
> definitions and rely on TCG not having implemented it to make it
> correct.

I am not relying on it to make it "correct", I just noted that this
specific change won't affect TCG. If TCG had x2apic, I would want to
enable it by default on TCG mode as well.

> Is x2apic something that TCG can never implement for some
> reason? Then that needs a better explanation. Otherwise, is there no
> criteria we can add this flag for when kvm_enabled()?

If TCG one day implements it, it will get enabled in TCG mode, and we
will want that, won't we?

(I thought we had already settled for _not_ making the CPU models look
different on KVM and TCG mode, to keep things simple in the code, and
keep things simple in management code that needs to probe for CPU model
information.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Tue, Jan 21, 2014 at 11:03:15AM +0100, Paolo Bonzini wrote:
> Il 20/01/2014 23:13, Andreas Färber ha scritto:
> > I don't like the argument that we can put arbitrary stuff in our model
> > definitions and rely on TCG not having implemented it to make it
> > correct. Is x2apic something that TCG can never implement for some
> > reason? Then that needs a better explanation. Otherwise, is there no
> > criteria we can add this flag for when kvm_enabled()?
> 
> We already do that for other bits (e.g. XSAVE/OSXSAVE), and in fact it
> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> used to trim the generic feature bits.

The only difference is that the "check" and "enforce" flags won't work
in TCG mode. But I plan to change this soon.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 21.01.2014 11:03, schrieb Paolo Bonzini:
> Il 20/01/2014 23:13, Andreas Färber ha scritto:
>> I don't like the argument that we can put arbitrary stuff in our model
>> definitions and rely on TCG not having implemented it to make it
>> correct. Is x2apic something that TCG can never implement for some
>> reason? Then that needs a better explanation. Otherwise, is there no
>> criteria we can add this flag for when kvm_enabled()?
> 
> We already do that for other bits (e.g. XSAVE/OSXSAVE),

Please point me to the commit, a search for xsave did not come up with a
commit changing such a thing - either it did not go through my queue or
it slipped me through: Bugs are no excuse to produce more bugs.

> and in fact it
> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> used to trim the generic feature bits.

Our model definitions are the place to put stuff that real CPUs have.
Either the CPU has it or it doesn't. If it does, then this patch is
fully correct and it's TCG's job to mask things out. If we're adding
artificial flags to the generic model definitions just to make KVM
faster, then it is wrong - we have a choice of post_initialize and
realize hooks for that.

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] target-i386: enable x2apic by default on more recent CPU models

[Paolo Bonzini]

Il 21/01/2014 16:51, Andreas Färber ha scritto:
>> > We already do that for other bits (e.g. XSAVE/OSXSAVE),
> Please point me to the commit, a search for xsave did not come up with a
> commit changing such a thing - either it did not go through my queue or
> it slipped me through: Bugs are no excuse to produce more bugs.

I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't 
have XSAVE.

>> > and in fact it
>> > is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
>> > used to trim the generic feature bits.
> Our model definitions are the place to put stuff that real CPUs have.
> Either the CPU has it or it doesn't. If it does, then this patch is
> fully correct and it's TCG's job to mask things out. If we're adding
> artificial flags to the generic model definitions just to make KVM
> faster, then it is wrong - we have a choice of post_initialize and
> realize hooks for that.

It would make TCG faster as well, and there would be no reason really to 
avoid the "artificial" x2apic on TCG, if TCG implemented x2apic at all.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Tue, Jan 21, 2014 at 04:51:42PM +0100, Andreas Färber wrote:
> Am 21.01.2014 11:03, schrieb Paolo Bonzini:
> > Il 20/01/2014 23:13, Andreas Färber ha scritto:
> >> I don't like the argument that we can put arbitrary stuff in our model
> >> definitions and rely on TCG not having implemented it to make it
> >> correct. Is x2apic something that TCG can never implement for some
> >> reason? Then that needs a better explanation. Otherwise, is there no
> >> criteria we can add this flag for when kvm_enabled()?
> > 
> > We already do that for other bits (e.g. XSAVE/OSXSAVE),
> 
> Please point me to the commit, a search for xsave did not come up with a
> commit changing such a thing - either it did not go through my queue or
> it slipped me through: Bugs are no excuse to produce more bugs.
> 
> > and in fact it
> > is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> > used to trim the generic feature bits.
> 
> Our model definitions are the place to put stuff that real CPUs have.
> Either the CPU has it or it doesn't. If it does, then this patch is
> fully correct and it's TCG's job to mask things out. If we're adding
> artificial flags to the generic model definitions just to make KVM
> faster, then it is wrong - we have a choice of post_initialize and
> realize hooks for that.

I see this as a trade-off between 3 things:

1) Having the behavior of CPU models simple (having the predictable
   results not depending on things like kvm_enabled());
2) Having reasonable defaults out of the box;
3) Having the CPU models match real CPUs as closely as possible.

We can't have the three at the same time. Which item are we going to
let go?

Your suggestion seems to kill item 1 (and I thought we didn't want to).

To me 1 and 2 are more important: 1 is important to help us solve the
serious issues we have in the libvirt/QEMU interaction; 2 is important
because (I assume) we want to have good performance out of the box. I
see 3 as just a theoretical exercise that doesn't give us any benefit.


-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
> Il 21/01/2014 16:51, Andreas Färber ha scritto:
> >>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
> >Please point me to the commit, a search for xsave did not come up with a
> >commit changing such a thing - either it did not go through my queue or
> >it slipped me through: Bugs are no excuse to produce more bugs.
> 
> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
> have XSAVE.
> 
> >>> and in fact it
> >>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> >>> used to trim the generic feature bits.
> >Our model definitions are the place to put stuff that real CPUs have.
> >Either the CPU has it or it doesn't. If it does, then this patch is
> >fully correct and it's TCG's job to mask things out. If we're adding
> >artificial flags to the generic model definitions just to make KVM
> >faster, then it is wrong - we have a choice of post_initialize and
> >realize hooks for that.
> 
> It would make TCG faster as well, and there would be no reason
> really to avoid the "artificial" x2apic on TCG, if TCG implemented
> x2apic at all.

So, the discussion seem to have stalled.

Andreas, are you still against the patch, after the arguments from Paolo
and me?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 03.02.2014 20:01, schrieb Eduardo Habkost:
> On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
>> Il 21/01/2014 16:51, Andreas Färber ha scritto:
>>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
>>> Please point me to the commit, a search for xsave did not come up with a
>>> commit changing such a thing - either it did not go through my queue or
>>> it slipped me through: Bugs are no excuse to produce more bugs.
>>
>> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
>> have XSAVE.
>>
>>>>> and in fact it
>>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
>>>>> used to trim the generic feature bits.
>>> Our model definitions are the place to put stuff that real CPUs have.
>>> Either the CPU has it or it doesn't. If it does, then this patch is
>>> fully correct and it's TCG's job to mask things out. If we're adding
>>> artificial flags to the generic model definitions just to make KVM
>>> faster, then it is wrong - we have a choice of post_initialize and
>>> realize hooks for that.
>>
>> It would make TCG faster as well, and there would be no reason
>> really to avoid the "artificial" x2apic on TCG, if TCG implemented
>> x2apic at all.
> 
> So, the discussion seem to have stalled.
> 
> Andreas, are you still against the patch, after the arguments from Paolo
> and me?

Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
not there, so no solution yet.

My main concern still is that if a CPU does not have a certain feature
we should not list it as one of its features but add it to its features
where sensible. Just because TCG filters it out today is not keeping
anyone from implementing it tomorrow, in which case the emulated CPUs
would suddenly gain the feature. So my question still is, what rule can
we apply for enabling x2apic? (something like greater or equal this
family, etc. - then we can put it in your post_initialize hook so that
users can still override it)

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

[Qemu-devel] Enabling x2apic by default in KvM (was Re: [PATCH] target-i386: enable x2apic by default on more recent CPU models)

[Eduardo Habkost]

On Tue, Feb 04, 2014 at 03:12:43PM +0100, Andreas Färber wrote:
> Am 03.02.2014 20:01, schrieb Eduardo Habkost:
> > On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
> >> Il 21/01/2014 16:51, Andreas Färber ha scritto:
> >>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
> >>> Please point me to the commit, a search for xsave did not come up with a
> >>> commit changing such a thing - either it did not go through my queue or
> >>> it slipped me through: Bugs are no excuse to produce more bugs.
> >>
> >> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
> >> have XSAVE.
> >>
> >>>>> and in fact it
> >>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> >>>>> used to trim the generic feature bits.
> >>> Our model definitions are the place to put stuff that real CPUs have.
> >>> Either the CPU has it or it doesn't. If it does, then this patch is
> >>> fully correct and it's TCG's job to mask things out. If we're adding
> >>> artificial flags to the generic model definitions just to make KVM
> >>> faster, then it is wrong - we have a choice of post_initialize and
> >>> realize hooks for that.
> >>
> >> It would make TCG faster as well, and there would be no reason
> >> really to avoid the "artificial" x2apic on TCG, if TCG implemented
> >> x2apic at all.
> > 
> > So, the discussion seem to have stalled.
> > 
> > Andreas, are you still against the patch, after the arguments from Paolo
> > and me?
> 
> Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
> not there, so no solution yet.
> 
> My main concern still is that if a CPU does not have a certain feature
> we should not list it as one of its features but add it to its features
> where sensible. Just because TCG filters it out today is not keeping
> anyone from implementing it tomorrow, in which case the emulated CPUs
> would suddenly gain the feature. So my question still is, what rule can
> we apply for enabling x2apic? (something like greater or equal this
> family, etc. - then we can put it in your post_initialize hook so that
> users can still override it)

The rule needs be simple and predictable, so libvirt can know in advance
if the results are going to match what was requested by the user. That's
the problem with enabling it automatically based on some rule more
complex than the existing "it's enabled if it's on the CPU model table
or on the command-line" rule.

Considering the whole stack, there's a strong reason to enable x2apic by
default on all cases when using KVM. But if keeping the CPU model table
match real CPUs bit-by-bit is more important for QEMU (which I don't
understand why), I believe we will need to ask libvirt to do it (enable
x2apic explicitly in the command-line by default). Or we could make the
CPU models look different in KVM and TCG mode, but I am trying to avoid
that.

But I still believe that enabling x2apic by default in TCG _and_ KVM
mode is easier and harmless. I don't see the value in this academic
exercise of trying to make the CPU model table match real CPUs
bit-by-bit.

I mean: I simply don't expect any user will come to us complaining
because they didn't want x2apic to be enabled by default. And if some
user really cares about that, they can simply use "-x2apic" in the
command-line. In other words, if it is not going to affect users
negatively, why are we spending energy on it?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Michael S. Tsirkin]

On Tue, Feb 04, 2014 at 03:12:43PM +0100, Andreas Färber wrote:
> Am 03.02.2014 20:01, schrieb Eduardo Habkost:
> > On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
> >> Il 21/01/2014 16:51, Andreas Färber ha scritto:
> >>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
> >>> Please point me to the commit, a search for xsave did not come up with a
> >>> commit changing such a thing - either it did not go through my queue or
> >>> it slipped me through: Bugs are no excuse to produce more bugs.
> >>
> >> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
> >> have XSAVE.
> >>
> >>>>> and in fact it
> >>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> >>>>> used to trim the generic feature bits.
> >>> Our model definitions are the place to put stuff that real CPUs have.
> >>> Either the CPU has it or it doesn't. If it does, then this patch is
> >>> fully correct and it's TCG's job to mask things out. If we're adding
> >>> artificial flags to the generic model definitions just to make KVM
> >>> faster, then it is wrong - we have a choice of post_initialize and
> >>> realize hooks for that.
> >>
> >> It would make TCG faster as well, and there would be no reason
> >> really to avoid the "artificial" x2apic on TCG, if TCG implemented
> >> x2apic at all.
> > 
> > So, the discussion seem to have stalled.
> > 
> > Andreas, are you still against the patch, after the arguments from Paolo
> > and me?
> 
> Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
> not there, so no solution yet.

We have the weekly call tomorrow.
Let's discuss there?

> My main concern still is that if a CPU does not have a certain feature
> we should not list it as one of its features but add it to its features
> where sensible. Just because TCG filters it out today is not keeping
> anyone from implementing it tomorrow, in which case the emulated CPUs
> would suddenly gain the feature.

Why is this a problem?
We will just have to make sure features stay consistent
for old -M machine types.

> So my question still is, what rule can
> we apply for enabling x2apic? (something like greater or equal this
> family, etc. - then we can put it in your post_initialize hook so that
> users can still override it)
> 
> 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] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Mon, Feb 17, 2014 at 03:58:13PM +0200, Michael S. Tsirkin wrote:
> On Tue, Feb 04, 2014 at 03:12:43PM +0100, Andreas Färber wrote:
> > Am 03.02.2014 20:01, schrieb Eduardo Habkost:
> > > On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
> > >> Il 21/01/2014 16:51, Andreas Färber ha scritto:
> > >>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
> > >>> Please point me to the commit, a search for xsave did not come up with a
> > >>> commit changing such a thing - either it did not go through my queue or
> > >>> it slipped me through: Bugs are no excuse to produce more bugs.
> > >>
> > >> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
> > >> have XSAVE.
> > >>
> > >>>>> and in fact it
> > >>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> > >>>>> used to trim the generic feature bits.
> > >>> Our model definitions are the place to put stuff that real CPUs have.
> > >>> Either the CPU has it or it doesn't. If it does, then this patch is
> > >>> fully correct and it's TCG's job to mask things out. If we're adding
> > >>> artificial flags to the generic model definitions just to make KVM
> > >>> faster, then it is wrong - we have a choice of post_initialize and
> > >>> realize hooks for that.
> > >>
> > >> It would make TCG faster as well, and there would be no reason
> > >> really to avoid the "artificial" x2apic on TCG, if TCG implemented
> > >> x2apic at all.
> > > 
> > > So, the discussion seem to have stalled.
> > > 
> > > Andreas, are you still against the patch, after the arguments from Paolo
> > > and me?
> > 
> > Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
> > not there, so no solution yet.
> 
> We have the weekly call tomorrow.
> Let's discuss there?

Sounds good to me.

> 
> > My main concern still is that if a CPU does not have a certain feature
> > we should not list it as one of its features but add it to its features
> > where sensible. Just because TCG filters it out today is not keeping
> > anyone from implementing it tomorrow, in which case the emulated CPUs
> > would suddenly gain the feature.
> 
> Why is this a problem?
> We will just have to make sure features stay consistent
> for old -M machine types.

To be exact, we just have to make sure features stay consistent if
"enforce" is used, and "enforce" doesn't even work with TCG today. And
once we make "enforce" work with TCG, all the CPUs affected by this
patch won't even run with TCG+enforce.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 17.02.2014 14:58, schrieb Michael S. Tsirkin:
> On Tue, Feb 04, 2014 at 03:12:43PM +0100, Andreas Färber wrote:
>> Am 03.02.2014 20:01, schrieb Eduardo Habkost:
>>> On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
>>>> Il 21/01/2014 16:51, Andreas Färber ha scritto:
>>>>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
>>>>> Please point me to the commit, a search for xsave did not come up with a
>>>>> commit changing such a thing - either it did not go through my queue or
>>>>> it slipped me through: Bugs are no excuse to produce more bugs.
>>>>
>>>> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
>>>> have XSAVE.
>>>>
>>>>>>> and in fact it
>>>>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
>>>>>>> used to trim the generic feature bits.
>>>>> Our model definitions are the place to put stuff that real CPUs have.
>>>>> Either the CPU has it or it doesn't. If it does, then this patch is
>>>>> fully correct and it's TCG's job to mask things out. If we're adding
>>>>> artificial flags to the generic model definitions just to make KVM
>>>>> faster, then it is wrong - we have a choice of post_initialize and
>>>>> realize hooks for that.
>>>>
>>>> It would make TCG faster as well, and there would be no reason
>>>> really to avoid the "artificial" x2apic on TCG, if TCG implemented
>>>> x2apic at all.
>>>
>>> So, the discussion seem to have stalled.
>>>
>>> Andreas, are you still against the patch, after the arguments from Paolo
>>> and me?
>>
>> Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
>> not there, so no solution yet.
> 
> We have the weekly call tomorrow.
> Let's discuss there?
> 
>> My main concern still is that if a CPU does not have a certain feature
>> we should not list it as one of its features but add it to its features
>> where sensible. Just because TCG filters it out today is not keeping
>> anyone from implementing it tomorrow, in which case the emulated CPUs
>> would suddenly gain the feature.
> 
> Why is this a problem?
> We will just have to make sure features stay consistent
> for old -M machine types.

I thought I made my point pretty clear: I will not accept a patch that

a) adds a feature flag that has no relation to the model it is named after

Instead of adding the flag to 9 models, define a rule of when to add it.
=> Model semantics remain clear.
=> Avoids someone forgetting the flag for new qualifying models.
=> 3 lines of code, including braces.

b) adds the flag to "more recent models"

Please define this in technical terms, so that we can implement a rule.
Patch is waiting in qom-cpu-next for that answer.
=> Avoids forgetting to add to some new "more recent" model.
Suppose I want to add a model for, e.g., the Intel Quark X1000 (new
product, but stripped of features), should it get the flag or not?!

c) relies on TCG filtering out a feature being added

Someone implementing it for TCG in 2.x needs to know for which models to
filter it out via x86_cpu_compat_set_features() or future compat_props.
Previously I was under the assumption they would need to be filtered out
based on whether the hardware actually has it, but in fact it would need
to be filtered out for all affected models. How?
=> kvm_enabled() for speed optimizations, tcg_enabled() for emulation.
If desired for TCG, it can be added via -cpu ...,+x2apic syntax. Why
would a user instead need to know she needs to use Westmere,-x2apic in
order to get a Westmere CPU. That's counter-intuitive.

Libvirt launching an x86 TCG guest will be rather rare BTW for
performance reasons waaay beyond x2apic, and it would be for the libvirt
folks to answer whether their TCG machines are supposed to get
performance-optimized or not (I doubt so, but anyway libvirt != QEMU; a
consequence would otherwise be enabling SIMD such as SSE42 for old x86
models, too, and NEON and VFPv4 for old ARM models, etc. so that we
would in practice only need one "any" model as for linux-user).

For accel=kvm we already do all sorts of tweaks, including vendor
override; no disagreement on the overall goal there. I just don't want
KVM people to ignorantly trample our emulation command line (ehabkost:
"academic"; I say consistency, usability and maintainability) for a
misguided convenience, when my suggestion probably even requires less
lines of code than the original patch, not to mention all the discussion
that followed and is taking away time for other patches.

Answers or v2 patches could've easily been provided via list already.
Without input, I see nothing to discuss really, Eduardo doesn't seem to
understand or care. I would be available tomorrow, but I will rather
step down as x86 CPU maintainer than sign off a hostile takeover by KVM
or libvirt! CC'ing PMM and rth to counter the KVM/libvirt bias in CC.

A compromise might be some global command line flag to enforce
hardware-close emulation or to tweak for performance. But that still
requires a) and b) to be addressed. And it would likely be redundant
with accel=.

Regards,
Andreas

>> So my question still is, what rule can
>> we apply for enabling x2apic? (something like greater or equal this
>> family, etc. - then we can put it in your post_initialize hook so that
>> users can still override it)
>>
>> 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] target-i386: enable x2apic by default on more recent CPU models

[Eduardo Habkost]

On Mon, Feb 17, 2014 at 05:17:51PM +0100, Andreas Färber wrote:
> Am 17.02.2014 14:58, schrieb Michael S. Tsirkin:
> > On Tue, Feb 04, 2014 at 03:12:43PM +0100, Andreas Färber wrote:
> >> Am 03.02.2014 20:01, schrieb Eduardo Habkost:
> >>> On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
> >>>> Il 21/01/2014 16:51, Andreas Färber ha scritto:
> >>>>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
> >>>>> Please point me to the commit, a search for xsave did not come up with a
> >>>>> commit changing such a thing - either it did not go through my queue or
> >>>>> it slipped me through: Bugs are no excuse to produce more bugs.
> >>>>
> >>>> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
> >>>> have XSAVE.
> >>>>
> >>>>>>> and in fact it
> >>>>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
> >>>>>>> used to trim the generic feature bits.
> >>>>> Our model definitions are the place to put stuff that real CPUs have.
> >>>>> Either the CPU has it or it doesn't. If it does, then this patch is
> >>>>> fully correct and it's TCG's job to mask things out. If we're adding
> >>>>> artificial flags to the generic model definitions just to make KVM
> >>>>> faster, then it is wrong - we have a choice of post_initialize and
> >>>>> realize hooks for that.
> >>>>
> >>>> It would make TCG faster as well, and there would be no reason
> >>>> really to avoid the "artificial" x2apic on TCG, if TCG implemented
> >>>> x2apic at all.
> >>>
> >>> So, the discussion seem to have stalled.
> >>>
> >>> Andreas, are you still against the patch, after the arguments from Paolo
> >>> and me?
> >>
> >> Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
> >> not there, so no solution yet.
> > 
> > We have the weekly call tomorrow.
> > Let's discuss there?
> > 
> >> My main concern still is that if a CPU does not have a certain feature
> >> we should not list it as one of its features but add it to its features
> >> where sensible. Just because TCG filters it out today is not keeping
> >> anyone from implementing it tomorrow, in which case the emulated CPUs
> >> would suddenly gain the feature.
> > 
> > Why is this a problem?
> > We will just have to make sure features stay consistent
> > for old -M machine types.
> 
> I thought I made my point pretty clear: I will not accept a patch that
> 
> a) adds a feature flag that has no relation to the model it is named after
> 
> Instead of adding the flag to 9 models, define a rule of when to add it.
> => Model semantics remain clear.
> => Avoids someone forgetting the flag for new qualifying models.
> => 3 lines of code, including braces.

I am not worried about code size, I worry about complexity added when
probing for CPU model information from the outside. Right now we have a
single namespace for CPU models, where the results all look the same on
TCG and KVM mode (except for the CPU vendor, and I wouldn't like to make
the list of exceptions grow).

If you propose changing it and having different results in KVM and TCG
mode, then let's discuss how to implement that. If we are going to do
that, I would like to have different CPU model names for KVM and TCG CPU
models, so management code won't get confused. Or maybe we could flag
some KVM-specific CPU models as KVM-only. Or we could look for other
solutions.

> 
> b) adds the flag to "more recent models"
> 
> Please define this in technical terms, so that we can implement a rule.
> Patch is waiting in qom-cpu-next for that answer.
> => Avoids forgetting to add to some new "more recent" model.
> Suppose I want to add a model for, e.g., the Intel Quark X1000 (new
> product, but stripped of features), should it get the flag or not?!

What about "CPU models that can't run in TCG mode and are only useful
for KVM"? The Opteron_G* models can't run in TCG mode today, and were
added only because of KVM. I am not sure about Penryn, Conroe, Nehalem,
Westmere, but I won't be surprised if they can't run in TCG mode as
well.

All I want is to have CPU models that are useful. You, too, want CPU
models that are useful, but for different purposes. We have two
conflicting goals, so let's find solutions that will help us reach those
goals.


> 
> c) relies on TCG filtering out a feature being added
> 
> Someone implementing it for TCG in 2.x needs to know for which models to
> filter it out via x86_cpu_compat_set_features() or future compat_props.
> Previously I was under the assumption they would need to be filtered out
> based on whether the hardware actually has it, but in fact it would need
> to be filtered out for all affected models. How?
> => kvm_enabled() for speed optimizations, tcg_enabled() for emulation.
> If desired for TCG, it can be added via -cpu ...,+x2apic syntax. Why
> would a user instead need to know she needs to use Westmere,-x2apic in
> order to get a Westmere CPU. That's counter-intuitive.

Having "-cpu Westmere" mean different things in KVM and TCG mode is,
too, counter-intuitive. I am trying to find a trade-off here.


> 
> Libvirt launching an x86 TCG guest will be rather rare BTW for
> performance reasons waaay beyond x2apic, and it would be for the libvirt
> folks to answer whether their TCG machines are supposed to get
> performance-optimized or not (I doubt so, but anyway libvirt != QEMU; a
> consequence would otherwise be enabling SIMD such as SSE42 for old x86
> models, too, and NEON and VFPv4 for old ARM models, etc. so that we
> would in practice only need one "any" model as for linux-user).

The point was not optimizing TCG, but avoiding adding complexity by
making the CPU models look different in KVM and TCG mode.


> 
> For accel=kvm we already do all sorts of tweaks, including vendor
> override; no disagreement on the overall goal there. I just don't want
> KVM people to ignorantly trample our emulation command line (ehabkost:
> "academic"; I say consistency, usability and maintainability) for a
> misguided convenience, when my suggestion probably even requires less
> lines of code than the original patch, not to mention all the discussion
> that followed and is taking away time for other patches.

We fortunately don't do "all sorts of tweaks", except for the vendor
override, which also adds the kind of complexity I am trying to avoid.


> 
> Answers or v2 patches could've easily been provided via list already.
> Without input, I see nothing to discuss really, Eduardo doesn't seem to
> understand or care. I would be available tomorrow, but I will rather
> step down as x86 CPU maintainer than sign off a hostile takeover by KVM
> or libvirt! CC'ing PMM and rth to counter the KVM/libvirt bias in CC.

I was still waiting for an answer for my questions/assumptions at:
http://marc.info/?l=qemu-devel&m=139176948602576

I didn't send v2 because I still don't see my main point being answered:
I believed we wanted to avoid having separate CPU model namespaces for
TCG and KVM (in other words, having different results depending on
accel)[1]. Has this changed?

I am trying to find and evaluate alternatives, but it is hard to do that
when I don't get my questions answered. I don't understand why you call
this a "hostile takeover".


[1] http://marc.info/?l=qemu-devel&m=136983789405010&w=2

> 
> A compromise might be some global command line flag to enforce
> hardware-close emulation or to tweak for performance. But that still
> requires a) and b) to be addressed. And it would likely be redundant
> with accel=.

If we change the resulting CPU depending on a new command-line option or
accel=, we are in practice having two CPU model namespaces, depending on
that option (see [1] above).

Let's discuss approaches to implement that tomorrow, then? Because I had
the feeling we had discarded that option in the past.

> 
> Regards,
> Andreas
> 
> >> So my question still is, what rule can
> >> we apply for enabling x2apic? (something like greater or equal this
> >> family, etc. - then we can put it in your post_initialize hook so that
> >> users can still override it)
> >>
> >> 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

-- 
Eduardo