[PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

On Thu, Jun 14, 2018 at 01:50:00PM +0100, Marc Zyngier wrote:

Hi Dave,

On 14/06/18 12:42, Dave Martin wrote:

quoted

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") introduces a specific helper
kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
vcpu_put().

This function uses local_bh_disable()/_enable() to protect the
FPSIMD context manipulation from interruption by softirqs.

This approach is not correct, because vcpu_put() can be invoked
either from the KVM host vcpu thread (when exiting the vcpu run
loop), or via a preempt notifier.  In the former case, only
preemption is disabled.  In the latter case, the function is called
from inside __schedule(), which means that IRQs are disabled.

Use of local_bh_disable()/_enable() with IRQs disabled is considerd
an error, resulting in lockdep splats while running VMs if lockdep
is enabled.

It is probably possible in theory to relax this restriction on
local_bh_disable()/_enable() usage, but for now this patch takes
the simple approach of managing softirq masking only if IRQs happen
to be enabled when kvm_arch_vcpu_put_fp() is called.

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Reported-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index dc6ecfa..b51ff80 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c

@@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
  * disappears and another task or vcpu appears that recycles the same
  * struct fpsimd_state.
  */
-void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 {
-	local_bh_disable();
-
 	update_thread_flag(TIF_SVE,
 			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
 

@@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		/* Ensure user trap controls are correctly restored */
 		fpsimd_bind_task_to_cpu();
 	}
+}
+
 
-	local_bh_enable();
+void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+{
+	if (irqs_disabled())
+		__kvm_arch_vcpu_put_fp(vcpu);
+	else {
+		local_bh_disable();
+		__kvm_arch_vcpu_put_fp(vcpu);
+		local_bh_enable();
+	}
 }

I'm of the opinion to always run kvm_arch_vcpu_put_fp() with interrupt
disabled. local_bh_enable() does quite a lot of stuff (running the
softirqs), which adds overhead we could do without.

I'd replace local_bh_{disable,enable} with local_irq_{save,restore).

I don't have a huge problem with that.  This creates interrupt blackout
on run loop exit, but it's a) not worse than the blackout in
__schedule() and b) presumably the rare case compared with run loop
preemption.

So, while disabling interrupts seemed a bit brutal, in context it
doesn't look like such a big deal.

Cheers
---Dave