[Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

This series enables kvm_pv_unhalt by default on pc-*-2.11 and
newer.

To do that, I first reworked the existing
x86_cpu_change_kvm_default() logic to use compat_props instead,
so we don't need to make the chain of pc_compat_*() functions
grow.

Based-on: 20171006132502.9191-1-ehabkost@redhat.com
(Subject: [PATCH] isapc: Remove unnecessary migration compatibility code)

Eduardo Habkost (7):
  qemu-doc: Document minimum kernel version for KVM in x86_64
  target/i386: x86_cpu_expand_feature() helper
  target/i386: Use global variables to control KVM defaults
  kvm: Define KVM_FEAT_* even if CONFIG_KVM is not defined
  target/i386: Handle kvm_auto_* compat in x86_cpu_expand_features()
  pc: Use compat_props to control KVM defaults compatibility
  target/i386: Enable kvm_pv_unhalt by default

 include/hw/i386/pc.h   | 12 +++++++
 target/i386/cpu.h      | 22 +++++++------
 target/i386/kvm_i386.h |  9 ++++++
 hw/i386/pc_piix.c      |  7 ++--
 target/i386/cpu.c      | 88 +++++++++++++++++++++++++++++++++-----------------
 qemu-doc.texi          |  9 ++++++
 6 files changed, 105 insertions(+), 42 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/7] qemu-doc: Document minimum kernel version for KVM in x86_64

[Eduardo Habkost]

The default set of KVM CPU features require the host kernel to
support them.  KVM_PV_EOI is the newest one, and was included on
Linux v3.6 (Linux commit ae7a2a3f).

Running on an old host might break management software
expectations because the latest machine-type won't be runnable
while older machine-types might be runnable.  Document v3.6 as
the minimum kernel version for KVM on x86_64.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-doc.texi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index ecd186a159..be45b6b6f6 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -37,6 +37,7 @@
 * QEMU System emulator for non PC targets::
 * QEMU Guest Agent::
 * QEMU User space emulator::
+* System requirements::
 * Implementation notes::
 * Deprecated features::
 * License::
@@ -2348,6 +2349,14 @@ Act as if the host page size was 'pagesize' bytes
 Run the emulation in single step mode.
 @end table
 
+@node System requirements
+@chapter System requirements
+
+@section KVM kernel module
+
+On x86_64 hosts, the default set of CPU features enabled by the KVM accelerator
+require the host to be running Linux v3.6 or newer.
+
 
 @include qemu-tech.texi
 
-- 
2.13.6

Re: [Qemu-devel] [PATCH 1/7] qemu-doc: Document minimum kernel version for KVM in x86_64

[Paolo Bonzini]

On 06/10/2017 23:52, Eduardo Habkost wrote:
> The default set of KVM CPU features require the host kernel to
> support them.  KVM_PV_EOI is the newest one, and was included on
> Linux v3.6 (Linux commit ae7a2a3f).
> 
> Running on an old host might break management software
> expectations because the latest machine-type won't be runnable
> while older machine-types might be runnable.  Document v3.6 as
> the minimum kernel version for KVM on x86_64.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qemu-doc.texi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index ecd186a159..be45b6b6f6 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -37,6 +37,7 @@
>  * QEMU System emulator for non PC targets::
>  * QEMU Guest Agent::
>  * QEMU User space emulator::
> +* System requirements::
>  * Implementation notes::
>  * Deprecated features::
>  * License::
> @@ -2348,6 +2349,14 @@ Act as if the host page size was 'pagesize' bytes
>  Run the emulation in single step mode.
>  @end table
>  
> +@node System requirements
> +@chapter System requirements
> +
> +@section KVM kernel module
> +
> +On x86_64 hosts, the default set of CPU features enabled by the KVM accelerator
> +require the host to be running Linux v3.6 or newer.
> +
>  
>  @include qemu-tech.texi
>  
> 

Maybe we should expand on the consequences of running on older versions?

Paolo

Re: [Qemu-devel] [PATCH 1/7] qemu-doc: Document minimum kernel version for KVM in x86_64

[Eduardo Habkost]

On Mon, Oct 09, 2017 at 03:40:02PM +0200, Paolo Bonzini wrote:
> On 06/10/2017 23:52, Eduardo Habkost wrote:
> > The default set of KVM CPU features require the host kernel to
> > support them.  KVM_PV_EOI is the newest one, and was included on
> > Linux v3.6 (Linux commit ae7a2a3f).
> > 
> > Running on an old host might break management software
> > expectations because the latest machine-type won't be runnable
> > while older machine-types might be runnable.  Document v3.6 as
> > the minimum kernel version for KVM on x86_64.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qemu-doc.texi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/qemu-doc.texi b/qemu-doc.texi
> > index ecd186a159..be45b6b6f6 100644
> > --- a/qemu-doc.texi
> > +++ b/qemu-doc.texi
> > @@ -37,6 +37,7 @@
> >  * QEMU System emulator for non PC targets::
> >  * QEMU Guest Agent::
> >  * QEMU User space emulator::
> > +* System requirements::
> >  * Implementation notes::
> >  * Deprecated features::
> >  * License::
> > @@ -2348,6 +2349,14 @@ Act as if the host page size was 'pagesize' bytes
> >  Run the emulation in single step mode.
> >  @end table
> >  
> > +@node System requirements
> > +@chapter System requirements
> > +
> > +@section KVM kernel module
> > +
> > +On x86_64 hosts, the default set of CPU features enabled by the KVM accelerator
> > +require the host to be running Linux v3.6 or newer.
> > +
> >  
> >  @include qemu-tech.texi
> >  
> > 
> 
> Maybe we should expand on the consequences of running on older versions?

I intentionally tried to not make any commitment to what should
happen if running an older kernel.  But a quick explanation of
what can happen is probably a good idea.  I will try to come up
with something.

-- 
Eduardo

[Qemu-devel] [PATCH 2/7] target/i386: x86_cpu_expand_feature() helper

[Eduardo Habkost]

x86_cpu_expand_features() needs to ensure it won't touch
user-configured features when changing cpu->features.  Make a
helper for that, and use it when handling cpu->max_features.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 98732cd65f..90c969363e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3486,6 +3486,25 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
  *   any CPUID data based on host capabilities.
  */
 
