[Qemu-devel] [PATCH 0/3] Enable x2apic by default on KVM

[Qemu-devel] [PATCH 0/3] Enable x2apic by default on KVM

[Eduardo Habkost]

This is the approach I believe was agreed upon during the last QEMU developers
conf call (sorry I didn't join, the meeting timezone change confused me).

Some may notice that representing this new behavior using solely default values
on CPU properties may be a little difficult. Making it possible to replace
x86_cpu_compat_disable_kvm_features() with compat_props entries may be an
interesting problem, too.


Eduardo Habkost (3):
  target-i386: Make kvm_default_features an array
  target-i386: Introduce x86_cpu_compat_disable_kvm_features()
  target-i386: Enable x2apic by default on KVM

 hw/i386/pc_piix.c |  7 ++++---
 hw/i386/pc_q35.c  |  2 ++
 target-i386/cpu.c | 20 +++++++++++++++-----
 target-i386/cpu.h |  4 ++--
 4 files changed, 23 insertions(+), 10 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/3] target-i386: Make kvm_default_features an array

[Eduardo Habkost]

We will later make the KVM-specific code affect other feature words,
too.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5a530b5..ee9dff1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -358,17 +358,22 @@ typedef struct model_features_t {
     FeatureWord feat_word;
 } model_features_t;
 
-static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
+/* KVM-specific features that are automatically added to all CPU models
+ * when KVM is enabled.
+ */
+static uint32_t kvm_default_features[FEATURE_WORDS] = {
+    [FEAT_KVM] = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_NOP_IO_DELAY) |
         (1 << KVM_FEATURE_CLOCKSOURCE2) |
         (1 << KVM_FEATURE_ASYNC_PF) |
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_PV_EOI) |
-        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
+        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
+};
 
 void disable_kvm_pv_eoi(void)
 {
-    kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI);
+    kvm_default_features[FEAT_KVM] &= ~(1UL << KVM_FEATURE_PV_EOI);
 }
 
 void host_cpuid(uint32_t function, uint32_t count,
@@ -1851,8 +1856,12 @@ static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
 
     /* Special cases not set in the X86CPUDefinition structs: */
     if (kvm_enabled()) {
-        env->features[FEAT_KVM] |= kvm_default_features;
+        FeatureWord w;
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            env->features[w] |= kvm_default_features[w];
+        }
     }
+
     env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
 
     /* sysenter isn't supported in compatibility mode on AMD,
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_compat_disable_kvm_features()

[Eduardo Habkost]

Instead of the feature-specific disable_kvm_pv_eoi() function, create a
more general function that can be used to disable other feature bits in
machine-type compat code.

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1acd2b2..dab0c78 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -292,7 +292,7 @@ static void pc_compat_1_3(QEMUMachineInitArgs *args)
 static void pc_compat_1_2(QEMUMachineInitArgs *args)
 {
     pc_compat_1_3(args);
-    disable_kvm_pv_eoi();
+    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
 }
 
 static void pc_init_pci_1_7(QEMUMachineInitArgs *args)
@@ -338,7 +338,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
     has_pci_info = false;
     has_acpi_build = false;
     smbios_type1_defaults = false;
-    disable_kvm_pv_eoi();
+    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
     enable_compat_apic_id_mode();
     pc_init1(args, 1, 0);
 }
@@ -351,7 +351,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
     if (!args->cpu_model) {
         args->cpu_model = "486";
     }
-    disable_kvm_pv_eoi();
+    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
     enable_compat_apic_id_mode();
     pc_init1(args, 0, 1);
 }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ee9dff1..d65ca26 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -371,9 +371,9 @@ static uint32_t kvm_default_features[FEATURE_WORDS] = {
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
 };
 
-void disable_kvm_pv_eoi(void)
+void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features)
 {
-    kvm_default_features[FEAT_KVM] &= ~(1UL << KVM_FEATURE_PV_EOI);
+    kvm_default_features[w] &= ~features;
 }
 
 void host_cpuid(uint32_t function, uint32_t count,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 9989216..d6eb97d 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1257,11 +1257,11 @@ void do_smm_enter(X86CPU *cpu);
 
 void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 
-void disable_kvm_pv_eoi(void);
-
 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);
+
 
 /* Return name of 32-bit register, from a R_* constant */
 const char *get_register_name_32(unsigned int reg);
-- 
1.8.5.3

[Qemu-devel] [PATCH 3/3] target-i386: Enable x2apic by default on KVM

[Eduardo Habkost]

When on KVM mode, enable x2apic by default on all CPU models.

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.

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

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dab0c78..cf12873 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -260,6 +260,7 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
     gigabyte_align = false;
+    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, 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 a7f6260..71b05f1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -244,6 +244,8 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
     smbios_type1_defaults = false;
     gigabyte_align = false;
