[PATCH v4 0/4] Add a unified parameter "nopvspin"

[PATCH v4 0/4] Add a unified parameter "nopvspin"

[Zhenzhong Duan]

There are cases folks want to disable spinlock optimization for
debug/test purpose. Xen and hyperv already have parameters "xen_nopvspin"
and "hv_nopvspin" to support that, but kvm doesn't.

The first patch adds that feature to KVM guest with "nopvspin".

For compatibility reason original parameters "xen_nopvspin" and
"hv_nopvspin" are retained and marked obsolete.


v4:
PATCH1: use variable name nopvspin instead of pvspin and
        defined it as __initdata, changed print message,
        updated patch description                     [Sean Christopherson]
PATCH2: remove Suggested-by, use "kvm-guest:" prefix  [Sean Christopherson]
PATCH3: make variable nopvsin and xen_pvspin coexist
        remove Reviewed-by due to code change         [Sean Christopherson]
PATCH4: make variable nopvsin and hv_pvspin coexist   [Sean Christopherson]

v3:
PATCH2: Fix indentation

v2:
PATCH1: pick the print code change into separate PATCH2,
        updated patch description             [Vitaly Kuznetsov]
PATCH2: new patch with print code change      [Vitaly Kuznetsov]
PATCH3: add Reviewed-by                       [Juergen Gross]

Zhenzhong Duan (4):
  x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
  x86/kvm: Change print code to use pr_*() format
  xen: Mark "xen_nopvspin" parameter obsolete
  x86/hyperv: Mark "hv_nopvspin" parameter obsolete

 Documentation/admin-guide/kernel-parameters.txt | 14 ++++++-
 arch/x86/hyperv/hv_spinlock.c                   |  4 ++
 arch/x86/include/asm/qspinlock.h                |  1 +
 arch/x86/kernel/kvm.c                           | 51 +++++++++++++++----------
 arch/x86/xen/spinlock.c                         |  3 ++
 kernel/locking/qspinlock.c                      |  7 ++++
 6 files changed, 57 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

There are cases where a guest tries to switch spinlocks to bare metal
behavior (e.g. by setting "xen_nopvspin" on XEN platform and
"hv_nopvspin" on HYPER_V).

That feature is missed on KVM, add a new parameter "nopvspin" to disable
PV spinlocks for KVM guest.

The new 'nopvspin' parameter will also replace Xen and Hyper-V specific
parameters in future patches.

Define variable nopvsin as global because it will be used in future patches
as above.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krcmar <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  5 +++++
 arch/x86/include/asm/qspinlock.h                |  1 +
 arch/x86/kernel/kvm.c                           | 21 ++++++++++++++-------
 kernel/locking/qspinlock.c                      |  7 +++++++
 4 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c7ac2f3..89d77ea 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5330,6 +5330,11 @@
 			as generic guest with no PV drivers. Currently support
 			XEN HVM, KVM, HYPER_V and VMWARE guest.
 
+	nopvspin	[X86,KVM]
+			Disables the qspinlock slow path using PV optimizations
+			which allow the hypervisor to 'idle' the guest on lock
+			contention.
+
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
 			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 444d6fd..d86ab94 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -32,6 +32,7 @@ static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lo
 extern void __pv_init_lock_hash(void);
 extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
 extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock);
+extern bool nopvspin;
 
 #define	queued_spin_unlock queued_spin_unlock
 /**
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index e820568..481d879 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -831,16 +831,23 @@ __visible bool __kvm_vcpu_is_preempted(long cpu)
  */
 void __init kvm_spinlock_init(void)
 {
-	/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
-	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
-		return;
-
-	if (kvm_para_has_hint(KVM_HINTS_REALTIME))
+	/*
+	 * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT feature
+	 * support, or there is REALTIME hints or only 1 vCPU.
+	 */
+	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
+	    kvm_para_has_hint(KVM_HINTS_REALTIME) ||
+	    num_possible_cpus() == 1) {
+		pr_info("PV spinlocks disabled\n");
 		return;
+	}
 
