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@xxxxxxxxxx>
......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
