linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Add a unified parameter "nopvspin"
@ 2019-09-30 12:44 Zhenzhong Duan
  2019-09-30 12:44 ` [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks Zhenzhong Duan
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-09-30 12:44 UTC (permalink / raw)
  To: linux-kernel; +Cc: vkuznets, linux-hyperv, kvm, 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.

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]

Thanks
Zhenzhong

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
  2019-09-30 12:44 [PATCH v3 0/4] Add a unified parameter "nopvspin" Zhenzhong Duan
@ 2019-09-30 12:44 ` Zhenzhong Duan
  2019-10-02 17:10   ` Sean Christopherson
  2019-09-30 12:44 ` [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format Zhenzhong Duan
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Zhenzhong Duan @ 2019-09-30 12:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkuznets, linux-hyperv, kvm, Zhenzhong Duan, Jonathan Corbet,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Paolo Bonzini, Radim Krcmar, Sean Christopherson, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Peter Zijlstra, Will Deacon

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.

This new parameter is also used to replace "xen_nopvspin" and
"hv_nopvspin".

The global variable pvspin isn't defined as __initdata as it's used at
runtime by XEN guest.

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 | 4 ++++
 arch/x86/include/asm/qspinlock.h                | 1 +
 arch/x86/kernel/kvm.c                           | 7 +++++++
 kernel/locking/qspinlock.c                      | 7 +++++++
 4 files changed, 19 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c7ac2f3..4b956d8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5330,6 +5330,10 @@
 			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..34a4484 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 pvspin;
 
 #define	queued_spin_unlock queued_spin_unlock
 /**
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index e820568..a4f108d 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void)
 	if (num_possible_cpus() == 1)
 		return;
 
+	if (!pvspin) {
+		pr_info("PV spinlocks disabled\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;
 	pv_ops.lock.queued_spin_unlock =
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 2473f10..945b510 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 pvspin = true;
+static __init int parse_nopvspin(char *arg)
+{
+	pvspin = false;
+	return 0;
+}
+early_param("nopvspin", parse_nopvspin);
 #endif
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format
  2019-09-30 12:44 [PATCH v3 0/4] Add a unified parameter "nopvspin" Zhenzhong Duan
  2019-09-30 12:44 ` [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks Zhenzhong Duan
@ 2019-09-30 12:44 ` Zhenzhong Duan
  2019-10-02 17:15   ` Sean Christopherson
  2019-09-30 12:44 ` [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin" Zhenzhong Duan
  2019-09-30 12:44 ` [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" " Zhenzhong Duan
  3 siblings, 1 reply; 13+ messages in thread
From: Zhenzhong Duan @ 2019-09-30 12:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkuznets, linux-hyperv, kvm, Zhenzhong Duan, Paolo Bonzini,
	Radim Krcmar, Sean Christopherson, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin

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

No functional change.

Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
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 | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index a4f108d..ce4f578 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: " 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)
@@ -509,7 +509,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 +639,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 +746,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 +879,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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
  2019-09-30 12:44 [PATCH v3 0/4] Add a unified parameter "nopvspin" Zhenzhong Duan
  2019-09-30 12:44 ` [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks Zhenzhong Duan
  2019-09-30 12:44 ` [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format Zhenzhong Duan
@ 2019-09-30 12:44 ` Zhenzhong Duan
  2019-10-02 17:18   ` Sean Christopherson
  2019-09-30 12:44 ` [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" " Zhenzhong Duan
  3 siblings, 1 reply; 13+ messages in thread
From: Zhenzhong Duan @ 2019-09-30 12:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkuznets, linux-hyperv, kvm, Zhenzhong Duan, Jonathan Corbet,
	Boris Ostrovsky, Juergen Gross, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

Fix stale description of "xen_nopvspin" as we use qspinlock now.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Reviewed-by: Juergen Gross <jgross@suse.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                         | 13 +++++++------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 4b956d8..1f0a62f 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] Disables the qspinlock slow path
+	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..092a53f 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -18,7 +18,6 @@
 static DEFINE_PER_CPU(int, lock_kicker_irq) = -1;
 static DEFINE_PER_CPU(char *, irq_name);
 static DEFINE_PER_CPU(atomic_t, xen_qlock_wait_nest);
-static bool xen_pvspin = true;
 
 static void xen_qlock_kick(int cpu)
 {
@@ -68,7 +67,7 @@ void xen_init_lock_cpu(int cpu)
 	int irq;
 	char *name;
 
-	if (!xen_pvspin)
+	if (!pvspin)
 		return;
 
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
@@ -93,7 +92,7 @@ void xen_init_lock_cpu(int cpu)
 
 void xen_uninit_lock_cpu(int cpu)
 {
-	if (!xen_pvspin)
+	if (!pvspin)
 		return;
 
 	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
@@ -117,9 +116,9 @@ void __init xen_init_spinlocks(void)
 
 	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
 	if (num_possible_cpus() == 1)
-		xen_pvspin = false;
+		pvspin = false;
 
-	if (!xen_pvspin) {
+	if (!pvspin) {
 		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
 		static_branch_disable(&virt_spin_lock_key);
 		return;
@@ -137,7 +136,9 @@ void __init xen_init_spinlocks(void)
 
 static __init int xen_parse_nopvspin(char *arg)
 {
-	xen_pvspin = false;
+	pr_notice("\"xen_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
+	if (xen_domain())
+		pvspin = false;
 	return 0;
 }
 early_param("xen_nopvspin", xen_parse_nopvspin);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
  2019-09-30 12:44 [PATCH v3 0/4] Add a unified parameter "nopvspin" Zhenzhong Duan
                   ` (2 preceding siblings ...)
  2019-09-30 12:44 ` [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin" Zhenzhong Duan
@ 2019-09-30 12:44 ` Zhenzhong Duan
  2019-10-02 17:19   ` Sean Christopherson
  3 siblings, 1 reply; 13+ messages in thread
From: Zhenzhong Duan @ 2019-09-30 12:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkuznets, linux-hyperv, kvm, Zhenzhong Duan, Jonathan Corbet,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

Includes asm/hypervisor.h in order to reference x86_hyper_type.

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                   | 9 +++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1f0a62f..43f922c 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] Disables the qspinlock slow path
+	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..e00e319 100644
--- a/arch/x86/hyperv/hv_spinlock.c
+++ b/arch/x86/hyperv/hv_spinlock.c
@@ -12,12 +12,11 @@
 
 #include <linux/spinlock.h>
 
+#include <asm/hypervisor.h>
 #include <asm/mshyperv.h>
 #include <asm/paravirt.h>
 #include <asm/apic.h>
 
-static bool __initdata hv_pvspin = true;
-
 static void hv_qlock_kick(int cpu)
 {
 	apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
@@ -64,7 +63,7 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
 
 void __init hv_init_spinlocks(void)
 {
-	if (!hv_pvspin || !apic ||
+	if (!pvspin || !apic ||
 	    !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
 	    !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
 		pr_info("PV spinlocks disabled\n");
@@ -82,7 +81,9 @@ void __init hv_init_spinlocks(void)
 
 static __init int hv_parse_nopvspin(char *arg)
 {
-	hv_pvspin = false;
+	pr_notice("\"hv_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
+	if (x86_hyper_type == X86_HYPER_MS_HYPERV)
+		pvspin = false;
 	return 0;
 }
 early_param("hv_nopvspin", hv_parse_nopvspin);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
  2019-09-30 12:44 ` [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks Zhenzhong Duan
@ 2019-10-02 17:10   ` Sean Christopherson
  2019-10-03 10:30     ` Zhenzhong Duan
  0 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2019-10-02 17:10 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Jonathan Corbet,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Paolo Bonzini, Radim Krcmar, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Peter Zijlstra, Will Deacon

On Mon, Sep 30, 2019 at 08:44:36PM +0800, Zhenzhong Duan wrote:
> 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.
> 
> This new parameter is also used to replace "xen_nopvspin" and
> "hv_nopvspin".

This is confusing as there are no Xen or Hyper-V changes in this patch.
Please make it clear that you're talking about future patches, e.g.:

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

> 
> The global variable pvspin isn't defined as __initdata as it's used at
> runtime by XEN guest.

Same comment as above regarding what this patch is doing versus what will
be done in the future.  Arguably you should even mark it __initdata in
this patch and deal with conflict in the Xen patch, e.g. use it only to
set the existing xen_pvspin variable.

> 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 | 4 ++++
>  arch/x86/include/asm/qspinlock.h                | 1 +
>  arch/x86/kernel/kvm.c                           | 7 +++++++
>  kernel/locking/qspinlock.c                      | 7 +++++++
>  4 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index c7ac2f3..4b956d8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5330,6 +5330,10 @@
>  			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..34a4484 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 pvspin;
>  
>  #define	queued_spin_unlock queued_spin_unlock
>  /**
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index e820568..a4f108d 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void)
>  	if (num_possible_cpus() == 1)
>  		return;
>  
> +	if (!pvspin) {
> +		pr_info("PV spinlocks disabled\n");
> +		static_branch_disable(&virt_spin_lock_key);
> +		return;
> +	}
> +	pr_info("PV spinlocks enabled\n");

These prints could be confusing as KVM also disables PV spinlocks when it
sees KVM_HINTS_REALTIME.

> +
>  	__pv_init_lock_hash();
>  	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
>  	pv_ops.lock.queued_spin_unlock =
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index 2473f10..945b510 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 pvspin = true;

This can be __ro_after_init, or probably better __initdata and have Xen
snapshot the value for its use case.

Personal preference: I'd invert the bool and name it nopvspin to make it
easier to connect the variable to the kernel param.

> +static __init int parse_nopvspin(char *arg)
> +{
> +	pvspin = false;
> +	return 0;
> +}
> +early_param("nopvspin", parse_nopvspin);
>  #endif
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format
  2019-09-30 12:44 ` [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format Zhenzhong Duan
@ 2019-10-02 17:15   ` Sean Christopherson
  2019-10-03 10:32     ` Zhenzhong Duan
  0 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2019-10-02 17:15 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Paolo Bonzini,
	Radim Krcmar, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

On Mon, Sep 30, 2019 at 08:44:37PM +0800, Zhenzhong Duan wrote:
> pr_*() is preferred than printk(KERN_* ...), after change all the print
> in arch/x86/kernel/kvm.c will have "KVM: xxx" style.
> 
> No functional change.
> 
> Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>

This wasn't really suggested by Vitaly, he just requested it be done in a
separate patch.

> 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 | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index a4f108d..ce4f578 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: " fmt

Not a fan of "KVM" as the prefix as it's easily confused with KVM the
hypervisor.  Maybe "kvm_guest"?

> +
>  #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)
> @@ -509,7 +509,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 +639,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 +746,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 +879,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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
  2019-09-30 12:44 ` [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin" Zhenzhong Duan
@ 2019-10-02 17:18   ` Sean Christopherson
  2019-10-03 11:21     ` Zhenzhong Duan
  0 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2019-10-02 17:18 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Jonathan Corbet,
	Boris Ostrovsky, Juergen Gross, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

On Mon, Sep 30, 2019 at 08:44:38PM +0800, Zhenzhong Duan wrote:
> Fix stale description of "xen_nopvspin" as we use qspinlock now.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Reviewed-by: Juergen Gross <jgross@suse.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                         | 13 +++++++------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4b956d8..1f0a62f 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] Disables the qspinlock slow path
> +	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..092a53f 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -18,7 +18,6 @@
>  static DEFINE_PER_CPU(int, lock_kicker_irq) = -1;
>  static DEFINE_PER_CPU(char *, irq_name);
>  static DEFINE_PER_CPU(atomic_t, xen_qlock_wait_nest);
> -static bool xen_pvspin = true;
>  
>  static void xen_qlock_kick(int cpu)
>  {
> @@ -68,7 +67,7 @@ void xen_init_lock_cpu(int cpu)
>  	int irq;
>  	char *name;
>  
> -	if (!xen_pvspin)
> +	if (!pvspin)
>  		return;
>  
>  	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
> @@ -93,7 +92,7 @@ void xen_init_lock_cpu(int cpu)
>  
>  void xen_uninit_lock_cpu(int cpu)
>  {
> -	if (!xen_pvspin)
> +	if (!pvspin)
>  		return;
>  
>  	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
> @@ -117,9 +116,9 @@ void __init xen_init_spinlocks(void)
>  
>  	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
>  	if (num_possible_cpus() == 1)
> -		xen_pvspin = false;
> +		pvspin = false;

As suggested in the other patch, if you incorporate pvspin (or nopvspin)
into xen_pvspin then the param can be __initdata and the diff for this
patch will be smaller, e.g. you wouldn't need the xen_domain() shenanigans
in xen_parse_nopvspin().

> -	if (!xen_pvspin) {
> +	if (!pvspin) {
>  		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
>  		static_branch_disable(&virt_spin_lock_key);
>  		return;
> @@ -137,7 +136,9 @@ void __init xen_init_spinlocks(void)
>  
>  static __init int xen_parse_nopvspin(char *arg)
>  {
> -	xen_pvspin = false;
> +	pr_notice("\"xen_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
> +	if (xen_domain())
> +		pvspin = false;
>  	return 0;
>  }
>  early_param("xen_nopvspin", xen_parse_nopvspin);
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
  2019-09-30 12:44 ` [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" " Zhenzhong Duan
@ 2019-10-02 17:19   ` Sean Christopherson
  2019-10-03 11:22     ` Zhenzhong Duan
  0 siblings, 1 reply; 13+ messages in thread
From: Sean Christopherson @ 2019-10-02 17:19 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Jonathan Corbet,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

On Mon, Sep 30, 2019 at 08:44:39PM +0800, Zhenzhong Duan wrote:
> Includes asm/hypervisor.h in order to reference x86_hyper_type.
> 
> 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                   | 9 +++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1f0a62f..43f922c 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] Disables the qspinlock slow path
> +	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..e00e319 100644
> --- a/arch/x86/hyperv/hv_spinlock.c
> +++ b/arch/x86/hyperv/hv_spinlock.c
> @@ -12,12 +12,11 @@
>  
>  #include <linux/spinlock.h>
>  
> +#include <asm/hypervisor.h>
>  #include <asm/mshyperv.h>
>  #include <asm/paravirt.h>
>  #include <asm/apic.h>
>  
> -static bool __initdata hv_pvspin = true;
> -
>  static void hv_qlock_kick(int cpu)
>  {
>  	apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
> @@ -64,7 +63,7 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
>  
>  void __init hv_init_spinlocks(void)
>  {
> -	if (!hv_pvspin || !apic ||
> +	if (!pvspin || !apic ||
>  	    !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
>  	    !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
>  		pr_info("PV spinlocks disabled\n");
> @@ -82,7 +81,9 @@ void __init hv_init_spinlocks(void)
>  
>  static __init int hv_parse_nopvspin(char *arg)
>  {
> -	hv_pvspin = false;
> +	pr_notice("\"hv_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
> +	if (x86_hyper_type == X86_HYPER_MS_HYPERV)
> +		pvspin = false;

Personal preference would be to keep the hv_pvspin variable and add the
extra check in hv_init_spinlocks().

>  	return 0;
>  }
>  early_param("hv_nopvspin", hv_parse_nopvspin);
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
  2019-10-02 17:10   ` Sean Christopherson
@ 2019-10-03 10:30     ` Zhenzhong Duan
  0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-10-03 10:30 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Jonathan Corbet,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Paolo Bonzini, Radim Krcmar, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Peter Zijlstra, Will Deacon

On 2019/10/3 1:10, Sean Christopherson wrote:

> On Mon, Sep 30, 2019 at 08:44:36PM +0800, Zhenzhong Duan wrote:
>> 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.
>>
>> This new parameter is also used to replace "xen_nopvspin" and
>> "hv_nopvspin".
> This is confusing as there are no Xen or Hyper-V changes in this patch.
> Please make it clear that you're talking about future patches, e.g.:
>
>    The new 'nopvspin' parameter will also replace Xen and Hyper-V specific
>    parameters in future patches.

Will fix

>
>> The global variable pvspin isn't defined as __initdata as it's used at
>> runtime by XEN guest.
> Same comment as above regarding what this patch is doing versus what will
> be done in the future.  Arguably you should even mark it __initdata in
> this patch and deal with conflict in the Xen patch, e.g. use it only to
> set the existing xen_pvspin variable.

Will fix

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>

......snip

>>   /**
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index e820568..a4f108d 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void)
>>   	if (num_possible_cpus() == 1)
>>   		return;
>>   
>> +	if (!pvspin) {
>> +		pr_info("PV spinlocks disabled\n");
>> +		static_branch_disable(&virt_spin_lock_key);
>> +		return;
>> +	}
>> +	pr_info("PV spinlocks enabled\n");
> These prints could be confusing as KVM also disables PV spinlocks when it
> sees KVM_HINTS_REALTIME.

What about below:

pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");

Or you prefer separate print for each disabling like below?

         /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
         if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) {
                 pr_info("PV spinlocks disabled, KVM_FEATURE_PV_UNHALT feature needed.\n");
                 return;
         }

         if (kvm_para_has_hint(KVM_HINTS_REALTIME)) {
                 pr_info("PV spinlocks disabled, having non-preemption hints.\n");
                 return;
         }

         /* Don't use the pvqspinlock code if there is only 1 vCPU. */
         if (num_possible_cpus() == 1) {
                 pr_info("PV spinlocks disabled on UP.\n");
                 return;
         }
	if (!pvspin) {
		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;
>>   	pv_ops.lock.queued_spin_unlock =
>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>> index 2473f10..945b510 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 pvspin = true;
> This can be __ro_after_init, or probably better __initdata and have Xen
> snapshot the value for its use case.

I will use __initdata

>
> Personal preference: I'd invert the bool and name it nopvspin to make it
> easier to connect the variable to the kernel param.

OK, will do that.  Thanks for review for all the patches.

Zhenzhong


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format
  2019-10-02 17:15   ` Sean Christopherson
@ 2019-10-03 10:32     ` Zhenzhong Duan
  0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-10-03 10:32 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Paolo Bonzini,
	Radim Krcmar, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

On 2019/10/3 1:15, Sean Christopherson wrote:

> On Mon, Sep 30, 2019 at 08:44:37PM +0800, Zhenzhong Duan wrote:
>> pr_*() is preferred than printk(KERN_* ...), after change all the print
>> in arch/x86/kernel/kvm.c will have "KVM: xxx" style.
>>
>> No functional change.
>>
>> Suggested-by: Vitaly Kuznetsov<vkuznets@redhat.com>
> This wasn't really suggested by Vitaly, he just requested it be done in a
> separate patch.

Will fix.

>
>> 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 | 24 ++++++++++++------------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index a4f108d..ce4f578 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: " fmt
> Not a fan of "KVM" as the prefix as it's easily confused with KVM the
> hypervisor.  Maybe "kvm_guest"?

Yeah, looks better, will change to"kvm_guest". Thanks

Zhenzhong


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
  2019-10-02 17:18   ` Sean Christopherson
@ 2019-10-03 11:21     ` Zhenzhong Duan
  0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-10-03 11:21 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Jonathan Corbet,
	Boris Ostrovsky, Juergen Gross, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin


On 2019/10/3 1:18, Sean Christopherson wrote:
> On Mon, Sep 30, 2019 at 08:44:38PM +0800, Zhenzhong Duan wrote:
>> Fix stale description of "xen_nopvspin" as we use qspinlock now.
>>
>> Signed-off-by: Zhenzhong Duan<zhenzhong.duan@oracle.com>
>> Reviewed-by: Juergen Gross<jgross@suse.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>
>> ---
...snip
>> @@ -93,7 +92,7 @@ void xen_init_lock_cpu(int cpu)
>>   
>>   void xen_uninit_lock_cpu(int cpu)
>>   {
>> -	if (!xen_pvspin)
>> +	if (!pvspin)
>>   		return;
>>   
>>   	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
>> @@ -117,9 +116,9 @@ void __init xen_init_spinlocks(void)
>>   
>>   	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
>>   	if (num_possible_cpus() == 1)
>> -		xen_pvspin = false;
>> +		pvspin = false;
> As suggested in the other patch, if you incorporate pvspin (or nopvspin)
> into xen_pvspin then the param can be __initdata and the diff for this
> patch will be smaller, e.g. you wouldn't need the xen_domain() shenanigans
> in xen_parse_nopvspin().

Ok, will fix. Thanks

Zhenzhong


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
  2019-10-02 17:19   ` Sean Christopherson
@ 2019-10-03 11:22     ` Zhenzhong Duan
  0 siblings, 0 replies; 13+ messages in thread
From: Zhenzhong Duan @ 2019-10-03 11:22 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, vkuznets, linux-hyperv, kvm, Jonathan Corbet,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin


On 2019/10/3 1:19, Sean Christopherson wrote:
> On Mon, Sep 30, 2019 at 08:44:39PM +0800, Zhenzhong Duan wrote:
>> Includes asm/hypervisor.h in order to reference x86_hyper_type.
>>
>> 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>
>> ---
...snip
>> @@ -64,7 +63,7 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
>>   
>>   void __init hv_init_spinlocks(void)
>>   {
>> -	if (!hv_pvspin || !apic ||
>> +	if (!pvspin || !apic ||
>>   	    !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
>>   	    !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
>>   		pr_info("PV spinlocks disabled\n");
>> @@ -82,7 +81,9 @@ void __init hv_init_spinlocks(void)
>>   
>>   static __init int hv_parse_nopvspin(char *arg)
>>   {
>> -	hv_pvspin = false;
>> +	pr_notice("\"hv_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
>> +	if (x86_hyper_type == X86_HYPER_MS_HYPERV)
>> +		pvspin = false;
> Personal preference would be to keep the hv_pvspin variable and add the
> extra check in hv_init_spinlocks().

OK, will do that way. Thanks

Zhenzhong


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-10-03 11:22 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30 12:44 [PATCH v3 0/4] Add a unified parameter "nopvspin" Zhenzhong Duan
2019-09-30 12:44 ` [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks Zhenzhong Duan
2019-10-02 17:10   ` Sean Christopherson
2019-10-03 10:30     ` Zhenzhong Duan
2019-09-30 12:44 ` [PATCH v3 2/4] x86/kvm: Change print code to use pr_*() format Zhenzhong Duan
2019-10-02 17:15   ` Sean Christopherson
2019-10-03 10:32     ` Zhenzhong Duan
2019-09-30 12:44 ` [PATCH v3 3/4] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin" Zhenzhong Duan
2019-10-02 17:18   ` Sean Christopherson
2019-10-03 11:21     ` Zhenzhong Duan
2019-09-30 12:44 ` [PATCH v3 4/4] x86/hyperv: Mark "hv_nopvspin" " Zhenzhong Duan
2019-10-02 17:19   ` Sean Christopherson
2019-10-03 11:22     ` Zhenzhong Duan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).