+    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
+
 }
 
 static void pc_compat_1_6(QEMUMachineInitArgs *args)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d65ca26..089328e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -369,6 +369,7 @@ static uint32_t kvm_default_features[FEATURE_WORDS] = {
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_PV_EOI) |
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
+    [FEAT_1_ECX] = CPUID_EXT_X2APIC,
 };
 
 void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features)
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 0/3] Enable x2apic by default on KVM

[Paolo Bonzini]

Il 19/02/2014 15:58, Eduardo Habkost ha scritto:
> This is the approach I believe was agreed upon during the last QEMU developers
> conf call (sorry I didn't join, the meeting timezone change confused me).

Yes, that's correct.

> Some may notice that representing this new behavior using solely default values
> on CPU properties may be a little difficult. Making it possible to replace
> x86_cpu_compat_disable_kvm_features() with compat_props entries may be an
> interesting problem, too.

Very clean patches!

Paolo

Re: [Qemu-devel] [PATCH 1/3] target-i386: Make kvm_default_features an array

[Igor Mammedov]

On Wed, 19 Feb 2014 11:58:10 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> We will later make the KVM-specific code affect other feature words,
> too.
patch doesn't apply to current master, is there dependencies on list?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 5a530b5..ee9dff1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -358,17 +358,22 @@ typedef struct model_features_t {
>      FeatureWord feat_word;
>  } model_features_t;
>  
> -static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
> +/* KVM-specific features that are automatically added to all CPU models
> + * when KVM is enabled.
> + */
> +static uint32_t kvm_default_features[FEATURE_WORDS] = {
> +    [FEAT_KVM] = (1 << KVM_FEATURE_CLOCKSOURCE) |
>          (1 << KVM_FEATURE_NOP_IO_DELAY) |
>          (1 << KVM_FEATURE_CLOCKSOURCE2) |
>          (1 << KVM_FEATURE_ASYNC_PF) |
>          (1 << KVM_FEATURE_STEAL_TIME) |
>          (1 << KVM_FEATURE_PV_EOI) |
> -        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> +        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
> +};
>  
>  void disable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI);
> +    kvm_default_features[FEAT_KVM] &= ~(1UL << KVM_FEATURE_PV_EOI);
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> @@ -1851,8 +1856,12 @@ static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
>  
>      /* Special cases not set in the X86CPUDefinition structs: */
>      if (kvm_enabled()) {
> -        env->features[FEAT_KVM] |= kvm_default_features;
> +        FeatureWord w;
> +        for (w = 0; w < FEATURE_WORDS; w++) {
> +            env->features[w] |= kvm_default_features[w];
> +        }
>      }
> +
>      env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
>  
>      /* sysenter isn't supported in compatibility mode on AMD,

Re: [Qemu-devel] [PATCH 1/3] target-i386: Make kvm_default_features an array

[Eduardo Habkost]

On Wed, Feb 19, 2014 at 05:38:28PM +0100, Igor Mammedov wrote:
> On Wed, 19 Feb 2014 11:58:10 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > We will later make the KVM-specific code affect other feature words,
> > too.
> patch doesn't apply to current master, is there dependencies on list?

It is against the qom-cpu tree. I forgot to add it to Subject, sorry.

Git tree at:
https://github.com/ehabkost/qemu/tree/work/x2apic-kvm-only

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] Enable x2apic by default on KVM

[Michael S. Tsirkin]

On Wed, Feb 19, 2014 at 11:58:09AM -0300, Eduardo Habkost wrote:
> This is the approach I believe was agreed upon during the last QEMU developers
> conf call (sorry I didn't join, the meeting timezone change confused me).

Yes.

> Some may notice that representing this new behavior using solely default values
> on CPU properties may be a little difficult. Making it possible to replace
> x86_cpu_compat_disable_kvm_features() with compat_props entries may be an
> interesting problem, too.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> 
> Eduardo Habkost (3):
>   target-i386: Make kvm_default_features an array
>   target-i386: Introduce x86_cpu_compat_disable_kvm_features()
>   target-i386: Enable x2apic by default on KVM
> 
>  hw/i386/pc_piix.c |  7 ++++---
>  hw/i386/pc_q35.c  |  2 ++
>  target-i386/cpu.c | 20 +++++++++++++++-----
>  target-i386/cpu.h |  4 ++--
>  4 files changed, 23 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.5.3

Re: [Qemu-devel] [PATCH 0/3] Enable x2apic by default on KVM

[Andreas Färber]

Am 19.02.2014 20:13, schrieb Michael S. Tsirkin:
> On Wed, Feb 19, 2014 at 11:58:09AM -0300, Eduardo Habkost wrote:
>> This is the approach I believe was agreed upon during the last QEMU developers
>> conf call (sorry I didn't join, the meeting timezone change confused me).
> 
> Yes.

Matches my understanding as well.

>> Some may notice that representing this new behavior using solely default values
>> on CPU properties may be a little difficult. Making it possible to replace
>> x86_cpu_compat_disable_kvm_features() with compat_props entries may be an
>> interesting problem, too.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Paolo clarified that he is okay with me going ahead with the series, so
let's get the behavior changed first and then we can later worry about
refactoring the compat infrastructure.

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

One question for 3/3 though: Is it "running KVM", "running on KVM",
"running in KVM" or "running with KVM"? I was unsure, so left it as is.

Regards,
Andreas

>> Eduardo Habkost (3):
>>   target-i386: Make kvm_default_features an array
>>   target-i386: Introduce x86_cpu_compat_disable_kvm_features()
>>   target-i386: Enable x2apic by default on KVM

-- 
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 3/3] target-i386: Enable x2apic by default on KVM

[Andreas Färber]

Am 19.02.2014 15:58, schrieb Eduardo Habkost:
> When on KVM mode, enable x2apic by default on all CPU models.
> 
> 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.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc_piix.c | 1 +
>  hw/i386/pc_q35.c  | 2 ++
>  target-i386/cpu.c | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index dab0c78..cf12873 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -260,6 +260,7 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
>  {
>      smbios_type1_defaults = false;
>      gigabyte_align = false;
> +    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, 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 a7f6260..71b05f1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -244,6 +244,8 @@ static void pc_compat_1_7(QEMUMachineInitArgs *args)
>  {
>      smbios_type1_defaults = false;
>      gigabyte_align = false;
> +    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
> +
>  }
>  
>  static void pc_compat_1_6(QEMUMachineInitArgs *args)
[snip]

Needed rebase:

diff --cc hw/i386/pc_piix.c
index 5011c3a,ebd442e..0000000
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@@ -266,7 -266,7 +266,8 @@@ static void pc_compat_1_7(QEMUMachineIn
  {
      smbios_type1_defaults = false;
      gigabyte_align = false;
 +    option_rom_has_mr = true;
+     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
  }

  static void pc_compat_1_6(QEMUMachineInitArgs *args)
diff --cc hw/i386/pc_q35.c
index 4b0456a,71b05f1..0000000
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@@ -244,7 -244,8 +244,8 @@@ static void pc_compat_1_7(QEMUMachineIn
  {
      smbios_type1_defaults = false;
      gigabyte_align = false;
 +    option_rom_has_mr = true;
+     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
 -
  }

  static void pc_compat_1_6(QEMUMachineInitArgs *args)

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 3/3] target-i386: Enable x2apic by default on KVM

[Eduardo Habkost]

On Wed, Mar 12, 2014 at 03:36:02PM +0100, Andreas Färber wrote:
[...]
> [snip]
> 
> Needed rebase:
> 
> diff --cc hw/i386/pc_piix.c
> index 5011c3a,ebd442e..0000000
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@@ -266,7 -266,7 +266,8 @@@ static void pc_compat_1_7(QEMUMachineIn
>   {
>       smbios_type1_defaults = false;
>       gigabyte_align = false;
>  +    option_rom_has_mr = true;
> +     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
>   }
> 
>   static void pc_compat_1_6(QEMUMachineInitArgs *args)
> diff --cc hw/i386/pc_q35.c
> index 4b0456a,71b05f1..0000000
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@@ -244,7 -244,8 +244,8 @@@ static void pc_compat_1_7(QEMUMachineIn
>   {
>       smbios_type1_defaults = false;
>       gigabyte_align = false;
>  +    option_rom_has_mr = true;
> +     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
>  -
>   }
> 
>   static void pc_compat_1_6(QEMUMachineInitArgs *args)

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

(I am guessing there's no need to resubmit. That's right?)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] target-i386: Enable x2apic by default on KVM

[Andreas Färber]

Am 12.03.2014 16:42, schrieb Eduardo Habkost:
> On Wed, Mar 12, 2014 at 03:36:02PM +0100, Andreas Färber wrote:
> [...]
>> [snip]
>>
>> Needed rebase:
>>
>> diff --cc hw/i386/pc_piix.c
>> index 5011c3a,ebd442e..0000000
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@@ -266,7 -266,7 +266,8 @@@ static void pc_compat_1_7(QEMUMachineIn
>>   {
>>       smbios_type1_defaults = false;
>>       gigabyte_align = false;
>>  +    option_rom_has_mr = true;
>> +     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
>>   }
>>
>>   static void pc_compat_1_6(QEMUMachineInitArgs *args)
>> diff --cc hw/i386/pc_q35.c
>> index 4b0456a,71b05f1..0000000
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@@ -244,7 -244,8 +244,8 @@@ static void pc_compat_1_7(QEMUMachineIn
>>   {
>>       smbios_type1_defaults = false;
>>       gigabyte_align = false;
>>  +    option_rom_has_mr = true;
>> +     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
>>  -
>>   }
>>
>>   static void pc_compat_1_6(QEMUMachineInitArgs *args)
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> (I am guessing there's no need to resubmit. That's right?)

Right, just FYI that there's changes happening on my branch and that
things are still being prepared and retested for submission.

Andreas

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