-	/* Don't use the pvqspinlock code if there is only 1 vCPU. */
-	if (num_possible_cpus() == 1)
+	if (nopvspin) {
+		pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");
+		static_branch_disable(&virt_spin_lock_key);
 		return;
+	}
+	pr_info("PV spinlocks enabled\n");
 
 	__pv_init_lock_hash();
 	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 2473f10..75193d6 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
 #include "qspinlock_paravirt.h"
 #include "qspinlock.c"
 
+bool nopvspin __initdata;
+static __init int parse_nopvspin(char *arg)
+{
+	nopvspin = true;
+	return 0;
+}
+early_param("nopvspin", parse_nopvspin);
 #endif
-- 
1.8.3.1

[PATCH v4 2/4] x86/kvm: Change print code to use pr_*() format

[Zhenzhong Duan]

pr_*() is preferred than printk(KERN_* ...), after change all the print
in arch/x86/kernel/kvm.c will have "kvm_guest: xxx" style.

No functional change.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krcmar <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/kernel/kvm.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 481d879..a4bfe67 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -7,6 +7,8 @@
  *   Authors: Anthony Liguori <aliguori@us.ibm.com>
  */
 
+#define pr_fmt(fmt) "kvm_guest: " fmt
+
 #include <linux/context_tracking.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -286,8 +288,8 @@ static void kvm_register_steal_time(void)
 		return;
 
 	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
-	pr_info("kvm-stealtime: cpu %d, msr %llx\n",
-		cpu, (unsigned long long) slow_virt_to_phys(st));
+	pr_info("stealtime: cpu %d, msr %llx\n", cpu,
+		(unsigned long long) slow_virt_to_phys(st));
 }
 
 static DEFINE_PER_CPU_DECRYPTED(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED;
@@ -321,8 +323,7 @@ static void kvm_guest_cpu_init(void)
 
 		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
 		__this_cpu_write(apf_reason.enabled, 1);
-		printk(KERN_INFO"KVM setup async PF for cpu %d\n",
-		       smp_processor_id());
+		pr_info("setup async PF for cpu %d\n", smp_processor_id());
 	}
 
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) {
@@ -347,8 +348,7 @@ static void kvm_pv_disable_apf(void)
 	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
 	__this_cpu_write(apf_reason.enabled, 0);
 
-	printk(KERN_INFO"Unregister pv shared memory for cpu %d\n",
-	       smp_processor_id());
+	pr_info("Unregister pv shared memory for cpu %d\n", smp_processor_id());
 }
 
 static void kvm_pv_guest_cpu_reboot(void *unused)
@@ -469,7 +469,8 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
 		} else {
 			ret = kvm_hypercall4(KVM_HC_SEND_IPI, (unsigned long)ipi_bitmap,
 				(unsigned long)(ipi_bitmap >> BITS_PER_LONG), min, icr);
-			WARN_ONCE(ret < 0, "KVM: failed to send PV IPI: %ld", ret);
+			WARN_ONCE(ret < 0, "kvm_guest: failed to send PV IPI: %ld",
+				  ret);
 			min = max = apic_id;
 			ipi_bitmap = 0;
 		}
@@ -479,7 +480,8 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
 	if (ipi_bitmap) {
 		ret = kvm_hypercall4(KVM_HC_SEND_IPI, (unsigned long)ipi_bitmap,
 			(unsigned long)(ipi_bitmap >> BITS_PER_LONG), min, icr);
-		WARN_ONCE(ret < 0, "KVM: failed to send PV IPI: %ld", ret);
+		WARN_ONCE(ret < 0, "kvm_guest: failed to send PV IPI: %ld",
+			  ret);
 	}
 
 	local_irq_restore(flags);