+/**
+ * x86_cpu_expand_feature:
+ *
+ * Change cpu->features, being careful to not override features explicitly
+ * configured bv the user.
+ *
+ * @w: the feature word to be changed
+ * @mask: the bits to be changed in cpu->features[w]
+ * @value: the new value for the bits in (cpu->features[w] & @mask)
+ */
+static void x86_cpu_expand_feature(X86CPU *cpu, FeatureWord w,
+                                   uint32_t mask, uint32_t value)
+{
+    assert((value & mask) == value);
+    mask &= ~cpu->env.user_features[w];
+    cpu->env.features[w] &= ~mask;
+    cpu->env.features[w] |= value & mask;
+}
+
 /* Expand CPU configuration data, based on configured features
  * and host/accelerator capabilities when appropriate.
  */
@@ -3503,12 +3522,8 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
      */
     if (cpu->max_features) {
         for (w = 0; w < FEATURE_WORDS; w++) {
-            /* Override only features that weren't set explicitly
-             * by the user.
-             */
-            env->features[w] |=
-                x86_cpu_get_supported_feature_word(w, cpu->migratable) &
-                ~env->user_features[w];
+            uint32_t f = x86_cpu_get_supported_feature_word(w, cpu->migratable);
+            x86_cpu_expand_feature(cpu, w, ~0, f);
         }
     }
 
-- 
2.13.6

[Qemu-devel] [PATCH 3/7] target/i386: Use global variables to control KVM defaults

[Eduardo Habkost]

The compatibility rules for KVM defaults are implemented today
using x86_cpu_change_kvm_default(), and representing the
arguments to x86_cpu_change_kvm_default() inside compat_props
would be a challenge.

But the compat system doesn't need to be that complex.  From the
10 entries at kvm_default_props, only 3 of them need to be
affected by machine-type compat code.  It's much simpler and
obvious to just have 3 boolean properties that will control
compatibility mode, and handle them manually at the same place
where kvm_default_props is applied to the CPU object.

This introduces 3 global properties to control the KVM defaults
for x2apic, kvm-pv-eoi, and svm.  The global variables are just
an intermediate step to convert this to 3 X86CPU properties to be
controlled using MachineClass::compat_props.

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

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index b086b1528b..0184f98241 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1700,15 +1700,13 @@ void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
 
 
-/* Change the value of a KVM-specific default
- *
- * If value is NULL, no default will be set and the original
- * value from the CPU model table will be kept.
- *
- * It is valid to call this function only for properties that
- * are already present in the kvm_default_props table.
+/*
+ * Compat globals to control features automatically enabled/disabled by KVM.
+ * TODO: convert them to X86CPU fields set by MachineClass::compat_props
  */
-void x86_cpu_change_kvm_default(const char *prop, const char *value);
+extern bool kvm_auto_disable_svm;
+extern bool kvm_auto_enable_x2apic;
+extern bool kvm_auto_enable_pv_eoi;
 
 /* mpx_helper.c */
 void cpu_sync_bndcs_hflags(CPUX86State *env);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 31646e63c4..3e466b3896 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -328,7 +328,7 @@ static void pc_compat_2_2(MachineState *machine)
 static void pc_compat_2_1(MachineState *machine)
 {
     pc_compat_2_2(machine);
-    x86_cpu_change_kvm_default("svm", NULL);
+    kvm_auto_disable_svm = false;
 }
 
 static void pc_compat_2_0(MachineState *machine)
@@ -339,7 +339,7 @@ static void pc_compat_2_0(MachineState *machine)
 static void pc_compat_1_7(MachineState *machine)
 {
     pc_compat_2_0(machine);
-    x86_cpu_change_kvm_default("x2apic", NULL);
+    kvm_auto_enable_x2apic = false;
 }
 
 static void pc_compat_1_6(MachineState *machine)
@@ -367,7 +367,7 @@ static void pc_compat_1_3(MachineState *machine)
 static void pc_compat_1_2(MachineState *machine)
 {
     pc_compat_1_3(machine);
-    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
+    kvm_auto_enable_pv_eoi = false;
 }
 
 /* PC compat function for pc-0.10 to pc-0.13 */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 90c969363e..f8d317f1f9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1577,15 +1577,16 @@ static PropValue kvm_default_props[] = {
     { "kvm-nopiodelay", "on" },
     { "kvm-asyncpf", "on" },
     { "kvm-steal-time", "on" },
-    { "kvm-pv-eoi", "on" },
     { "kvmclock-stable-bit", "on" },
-    { "x2apic", "on" },
     { "acpi", "off" },
     { "monitor", "off" },
-    { "svm", "off" },
     { NULL, NULL },
 };
 
+bool kvm_auto_disable_svm = true;
+bool kvm_auto_enable_x2apic = true;
+bool kvm_auto_enable_pv_eoi = true;
+
 /* TCG-specific defaults that override all CPU models when using TCG
  */
 static PropValue tcg_default_props[] = {
@@ -1594,22 +1595,6 @@ static PropValue tcg_default_props[] = {
 };
 
 
-void x86_cpu_change_kvm_default(const char *prop, const char *value)
-{
-    PropValue *pv;
-    for (pv = kvm_default_props; pv->prop; pv++) {
-        if (!strcmp(pv->prop, prop)) {
-            pv->value = value;
-            break;
-        }
-    }
-
-    /* It is valid to call this function only for properties that
-     * are already present in the kvm_default_props table.
-     */
-    assert(pv->prop);
-}
-
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only);
 