@@ -509,7 +511,7 @@ static void kvm_setup_pv_ipi(void)
 {
 	apic->send_IPI_mask = kvm_send_ipi_mask;
 	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
-	pr_info("KVM setup pv IPIs\n");
+	pr_info("setup pv IPIs\n");
 }
 
 static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
@@ -639,11 +641,11 @@ static void __init kvm_guest_init(void)
 	    !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
 	    kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
 		smp_ops.send_call_func_ipi = kvm_smp_send_call_func_ipi;
-		pr_info("KVM setup pv sched yield\n");
+		pr_info("setup pv sched yield\n");
 	}
 	if (cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/kvm:online",
 				      kvm_cpu_online, kvm_cpu_down_prepare) < 0)
-		pr_err("kvm_guest: Failed to install cpu hotplug callbacks\n");
+		pr_err("failed to install cpu hotplug callbacks\n");
 #else
 	sev_map_percpu_data();
 	kvm_guest_cpu_init();
@@ -746,7 +748,7 @@ static __init int kvm_setup_pv_tlb_flush(void)
 			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_tlb_mask, cpu),
 				GFP_KERNEL, cpu_to_node(cpu));
 		}
-		pr_info("KVM setup pv remote TLB flush\n");
+		pr_info("setup pv remote TLB flush\n");
 	}
 
 	return 0;
@@ -879,8 +881,8 @@ static void kvm_enable_host_haltpoll(void *i)
 void arch_haltpoll_enable(unsigned int cpu)
 {
 	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
-		pr_err_once("kvm: host does not support poll control\n");
-		pr_err_once("kvm: host upgrade recommended\n");
+		pr_err_once("host does not support poll control\n");
+		pr_err_once("host upgrade recommended\n");
 		return;
 	}
 
-- 
1.8.3.1

[PATCH v4 3/4] xen: Mark "xen_nopvspin" parameter obsolete

[Zhenzhong Duan]

Map "xen_nopvspin" to "nopvspin", fix stale description of "xen_nopvspin"
as we use qspinlock now.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 ++++---
 arch/x86/xen/spinlock.c                         | 3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 89d77ea..df1eacc 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5303,8 +5303,9 @@
 			never -- do not unplug even if version check succeeds
 
 	xen_nopvspin	[X86,XEN]
-			Disables the ticketlock slowpath using Xen PV
-			optimizations.
+			Disables the qspinlock slowpath using Xen PV optimizations.
+			This parameter is obsoleted by "nopvspin" parameter, which
+			has equivalent effect for XEN platform.
 
 	xen_nopv	[X86]
 			Disables the PV optimizations forcing the HVM guest to
@@ -5330,7 +5331,7 @@
 			as generic guest with no PV drivers. Currently support
 			XEN HVM, KVM, HYPER_V and VMWARE guest.
 
-	nopvspin	[X86,KVM]
+	nopvspin	[X86,XEN,KVM]
 			Disables the qspinlock slow path using PV optimizations
 			which allow the hypervisor to 'idle' the guest on lock
 			contention.
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 6deb490..bae29a4 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -114,6 +114,8 @@ void xen_uninit_lock_cpu(int cpu)
  */
 void __init xen_init_spinlocks(void)
 {
+	if (nopvspin)
+		xen_pvspin = false;
 
 	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
 	if (num_possible_cpus() == 1)
@@ -137,6 +139,7 @@ void __init xen_init_spinlocks(void)
 
 static __init int xen_parse_nopvspin(char *arg)
 {
+	pr_notice("\"xen_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
 	xen_pvspin = false;
 	return 0;
 }
-- 
1.8.3.1

[PATCH v4 4/4] x86/hyperv: Mark "hv_nopvspin" parameter obsolete

[Zhenzhong Duan]

Map "hv_nopvspin" to "nopvspin".

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 6 +++++-
 arch/x86/hyperv/hv_spinlock.c                   | 4 ++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index df1eacc..08c6d34 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1436,6 +1436,10 @@
 	hv_nopvspin	[X86,HYPER_V] Disables the paravirt spinlock optimizations
 				      which allow the hypervisor to 'idle' the
 				      guest on lock contention.
+				      This parameter is obsoleted by "nopvspin"
+				      parameter, which has equivalent effect for
+				      HYPER_V platform.
+
 
 	keep_bootcon	[KNL]
 			Do not unregister boot console at start. This is only
@@ -5331,7 +5335,7 @@
 			as generic guest with no PV drivers. Currently support
 			XEN HVM, KVM, HYPER_V and VMWARE guest.
 
-	nopvspin	[X86,XEN,KVM]
+	nopvspin	[X86,XEN,KVM,HYPER_V]
 			Disables the qspinlock slow path using PV optimizations
 			which allow the hypervisor to 'idle' the guest on lock
 			contention.
diff --git a/arch/x86/hyperv/hv_spinlock.c b/arch/x86/hyperv/hv_spinlock.c
index 07f21a0..47c7d6c 100644
--- a/arch/x86/hyperv/hv_spinlock.c
+++ b/arch/x86/hyperv/hv_spinlock.c
@@ -64,6 +64,9 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
 
 void __init hv_init_spinlocks(void)
 {
+	if (nopvspin)
+		hv_pvspin = false;
+
 	if (!hv_pvspin || !apic ||
 	    !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
 	    !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
@@ -82,6 +85,7 @@ void __init hv_init_spinlocks(void)
 
 static __init int hv_parse_nopvspin(char *arg)
 {
+	pr_notice("\"hv_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
 	hv_pvspin = false;
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCH v4 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Boris Ostrovsky]

On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
>  void __init kvm_spinlock_init(void)
>  {
> -	/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
> -	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
> -		return;
> -
> -	if (kvm_para_has_hint(KVM_HINTS_REALTIME))
> +	/*
> +	 * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT feature
> +	 * support, or there is REALTIME hints or only 1 vCPU.
> +	 */
> +	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
> +	    kvm_para_has_hint(KVM_HINTS_REALTIME) ||
> +	    num_possible_cpus() == 1) {
> +		pr_info("PV spinlocks disabled\n");
>  		return;
> +	}
>  
> -	/* Don't use the pvqspinlock code if there is only 1 vCPU. */
> -	if (num_possible_cpus() == 1)
> +	if (nopvspin) {
> +		pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");
> +		static_branch_disable(&virt_spin_lock_key);

Would it make sense to bring here the other site where the key is
disabled (in kvm_smp_prepare_cpus())?

(and, in fact, shouldn't all of the checks that result in early return
above disable the key?)

-boris

>  		return;
> +	}
> +	pr_info("PV spinlocks enabled\n");
>  
>  	__pv_init_lock_hash();
>  	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
>

Re: [PATCH v4 3/4] xen: Mark "xen_nopvspin" parameter obsolete

[Boris Ostrovsky]

On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
> Map "xen_nopvspin" to "nopvspin", fix stale description of "xen_nopvspin"
> as we use qspinlock now.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

with a small nit

>  void __init xen_init_spinlocks(void)
>  {
> +	if (nopvspin)
> +		xen_pvspin = false;
>  
>  	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
>  	if (num_possible_cpus() == 1)

I'd fold the change into this 'if' statement, I think it will still be
clear what the comment refers to.

-boris

RE: [PATCH v4 4/4] x86/hyperv: Mark "hv_nopvspin" parameter obsolete

[Michael Kelley]

From: Zhenzhong Duan <zhenzhong.duan@oracle.com>  Sent: Thursday, October 3, 2019 7:02 AM
> 
> Map "hv_nopvspin" to "nopvspin".
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 6 +++++-
>  arch/x86/hyperv/hv_spinlock.c                   | 4 ++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Re: [PATCH v4 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

On 2019/10/4 22:52, Boris Ostrovsky wrote:

> On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
>>   void __init kvm_spinlock_init(void)
>>   {
>> -	/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>> -	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>> -		return;
>> -
>> -	if (kvm_para_has_hint(KVM_HINTS_REALTIME))
>> +	/*
>> +	 * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT feature
>> +	 * support, or there is REALTIME hints or only 1 vCPU.
>> +	 */
>> +	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
>> +	    kvm_para_has_hint(KVM_HINTS_REALTIME) ||
>> +	    num_possible_cpus() == 1) {
>> +		pr_info("PV spinlocks disabled\n");
>>   		return;
>> +	}
>>   
>> -	/* Don't use the pvqspinlock code if there is only 1 vCPU. */
>> -	if (num_possible_cpus() == 1)
>> +	if (nopvspin) {
>> +		pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");
>> +		static_branch_disable(&virt_spin_lock_key);
> Would it make sense to bring here the other site where the key is
> disabled (in kvm_smp_prepare_cpus())?

Thanks for point out, I'll do it. Just not clear if I should do that in a separate patch,
there is a history about that code:

Its original place was here and then moved to kvm_smp_prepare_cpus() by below commit:
34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static key get initialized")
which fixed jump_label_init() calling late issue.

Then 8990cac6 ("x86/jump_label: Initialize static branching early") move jump_label_init()
early, so commit 34226b6b could be reverted.

>
> (and, in fact, shouldn't all of the checks that result in early return
> above disable the key?)

I think we should enable he key for !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case,
there is lock holder preemption issue as qspinlock is fair lock, virt_spin_lock()
is an optimization to that, imaging one pcpu running 10 vcpus of same guest
contending a same lock.

For kvm_para_has_hint(KVM_HINTS_REALTIME) case, hypervisor hints there is
no preemption and we should disable virt_spin_lock_key to use native qspinlock.

For the UP case, we don't care virt_spin_lock_key value.

For nopvspin case, we intentionally check native qspinlock code performance,
compare it with PV qspinlock, etc. So virt_spin_lock() optimization should be disabled.

Let me know if anything wrong with above understanding. Thanks

Zhenzhong

Re: [PATCH v4 3/4] xen: Mark "xen_nopvspin" parameter obsolete

[Zhenzhong Duan]

On 2019/10/4 22:57, Boris Ostrovsky wrote:
> On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
>> Map "xen_nopvspin" to "nopvspin", fix stale description of "xen_nopvspin"
>> as we use qspinlock now.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> with a small nit
>
>>   void __init xen_init_spinlocks(void)
>>   {
>> +	if (nopvspin)
>> +		xen_pvspin = false;
>>   
>>   	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
>>   	if (num_possible_cpus() == 1)
> I'd fold the change into this 'if' statement, I think it will still be
> clear what the comment refers to.

Good suggestion, will do that. Thanks

Zhenzhong

Re: [PATCH v4 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Boris Ostrovsky]

On 10/6/19 3:49 AM, Zhenzhong Duan wrote:
> On 2019/10/4 22:52, Boris Ostrovsky wrote:
>
>> On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
>>>   void __init kvm_spinlock_init(void)
>>>   {
>>> -    /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>>> -    if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>>> -        return;
>>> -
>>> -    if (kvm_para_has_hint(KVM_HINTS_REALTIME))
>>> +    /*
>>> +     * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT
>>> feature
>>> +     * support, or there is REALTIME hints or only 1 vCPU.
>>> +     */
>>> +    if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
>>> +        kvm_para_has_hint(KVM_HINTS_REALTIME) ||
>>> +        num_possible_cpus() == 1) {
>>> +        pr_info("PV spinlocks disabled\n");
>>>           return;
>>> +    }
>>>   -    /* Don't use the pvqspinlock code if there is only 1 vCPU. */
>>> -    if (num_possible_cpus() == 1)
>>> +    if (nopvspin) {
>>> +        pr_info("PV spinlocks disabled forced by \"nopvspin\"
>>> parameter.\n");
>>> +        static_branch_disable(&virt_spin_lock_key);
>> Would it make sense to bring here the other site where the key is
>> disabled (in kvm_smp_prepare_cpus())?
>
> Thanks for point out, I'll do it. Just not clear if I should do that
> in a separate patch,
> there is a history about that code:
>
> Its original place was here and then moved to kvm_smp_prepare_cpus()
> by below commit:
> 34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static
> key get initialized")
> which fixed jump_label_init() calling late issue.
>
> Then 8990cac6 ("x86/jump_label: Initialize static branching early")
> move jump_label_init()
> early, so commit 34226b6b could be reverted.


Which is similar to what you did earlier for Xen.


>
>>
>> (and, in fact, shouldn't all of the checks that result in early return
>> above disable the key?)
>
> I think we should enable he key for
> !kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) case,
> there is lock holder preemption issue as qspinlock is fair lock,
> virt_spin_lock()
> is an optimization to that, imaging one pcpu running 10 vcpus of same
> guest
> contending a same lock.

Right. I conflated pv lock and virt_spin_lock_key, and that is wrong.

-boris


>
> For kvm_para_has_hint(KVM_HINTS_REALTIME) case, hypervisor hints there is
> no preemption and we should disable virt_spin_lock_key to use native
> qspinlock.
>
> For the UP case, we don't care virt_spin_lock_key value.
>
> For nopvspin case, we intentionally check native qspinlock code
> performance,
> compare it with PV qspinlock, etc. So virt_spin_lock() optimization
> should be disabled.
>
> Let me know if anything wrong with above understanding. Thanks
>
> Zhenzhong
>

Re: [PATCH v4 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

On 2019/10/7 22:46, Boris Ostrovsky wrote:
> On 10/6/19 3:49 AM, Zhenzhong Duan wrote:
>> On 2019/10/4 22:52, Boris Ostrovsky wrote:
>>
>>> On 10/3/19 10:02 AM, Zhenzhong Duan wrote:
>>>>    void __init kvm_spinlock_init(void)
>>>>    {
>>>> -    /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>>>> -    if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>>>> -        return;
>>>> -
>>>> -    if (kvm_para_has_hint(KVM_HINTS_REALTIME))
>>>> +    /*
>>>> +     * Don't use the pvqspinlock code if no KVM_FEATURE_PV_UNHALT
>>>> feature
>>>> +     * support, or there is REALTIME hints or only 1 vCPU.
>>>> +     */
>>>> +    if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
>>>> +        kvm_para_has_hint(KVM_HINTS_REALTIME) ||
>>>> +        num_possible_cpus() == 1) {
>>>> +        pr_info("PV spinlocks disabled\n");
>>>>            return;
>>>> +    }
>>>>    -    /* Don't use the pvqspinlock code if there is only 1 vCPU. */
>>>> -    if (num_possible_cpus() == 1)
>>>> +    if (nopvspin) {
>>>> +        pr_info("PV spinlocks disabled forced by \"nopvspin\"
>>>> parameter.\n");
>>>> +        static_branch_disable(&virt_spin_lock_key);
>>> Would it make sense to bring here the other site where the key is
>>> disabled (in kvm_smp_prepare_cpus())?
>> Thanks for point out, I'll do it. Just not clear if I should do that
>> in a separate patch,
>> there is a history about that code:
>>
>> Its original place was here and then moved to kvm_smp_prepare_cpus()
>> by below commit:
>> 34226b6b ("KVM: X86: Fix setup the virt_spin_lock_key before static
>> key get initialized")
>> which fixed jump_label_init() calling late issue.
>>
>> Then 8990cac6 ("x86/jump_label: Initialize static branching early")
>> move jump_label_init()
>> early, so commit 34226b6b could be reverted.
> Which is similar to what you did earlier for Xen.

You remember that, ok, I'll do the same for KVM.

Thanks

Zhenzhong