@@ -2439,7 +2424,19 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     /* Special cases not set in the X86CPUDefinition structs: */
     if (kvm_enabled()) {
         if (!kvm_irqchip_in_kernel()) {
-            x86_cpu_change_kvm_default("x2apic", "off");
+            object_property_set_bool(OBJECT(cpu), false, "x2apic",
+                                     &error_abort);
+        } else if (kvm_auto_enable_x2apic) {
+            object_property_set_bool(OBJECT(cpu), true, "x2apic",
+                                     &error_abort);
+        }
+        if (kvm_auto_disable_svm) {
+            object_property_set_bool(OBJECT(cpu), false, "svm",
+                                     &error_abort);
+        }
+        if (kvm_auto_enable_pv_eoi) {
+            object_property_set_bool(OBJECT(cpu), true, "kvm-pv-eoi",
+                                     &error_abort);
         }
 
         x86_cpu_apply_props(cpu, kvm_default_props);
-- 
2.13.6

[Qemu-devel] [PATCH 4/7] kvm: Define KVM_FEAT_* even if CONFIG_KVM is not defined

[Eduardo Habkost]

This will allow us to avoid #ifdefs in code that use those macros
in target/i386/cpu.c.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/kvm_i386.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
index 1de9876cd9..8984cb02a1 100644
--- a/target/i386/kvm_i386.h
+++ b/target/i386/kvm_i386.h
@@ -36,6 +36,15 @@
  */
 #define KVM_CPUID_FEATURES       0
 
+#define KVM_FEATURE_CLOCKSOURCE  0
+#define KVM_FEATURE_NOP_IO_DELAY 0
+#define KVM_FEATURE_MMU_OP       0
+#define KVM_FEATURE_CLOCKSOURCE2 0
+#define KVM_FEATURE_ASYNC_PF     0
+#define KVM_FEATURE_STEAL_TIME   0
+#define KVM_FEATURE_PV_EOI       0
+#define KVM_FEATURE_PV_UNHALT    0
+
 #endif  /* CONFIG_KVM */
 
 bool kvm_allows_irq0_override(void);
-- 
2.13.6

[Qemu-devel] [PATCH 5/7] target/i386: Handle kvm_auto_* compat in x86_cpu_expand_features()

[Eduardo Habkost]

x86_cpu_expand_features() is run after global properties are
applied, so we will be able to convert the kvm_auto_* global
variables to QOM properties controlled by
MachineClass::compat_props.

When doing that, we need to use x86_cpu_expand_feature() so we
don't override properties set explicitly by the user.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f8d317f1f9..83e234cef4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2423,22 +2423,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
     /* Special cases not set in the X86CPUDefinition structs: */
     if (kvm_enabled()) {
-        if (!kvm_irqchip_in_kernel()) {
-            object_property_set_bool(OBJECT(cpu), false, "x2apic",
-                                     &error_abort);
-        } else if (kvm_auto_enable_x2apic) {
-            object_property_set_bool(OBJECT(cpu), true, "x2apic",
-                                     &error_abort);
-        }
-        if (kvm_auto_disable_svm) {
-            object_property_set_bool(OBJECT(cpu), false, "svm",
-                                     &error_abort);
-        }
-        if (kvm_auto_enable_pv_eoi) {
-            object_property_set_bool(OBJECT(cpu), true, "kvm-pv-eoi",
-                                     &error_abort);
-        }
-
         x86_cpu_apply_props(cpu, kvm_default_props);
     } else if (tcg_enabled()) {
         x86_cpu_apply_props(cpu, tcg_default_props);
@@ -3511,6 +3495,31 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
     FeatureWord w;
     GList *l;
     Error *local_err = NULL;
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
+
+    /*
+     * Note: "base" is a static CPU model and shouldn't auto-enable KVM
+     * defaults, so we check xcc->cpu_def here.
+     */
+    if (xcc->cpu_def && kvm_enabled()) {
+        /* KVM-specific defaults that depend on compatibility globals: */
+
+        if (!kvm_irqchip_in_kernel()) {
+            x86_cpu_expand_feature(cpu, FEAT_1_ECX, CPUID_EXT_X2APIC, 0);
+        } else if (kvm_auto_enable_x2apic) {
+            x86_cpu_expand_feature(cpu, FEAT_1_ECX, CPUID_EXT_X2APIC,
+                                   CPUID_EXT_X2APIC);
+        }
+
+        if (kvm_auto_disable_svm) {
+            x86_cpu_expand_feature(cpu, FEAT_8000_0001_ECX, CPUID_EXT3_SVM, 0);
+        }
+
+        if (kvm_auto_enable_pv_eoi) {
+            x86_cpu_expand_feature(cpu, FEAT_KVM, (1 << KVM_FEATURE_PV_EOI),
+                                   (1 << KVM_FEATURE_PV_EOI));
+        }
+    }
 
     /*TODO: Now cpu->max_features doesn't overwrite features
      * set using QOM properties, and we can convert
-- 
2.13.6

[Qemu-devel] [PATCH 6/7] pc: Use compat_props to control KVM defaults compatibility

[Eduardo Habkost]

Replace the kvm_auto_* global variables with QOM properties
controlled by MachineClass::compat_props.

This will help us eliminate the pc_compat_*() function chain in
the future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/pc.h |  8 ++++++++
 target/i386/cpu.h    | 17 +++++++++--------
 hw/i386/pc_piix.c    |  7 ++++---
 target/i386/cpu.c    | 18 ++++++++++--------
 4 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 087d184ef5..d2742dd0bc 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -753,6 +753,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver = "core2duo" "-" TYPE_X86_CPU,\
         .property = "vmx",\
         .value = "on",\
+    },{\
+        .driver = TYPE_X86_CPU,\
+        .property = "x-kvm-auto-disable-svm",\
+        .value = "off",\
     },
 
 #define PC_COMPAT_2_0 \
@@ -831,6 +835,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = "hpet",\
         .property = HPET_INTCAP,\
         .value    = stringify(4),\
+    },{\
+        .driver = TYPE_X86_CPU,\
+        .property = "x-kvm-auto-enable-x2apic",\
+        .value = "off",\
     },
 
 #define PC_COMPAT_1_6 \
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 0184f98241..7e5bf86921 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1267,6 +1267,15 @@ struct X86CPU {
     /* Stop SMI delivery for migration compatibility with old machines */
     bool kvm_no_smi_migration;
 
+    /* KVM automatically disables SVM if not explicitly enabled by user */
+    bool kvm_auto_disable_svm;
+
+    /* KVM automatically enables x2apic if not explicitly disabled by user */
+    bool kvm_auto_enable_x2apic;
+
+    /* KVM automatically enables kvm-pv-eoi if not explicitly disabled by user */
+    bool kvm_auto_enable_pv_eoi;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;
 
@@ -1700,14 +1709,6 @@ void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
 
 
-/*
- * Compat globals to control features automatically enabled/disabled by KVM.
- * TODO: convert them to X86CPU fields set by MachineClass::compat_props
- */
-extern bool kvm_auto_disable_svm;
-extern bool kvm_auto_enable_x2apic;
-extern bool kvm_auto_enable_pv_eoi;
-
 /* mpx_helper.c */
 void cpu_sync_bndcs_hflags(CPUX86State *env);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3e466b3896..e0ce633876 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -328,7 +328,6 @@ static void pc_compat_2_2(MachineState *machine)
 static void pc_compat_2_1(MachineState *machine)
 {
     pc_compat_2_2(machine);
-    kvm_auto_disable_svm = false;
 }
 
 static void pc_compat_2_0(MachineState *machine)
@@ -339,7 +338,6 @@ static void pc_compat_2_0(MachineState *machine)
 static void pc_compat_1_7(MachineState *machine)
 {
     pc_compat_2_0(machine);
-    kvm_auto_enable_x2apic = false;
 }
 
 static void pc_compat_1_6(MachineState *machine)
@@ -367,7 +365,6 @@ static void pc_compat_1_3(MachineState *machine)
 static void pc_compat_1_2(MachineState *machine)
 {
     pc_compat_1_3(machine);
-    kvm_auto_enable_pv_eoi = false;
 }
 
 /* PC compat function for pc-0.10 to pc-0.13 */
@@ -705,6 +702,10 @@ DEFINE_I440FX_MACHINE(v1_3, "pc-1.3", pc_compat_1_3,
             .driver   = "VGA",\
             .property = "mmio",\
             .value    = "off",\
+        },{\
+            .driver = TYPE_X86_CPU,\
+            .property = "x-kvm-auto-enable-pv-eoi",\
+            .value = "off",\
         },
 
 static void pc_i440fx_1_2_machine_options(MachineClass *m)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 83e234cef4..2160738a37 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1583,10 +1583,6 @@ static PropValue kvm_default_props[] = {
     { NULL, NULL },
 };
 
-bool kvm_auto_disable_svm = true;
-bool kvm_auto_enable_x2apic = true;
-bool kvm_auto_enable_pv_eoi = true;
-
 /* TCG-specific defaults that override all CPU models when using TCG
  */
 static PropValue tcg_default_props[] = {
@@ -3502,20 +3498,20 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
      * defaults, so we check xcc->cpu_def here.
      */
     if (xcc->cpu_def && kvm_enabled()) {
-        /* KVM-specific defaults that depend on compatibility globals: */
+        /* KVM-specific defaults that depend on compatibility properties: */
 
         if (!kvm_irqchip_in_kernel()) {
             x86_cpu_expand_feature(cpu, FEAT_1_ECX, CPUID_EXT_X2APIC, 0);
-        } else if (kvm_auto_enable_x2apic) {
+        } else if (cpu->kvm_auto_enable_x2apic) {
             x86_cpu_expand_feature(cpu, FEAT_1_ECX, CPUID_EXT_X2APIC,
                                    CPUID_EXT_X2APIC);
         }
 
-        if (kvm_auto_disable_svm) {
+        if (cpu->kvm_auto_disable_svm) {
             x86_cpu_expand_feature(cpu, FEAT_8000_0001_ECX, CPUID_EXT3_SVM, 0);
         }
 
-        if (kvm_auto_enable_pv_eoi) {
+        if (cpu->kvm_auto_enable_pv_eoi) {
             x86_cpu_expand_feature(cpu, FEAT_KVM, (1 << KVM_FEATURE_PV_EOI),
                                    (1 << KVM_FEATURE_PV_EOI));
         }
@@ -4162,6 +4158,12 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
     DEFINE_PROP_BOOL("kvm-no-smi-migration", X86CPU, kvm_no_smi_migration,
                      false),
+    DEFINE_PROP_BOOL("x-kvm-auto-disable-svm",
+                     X86CPU, kvm_auto_disable_svm, true),
+    DEFINE_PROP_BOOL("x-kvm-auto-enable-x2apic",
+                     X86CPU, kvm_auto_enable_x2apic, true),
+    DEFINE_PROP_BOOL("x-kvm-auto-enable-pv-eoi",
+                     X86CPU, kvm_auto_enable_pv_eoi, true),
     DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
     DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
 
-- 
2.13.6

[Qemu-devel] [PATCH 7/7] target/i386: Enable kvm_pv_unhalt by default

[Eduardo Habkost]

Original description of a patch by Alexander Graf that did
something similar:

  Commit f010bc643a (target-i386: add feature kvm_pv_unhalt) introduced the
  kvm_pv_unhalt feature but didn't enable it by default.

  Without kvm_pv_unhalt we see a measurable degradation in scheduling
  performance, so enabling it by default does make sense IMHO. This patch
  just flips it to default to on by default.

    [With kvm_pv_unhalt disabled]
    $ perf bench sched messaging -l 10000
      Total time: 8.573 [sec]

    [With kvm_pv_unhalt enabled]
    $ perf bench sched messaging -l 10000
      Total time: 4.416 [sec]

This patch does the same as that patch, but using a
kvm_auto_enable_pv_unhalt flag to keep compatibility on older
machine-types.

kvm_pv_unhalt support was added to Linux on v3.12 (Linux commit
6aef266c6e17b798a1740cf70cd34f90664740b3), so update the system
requirements section in qemu-doc.texi accordingly.

Suggested-by: Alexander Graf <agraf@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/pc.h | 4 ++++
 target/i386/cpu.h    | 3 +++
 target/i386/cpu.c    | 7 +++++++
 qemu-doc.texi        | 2 +-
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d2742dd0bc..19b42de224 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -375,6 +375,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = TYPE_X86_CPU,\
         .property = "x-hv-max-vps",\
         .value    = "0x40",\
+    },{\
+        .driver = TYPE_X86_CPU,\
+        .property = "x-kvm-auto-enable-pv-unhalt",\
+        .value = "off",\
     },
 
 #define PC_COMPAT_2_9 \
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 7e5bf86921..db1eecff00 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1276,6 +1276,9 @@ struct X86CPU {
     /* KVM automatically enables kvm-pv-eoi if not explicitly disabled by user */
     bool kvm_auto_enable_pv_eoi;
 
+    /* KVM automatically enables kvm-pv-unhalt if not explicitly disabled */
+    bool kvm_auto_enable_pv_unhalt;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2160738a37..7461c2a25e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3515,6 +3515,11 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
             x86_cpu_expand_feature(cpu, FEAT_KVM, (1 << KVM_FEATURE_PV_EOI),
                                    (1 << KVM_FEATURE_PV_EOI));
         }
+
+        if (cpu->kvm_auto_enable_pv_unhalt) {
+            x86_cpu_expand_feature(cpu, FEAT_KVM, (1 << KVM_FEATURE_PV_UNHALT),
+                                   (1 << KVM_FEATURE_PV_UNHALT));
+        }
     }
 
     /*TODO: Now cpu->max_features doesn't overwrite features
@@ -4164,6 +4169,8 @@ static Property x86_cpu_properties[] = {
                      X86CPU, kvm_auto_enable_x2apic, true),
     DEFINE_PROP_BOOL("x-kvm-auto-enable-pv-eoi",
                      X86CPU, kvm_auto_enable_pv_eoi, true),
+    DEFINE_PROP_BOOL("x-kvm-auto-enable-pv-unhalt",
+                     X86CPU, kvm_auto_enable_pv_unhalt, true),
     DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
     DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
 
diff --git a/qemu-doc.texi b/qemu-doc.texi
index be45b6b6f6..73c831383a 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2355,7 +2355,7 @@ Run the emulation in single step mode.
 @section KVM kernel module
 
 On x86_64 hosts, the default set of CPU features enabled by the KVM accelerator
-require the host to be running Linux v3.6 or newer.
+require the host to be running Linux v3.12 or newer.
 
 
 @include qemu-tech.texi
-- 
2.13.6

Re: [Qemu-devel] [PATCH 7/7] target/i386: Enable kvm_pv_unhalt by default

[Paolo Bonzini]

On 06/10/2017 23:52, Eduardo Habkost wrote:
>   Commit f010bc643a (target-i386: add feature kvm_pv_unhalt) introduced the
>   kvm_pv_unhalt feature but didn't enable it by default.
> 
>   Without kvm_pv_unhalt we see a measurable degradation in scheduling
>   performance, so enabling it by default does make sense IMHO. This patch
>   just flips it to default to on by default.
> 
>     [With kvm_pv_unhalt disabled]
>     $ perf bench sched messaging -l 10000
>       Total time: 8.573 [sec]
> 
>     [With kvm_pv_unhalt enabled]
>     $ perf bench sched messaging -l 10000
>       Total time: 4.416 [sec]

I cannot reproduce this:

Host CPU model: Haswell-EP (Xeon E5-2697 v3 @ 2.60 GHz)
Host physical CPUs: 56 (2 sockets 14 cores/sockets, 2 thread/core)
Host Linux kernel: 4.14 (more or less :))
Host memory: 64 GB
Guest Linux kernel: 4.10.13

QEMU command line:

/usr/libexec/qemu-kvm -cpu host,+kvm_pv_unhalt -M q35 \
   -m XXX -smp YYY \
   /path/to/vm.qcow2

(XXX = MiB of guest memory, YYY = number of guest processors)

"perf bench sched messaging -l 50000" has the following result for me:

Guest vCPUs    Guest memory     without PV unhalt      with PV unhalt
1*96           32 GB            24.6 s                 24.2 s
2*96           24 GB            47.9 s (both VMs)      46.8 s (both VMs)
2*48           16 GB            50.4 s (both VMs)      49.3 s (both VMs)
4*24           12 GB            82.1 - 89.0 s          82.3 - 88.8 s
4*4            12 GB            87.2 - 91.3 s          90.3 - 94.9 s

All but the first line are running the benchmark in multiple guests,
concurrently.  The improvement seems to be about 2-3% for guests larger
than 1 NUMA node, and zero or negative for smaller guests (especially as
the host is no longer overcommitted).

The difference for large NUMA guests is small but I ran the benchmark
multiple times and it is statistically significant---but not as large as
what Alex reported.

Paolo

Re: [Qemu-devel] [PATCH 7/7] target/i386: Enable kvm_pv_unhalt by default

[Alexander Graf]

On 09.10.17 16:40, Paolo Bonzini wrote:
> On 06/10/2017 23:52, Eduardo Habkost wrote:
>>   Commit f010bc643a (target-i386: add feature kvm_pv_unhalt) introduced the
>>   kvm_pv_unhalt feature but didn't enable it by default.
>>
>>   Without kvm_pv_unhalt we see a measurable degradation in scheduling
>>   performance, so enabling it by default does make sense IMHO. This patch
>>   just flips it to default to on by default.
>>
>>     [With kvm_pv_unhalt disabled]
>>     $ perf bench sched messaging -l 10000
>>       Total time: 8.573 [sec]
>>
>>     [With kvm_pv_unhalt enabled]
>>     $ perf bench sched messaging -l 10000
>>       Total time: 4.416 [sec]
> 
> I cannot reproduce this:
> 
> Host CPU model: Haswell-EP (Xeon E5-2697 v3 @ 2.60 GHz)
> Host physical CPUs: 56 (2 sockets 14 cores/sockets, 2 thread/core)
> Host Linux kernel: 4.14 (more or less :))
> Host memory: 64 GB
> Guest Linux kernel: 4.10.13
> 
> QEMU command line:
> 
> /usr/libexec/qemu-kvm -cpu host,+kvm_pv_unhalt -M q35 \

-cpu host already enables kvm_pv_unhalt. It's actually the only CPU type
that does :).

The problem arises when you use libvirt with automatic cpu type
detection, as it then generates something that doesn't do -cpu host.


Alex

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Paolo Bonzini]

On 06/10/2017 23:52, Eduardo Habkost wrote:
> This series enables kvm_pv_unhalt by default on pc-*-2.11 and
> newer.
> 
> To do that, I first reworked the existing
> x86_cpu_change_kvm_default() logic to use compat_props instead,
> so we don't need to make the chain of pc_compat_*() functions
> grow.

I've discussed PV spinlocks with some folks at Microsoft for a few weeks
now, and I'm not 100% sure about enabling kvm_pv_unhalt by default.
It's probably a good idea overall, but it does come with some caveats.

The problem is that there were two different implementations of fair
spinlocks in Linux, ticket spinlocks and queued spinlocks.  When
kvm_pv_unhalt is disabled, ticket spinlocks sucked badly indeed; queued
spinlocks however simply revert to unfair spinlocks, which loses the
fairness but has the best performance.  See virt_spin_lock in
arch/x86/include/asm/qspinlock.h.

Now, fair spinlocks are only really needed for large NUMA machines.
With a single NUMA node, for example, test-and-set spinlocks work well
enough; there's not _much_ need for fairness in practice, and the
paravirtualization does introduce some overhead.

Therefore, the best performance would be achieved with kvm_pv_unhalt
disabled on small VMs, and enabled on large VMs spanning multiple host
NUMA nodes.

Waiman, Davidlohr, do you have an opinion on this as well?

Paolo

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Waiman Long]

On 10/09/2017 09:39 AM, Paolo Bonzini wrote:
> On 06/10/2017 23:52, Eduardo Habkost wrote:
>> This series enables kvm_pv_unhalt by default on pc-*-2.11 and
>> newer.
>>
>> To do that, I first reworked the existing
>> x86_cpu_change_kvm_default() logic to use compat_props instead,
>> so we don't need to make the chain of pc_compat_*() functions
>> grow.
> I've discussed PV spinlocks with some folks at Microsoft for a few weeks
> now, and I'm not 100% sure about enabling kvm_pv_unhalt by default.
> It's probably a good idea overall, but it does come with some caveats.
>
> The problem is that there were two different implementations of fair
> spinlocks in Linux, ticket spinlocks and queued spinlocks.  When
> kvm_pv_unhalt is disabled, ticket spinlocks sucked badly indeed; queued
> spinlocks however simply revert to unfair spinlocks, which loses the
> fairness but has the best performance.  See virt_spin_lock in
> arch/x86/include/asm/qspinlock.h.
>
> Now, fair spinlocks are only really needed for large NUMA machines.
> With a single NUMA node, for example, test-and-set spinlocks work well
> enough; there's not _much_ need for fairness in practice, and the
> paravirtualization does introduce some overhead.
>
> Therefore, the best performance would be achieved with kvm_pv_unhalt
> disabled on small VMs, and enabled on large VMs spanning multiple host
> NUMA nodes.
>
> Waiman, Davidlohr, do you have an opinion on this as well?

I agree. Using unfair lock in a small VM is good for performance. The
only problem I see is how do we define what is small. Now, even a
machine with a single socket can have up to 28 cores, 56 threads. If a
VM has up to 56 vCPUs, we may still want pvqspinlock to be used.

Is the kvm_pv_unhalt flag a global one for all VMs within a machine? Or
can it be different for each VM? We may want to have this flag
dynamically determined depending on the configuration of the VM.

Regards,
Longman

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Paolo Bonzini]

On 09/10/2017 17:15, Waiman Long wrote:
> On 10/09/2017 09:39 AM, Paolo Bonzini wrote:
>> On 06/10/2017 23:52, Eduardo Habkost wrote:
>>> This series enables kvm_pv_unhalt by default on pc-*-2.11 and
>>> newer.
>>
>> I've discussed PV spinlocks with some folks at Microsoft for a few weeks
>> now, and I'm not 100% sure about enabling kvm_pv_unhalt by default.
>> It's probably a good idea overall, but it does come with some caveats.
>>
>> The problem is that there were two different implementations of fair
>> spinlocks in Linux, ticket spinlocks and queued spinlocks.  When
>> kvm_pv_unhalt is disabled, ticket spinlocks sucked badly indeed; queued
>> spinlocks however simply revert to unfair spinlocks, which loses the
>> fairness but has the best performance.  See virt_spin_lock in
>> arch/x86/include/asm/qspinlock.h.
>>
>> Now, fair spinlocks are only really needed for large NUMA machines.
>> With a single NUMA node, for example, test-and-set spinlocks work well
>> enough; there's not _much_ need for fairness in practice, and the
>> paravirtualization does introduce some overhead.
>>
>> Therefore, the best performance would be achieved with kvm_pv_unhalt
>> disabled on small VMs, and enabled on large VMs spanning multiple host
>> NUMA nodes.
>>
>> Waiman, Davidlohr, do you have an opinion on this as well?
> 
> I agree. Using unfair lock in a small VM is good for performance. The
> only problem I see is how do we define what is small. Now, even a
> machine with a single socket can have up to 28 cores, 56 threads. If a
> VM has up to 56 vCPUs, we may still want pvqspinlock to be used.

Yes.  Another possibility is to enable it when there is >1 NUMA node in
the guest.  We generally don't do this kind of magic but higher layers
(oVirt/OpenStack) do.

It also depends on how worse performance is with PV qspinlocks,
especially since now there's also vcpu_is_preempted that has to be
thrown in the mix.  A lot more testing is needed.

> Is the kvm_pv_unhalt flag a global one for all VMs within a machine? Or
> can it be different for each VM? We may want to have this flag
> dynamically determined depending on the configuration of the VM.

It's per-VM, so that's easy.

Paolo

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

(CCing libvir-list)

On Mon, Oct 09, 2017 at 05:47:44PM +0200, Paolo Bonzini wrote:
> On 09/10/2017 17:15, Waiman Long wrote:
> > On 10/09/2017 09:39 AM, Paolo Bonzini wrote:
> >> On 06/10/2017 23:52, Eduardo Habkost wrote:
> >>> This series enables kvm_pv_unhalt by default on pc-*-2.11 and
> >>> newer.
> >>
> >> I've discussed PV spinlocks with some folks at Microsoft for a few weeks
> >> now, and I'm not 100% sure about enabling kvm_pv_unhalt by default.
> >> It's probably a good idea overall, but it does come with some caveats.
> >>
> >> The problem is that there were two different implementations of fair
> >> spinlocks in Linux, ticket spinlocks and queued spinlocks.  When
> >> kvm_pv_unhalt is disabled, ticket spinlocks sucked badly indeed; queued
> >> spinlocks however simply revert to unfair spinlocks, which loses the
> >> fairness but has the best performance.  See virt_spin_lock in
> >> arch/x86/include/asm/qspinlock.h.
> >>
> >> Now, fair spinlocks are only really needed for large NUMA machines.
> >> With a single NUMA node, for example, test-and-set spinlocks work well
> >> enough; there's not _much_ need for fairness in practice, and the
> >> paravirtualization does introduce some overhead.
> >>
> >> Therefore, the best performance would be achieved with kvm_pv_unhalt
> >> disabled on small VMs, and enabled on large VMs spanning multiple host
> >> NUMA nodes.
> >>
> >> Waiman, Davidlohr, do you have an opinion on this as well?
> > 
> > I agree. Using unfair lock in a small VM is good for performance. The
> > only problem I see is how do we define what is small. Now, even a
> > machine with a single socket can have up to 28 cores, 56 threads. If a
> > VM has up to 56 vCPUs, we may still want pvqspinlock to be used.
> 
> Yes.  Another possibility is to enable it when there is >1 NUMA node in
> the guest.  We generally don't do this kind of magic but higher layers
> (oVirt/OpenStack) do.

Can't the guest make this decision, instead of the host?

> 
> It also depends on how worse performance is with PV qspinlocks,
> especially since now there's also vcpu_is_preempted that has to be
> thrown in the mix.  A lot more testing is needed.
> 

This is an interesting case for "-cpu host"/host-passthrough:
usage of "-cpu host" has an implicit assumption that no feature
will decrease performance just because we told the guest that it
is available.

Can't the guest choose the most reasonable option based on the
information we provide to it, instead of having to hide
information from the guest to get better performance?


> > Is the kvm_pv_unhalt flag a global one for all VMs within a machine? Or
> > can it be different for each VM? We may want to have this flag
> > dynamically determined depending on the configuration of the VM.
> 
> It's per-VM, so that's easy.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Waiman Long]

On 10/10/2017 11:50 AM, Eduardo Habkost wrote:
>> Yes.  Another possibility is to enable it when there is >1 NUMA node in
>> the guest.  We generally don't do this kind of magic but higher layers
>> (oVirt/OpenStack) do.
> Can't the guest make this decision, instead of the host?

By guest, do you mean the guest OS itself or the admin of the guest VM?

I am thinking about maybe adding kernel boot command line option like
"unfair_pvspinlock_cpu_threshold=4" which will instruct the OS to use
unfair spinlock if the number of CPUs is 4 or less, for example. The
default value of 0 will have the same behavior as it is today. Please
let me know what you guys think about that.

Cheers,
Longman

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

On Tue, Oct 10, 2017 at 02:07:25PM -0400, Waiman Long wrote:
> On 10/10/2017 11:50 AM, Eduardo Habkost wrote:
> >> Yes.  Another possibility is to enable it when there is >1 NUMA node in
> >> the guest.  We generally don't do this kind of magic but higher layers
> >> (oVirt/OpenStack) do.
> > Can't the guest make this decision, instead of the host?
> 
> By guest, do you mean the guest OS itself or the admin of the guest VM?

It could be either.  But even if action is required from the
guest admin to get better performance in some cases, I'd argue
that the default behavior of a Linux guest shouldn't cause a
performance regression if the host stops hiding a feature in
CPUID.

> 
> I am thinking about maybe adding kernel boot command line option like
> "unfair_pvspinlock_cpu_threshold=4" which will instruct the OS to use
> unfair spinlock if the number of CPUs is 4 or less, for example. The
> default value of 0 will have the same behavior as it is today. Please
> let me know what you guys think about that.

If that's implemented, can't Linux choose a reasonable default
for unfair_pvspinlock_cpu_threshold that won't require the admin
to manually configure it on most cases?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Waiman Long]

On 10/10/2017 03:41 PM, Eduardo Habkost wrote:
> On Tue, Oct 10, 2017 at 02:07:25PM -0400, Waiman Long wrote:
>> On 10/10/2017 11:50 AM, Eduardo Habkost wrote:
>>>> Yes.  Another possibility is to enable it when there is >1 NUMA node in
>>>> the guest.  We generally don't do this kind of magic but higher layers
>>>> (oVirt/OpenStack) do.
>>> Can't the guest make this decision, instead of the host?
>> By guest, do you mean the guest OS itself or the admin of the guest VM?
> It could be either.  But even if action is required from the
> guest admin to get better performance in some cases, I'd argue
> that the default behavior of a Linux guest shouldn't cause a
> performance regression if the host stops hiding a feature in
> CPUID.
>
>> I am thinking about maybe adding kernel boot command line option like
>> "unfair_pvspinlock_cpu_threshold=4" which will instruct the OS to use
>> unfair spinlock if the number of CPUs is 4 or less, for example. The
>> default value of 0 will have the same behavior as it is today. Please
>> let me know what you guys think about that.
> If that's implemented, can't Linux choose a reasonable default
> for unfair_pvspinlock_cpu_threshold that won't require the admin
> to manually configure it on most cases?

It is hard to have a fixed value as it depends on the CPUs being used as
well as the kind of workloads that are being run. Besides, using unfair
locks have the undesirable side effect of being subject to lock
starvation under certain circumstances. So we may not work it to be
turned on by default. Customers have to take their own risk if they want
that.

Regards,
Longman

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

On Wed, Oct 11, 2017 at 04:19:38PM -0400, Waiman Long wrote:
> On 10/10/2017 03:41 PM, Eduardo Habkost wrote:
> > On Tue, Oct 10, 2017 at 02:07:25PM -0400, Waiman Long wrote:
> >> On 10/10/2017 11:50 AM, Eduardo Habkost wrote:
> >>>> Yes.  Another possibility is to enable it when there is >1 NUMA node in
> >>>> the guest.  We generally don't do this kind of magic but higher layers
> >>>> (oVirt/OpenStack) do.
> >>> Can't the guest make this decision, instead of the host?
> >> By guest, do you mean the guest OS itself or the admin of the guest VM?
> > It could be either.  But even if action is required from the
> > guest admin to get better performance in some cases, I'd argue
> > that the default behavior of a Linux guest shouldn't cause a
> > performance regression if the host stops hiding a feature in
> > CPUID.
> >
> >> I am thinking about maybe adding kernel boot command line option like
> >> "unfair_pvspinlock_cpu_threshold=4" which will instruct the OS to use
> >> unfair spinlock if the number of CPUs is 4 or less, for example. The
> >> default value of 0 will have the same behavior as it is today. Please
> >> let me know what you guys think about that.
> > If that's implemented, can't Linux choose a reasonable default
> > for unfair_pvspinlock_cpu_threshold that won't require the admin
> > to manually configure it on most cases?
> 
> It is hard to have a fixed value as it depends on the CPUs being used as
> well as the kind of workloads that are being run. Besides, using unfair
> locks have the undesirable side effect of being subject to lock
> starvation under certain circumstances. So we may not work it to be
> turned on by default. Customers have to take their own risk if they want
> that.

Probably I am not seeing all variables involved, so pardon my
confusion.  Would unfair_pvspinlock_cpu_threshold > num_cpus just
disable usage of kvm_pv_unhalt, or make the guest choose a
completely different spinlock implementation?

Is the current default behavior of Linux guests when
kvm_pv_unhalt is unavailable a good default?  If using
kvm_pv_unhalt is not always a good idea, why do Linux guests
default to eagerly trying to use it only because the host says
it's available?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Waiman Long]

On 10/13/2017 03:01 PM, Eduardo Habkost wrote:
> On Wed, Oct 11, 2017 at 04:19:38PM -0400, Waiman Long wrote:
>> On 10/10/2017 03:41 PM, Eduardo Habkost wrote:
>>> On Tue, Oct 10, 2017 at 02:07:25PM -0400, Waiman Long wrote:
>>>> On 10/10/2017 11:50 AM, Eduardo Habkost wrote:
>>>>>> Yes.  Another possibility is to enable it when there is >1 NUMA node in
>>>>>> the guest.  We generally don't do this kind of magic but higher layers
>>>>>> (oVirt/OpenStack) do.
>>>>> Can't the guest make this decision, instead of the host?
>>>> By guest, do you mean the guest OS itself or the admin of the guest VM?
>>> It could be either.  But even if action is required from the
>>> guest admin to get better performance in some cases, I'd argue
>>> that the default behavior of a Linux guest shouldn't cause a
>>> performance regression if the host stops hiding a feature in
>>> CPUID.
>>>
>>>> I am thinking about maybe adding kernel boot command line option like
>>>> "unfair_pvspinlock_cpu_threshold=4" which will instruct the OS to use
>>>> unfair spinlock if the number of CPUs is 4 or less, for example. The
>>>> default value of 0 will have the same behavior as it is today. Please
>>>> let me know what you guys think about that.
>>> If that's implemented, can't Linux choose a reasonable default
>>> for unfair_pvspinlock_cpu_threshold that won't require the admin
>>> to manually configure it on most cases?
>> It is hard to have a fixed value as it depends on the CPUs being used as
>> well as the kind of workloads that are being run. Besides, using unfair
>> locks have the undesirable side effect of being subject to lock
>> starvation under certain circumstances. So we may not work it to be
>> turned on by default. Customers have to take their own risk if they want
>> that.
> Probably I am not seeing all variables involved, so pardon my
> confusion.  Would unfair_pvspinlock_cpu_threshold > num_cpus just
> disable usage of kvm_pv_unhalt, or make the guest choose a
> completely different spinlock implementation?

What I am proposing is that if num_cpus <=
unfair_pvspinlock_cpu_threshold, the unfair spinlock will be used even
if kvm_pv_unhalt is set.

> Is the current default behavior of Linux guests when
> kvm_pv_unhalt is unavailable a good default?  If using
> kvm_pv_unhalt is not always a good idea, why do Linux guests
> default to eagerly trying to use it only because the host says
> it's available?

For kernel with CONFIG_PARVIRT_SPINLOCKS, the current default is to use
pvqspinlock if kvm_pv_unhalt is enabled, but use unfair spinlock if it
is disabled. For kernel with just CONFIG_PARVIRT but no
CONFIG_PARAVIRT_SPINLOCKS, the unfair lock will be use no matter the
setting of kvm_pv_unhalt. Without those config options, the standard
qspinlock will be used.

Cheers,
Longman

Re: [Qemu-devel] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

On Fri, Oct 13, 2017 at 04:58:23PM -0400, Waiman Long wrote:
> On 10/13/2017 03:01 PM, Eduardo Habkost wrote:
> > On Wed, Oct 11, 2017 at 04:19:38PM -0400, Waiman Long wrote:
> >> On 10/10/2017 03:41 PM, Eduardo Habkost wrote:
> >>> On Tue, Oct 10, 2017 at 02:07:25PM -0400, Waiman Long wrote:
> >>>> On 10/10/2017 11:50 AM, Eduardo Habkost wrote:
> >>>>>> Yes.  Another possibility is to enable it when there is >1 NUMA node in
> >>>>>> the guest.  We generally don't do this kind of magic but higher layers
> >>>>>> (oVirt/OpenStack) do.
> >>>>> Can't the guest make this decision, instead of the host?
> >>>> By guest, do you mean the guest OS itself or the admin of the guest VM?
> >>> It could be either.  But even if action is required from the
> >>> guest admin to get better performance in some cases, I'd argue
> >>> that the default behavior of a Linux guest shouldn't cause a
> >>> performance regression if the host stops hiding a feature in
> >>> CPUID.
> >>>
> >>>> I am thinking about maybe adding kernel boot command line option like
> >>>> "unfair_pvspinlock_cpu_threshold=4" which will instruct the OS to use
> >>>> unfair spinlock if the number of CPUs is 4 or less, for example. The
> >>>> default value of 0 will have the same behavior as it is today. Please
> >>>> let me know what you guys think about that.
> >>> If that's implemented, can't Linux choose a reasonable default
> >>> for unfair_pvspinlock_cpu_threshold that won't require the admin
> >>> to manually configure it on most cases?
> >> It is hard to have a fixed value as it depends on the CPUs being used as
> >> well as the kind of workloads that are being run. Besides, using unfair
> >> locks have the undesirable side effect of being subject to lock
> >> starvation under certain circumstances. So we may not work it to be
> >> turned on by default. Customers have to take their own risk if they want
> >> that.
> > Probably I am not seeing all variables involved, so pardon my
> > confusion.  Would unfair_pvspinlock_cpu_threshold > num_cpus just
> > disable usage of kvm_pv_unhalt, or make the guest choose a
> > completely different spinlock implementation?
> 
> What I am proposing is that if num_cpus <=
> unfair_pvspinlock_cpu_threshold, the unfair spinlock will be used even
> if kvm_pv_unhalt is set.
> 
> > Is the current default behavior of Linux guests when
> > kvm_pv_unhalt is unavailable a good default?  If using
> > kvm_pv_unhalt is not always a good idea, why do Linux guests
> > default to eagerly trying to use it only because the host says
> > it's available?
> 
> For kernel with CONFIG_PARVIRT_SPINLOCKS, the current default is to use
> pvqspinlock if kvm_pv_unhalt is enabled, but use unfair spinlock if it
> is disabled. For kernel with just CONFIG_PARVIRT but no
> CONFIG_PARAVIRT_SPINLOCKS, the unfair lock will be use no matter the
> setting of kvm_pv_unhalt. Without those config options, the standard
> qspinlock will be used.

Thanks for the explanation.

Now, I don't know yet what's the best default for a guest that
has CONFIG_PARAVIRT_SPINLOCK when it sees a host that supports
kvm_pv_unhalt.  But I'm arguing that it's the guest
responsibility to choose what to do when it detects such a host,
instead of expecting the host to hide features from the guest.
The guest and the guest administrator have more information to
choose what's best.

In other words, if exposing kvm_pv_unhalt on CPUID really makes
some guests behave poorly, can we fix the guests instead?

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Paolo Bonzini]

On 14/10/2017 01:56, Eduardo Habkost wrote:
> Now, I don't know yet what's the best default for a guest that
> has CONFIG_PARAVIRT_SPINLOCK when it sees a host that supports
> kvm_pv_unhalt.  But I'm arguing that it's the guest
> responsibility to choose what to do when it detects such a host,
> instead of expecting the host to hide features from the guest.
> The guest and the guest administrator have more information to
> choose what's best.
> 
> In other words, if exposing kvm_pv_unhalt on CPUID really makes
> some guests behave poorly, can we fix the guests instead?

Waiman just did it. :)

https://marc.info/?l=linux-kernel&m=150972337909996

Paolo

Re: [Qemu-devel] [libvirt] [PATCH 0/7] x86: Rework KVM-defaults compat code, enable kvm_pv_unhalt by default

[Eduardo Habkost]

On Tue, Nov 07, 2017 at 12:21:16PM +0100, Paolo Bonzini wrote:
> On 14/10/2017 01:56, Eduardo Habkost wrote:
> > Now, I don't know yet what's the best default for a guest that
> > has CONFIG_PARAVIRT_SPINLOCK when it sees a host that supports
> > kvm_pv_unhalt.  But I'm arguing that it's the guest
> > responsibility to choose what to do when it detects such a host,
> > instead of expecting the host to hide features from the guest.
> > The guest and the guest administrator have more information to
> > choose what's best.
> > 
> > In other words, if exposing kvm_pv_unhalt on CPUID really makes
> > some guests behave poorly, can we fix the guests instead?
> 
> Waiman just did it. :)
> 
> https://marc.info/?l=linux-kernel&m=150972337909996

Thanks for the pointer.  I will resubmit the series for 2.12
after 2.11 is released.

-- 
Eduardo