linux-kernel - Re: [PATCH] KVM: VMX: Dont' deliver posted IRQ if vCPU == this vCPU and vCPU is IN_GUEST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdjX//gxZtP/ZMME@google.com>
Date:   Sat, 8 Jan 2022 00:17:03 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Wanpeng Li <kernellwp@...il.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH] KVM: VMX: Dont' deliver posted IRQ if vCPU == this vCPU
 and vCPU is IN_GUEST_MODE

Nit, s/deliver/send, "deliver" reads as though KVM is ignoring an event that was
sent by something else.

On Thu, Jan 06, 2022, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@...cent.com>
> 
> Commit fdba608f15e2 (KVM: VMX: Wake vCPU when delivering posted IRQ even 
> if vCPU == this vCPU) fixes wakeup event is missing when it is not from 
> synchronous kvm context by dropping vcpu == running_vcpu checking completely.
> However, it will break the original goal to optimise timer fastpath, let's 
> move the checking under vCPU is IN_GUEST_MODE to restore the performance.

Please (a) explain why this is safe and (b) provide context for exactly what
fastpath this helpers.  Lack of context is partly what led to the optimization
being reverted instead of being fixed as below, and forcing readers to jump through
multiple changelogs to understand what's going on is unnecessarily mean.

E.g.

  When delivering a virtual interrupt, don't actually send a posted interrupt
  if the target vCPU is also the currently running vCPU and is IN_GUEST_MODE,
  in which case the interrupt is being sent from a VM-Exit fastpath and the
  core run loop in vcpu_enter_guest() will manually move the interrupt from
  the PIR to vmcs.GUEST_RVI.  IRQs are disabled while IN_GUEST_MODE, thus
  there's no possibility of the virtual interrupt being sent from anything
  other than KVM, i.e. KVM won't suppress a wake event from an IRQ handler
  (see commit fdba608f15e2, "KVM: VMX: Wake vCPU when delivering posted IRQ
  even if vCPU == this vCPU").

  Eliding the posted interrupt restores the performance provided by the
  combination of commits 379a3c8ee444 ("KVM: VMX: Optimize posted-interrupt
  delivery for timer fastpath") and 26efe2fd92e5 ("KVM: VMX: Handle
  preemption timer fastpath").

The comment above send_IPI_mask() also needs to be updated.  There are a few
existing grammar and style nits that can be opportunistically cleaned up, too.

Paolo, if Wanpeng doesn't object, can you use the above changelog and the below
comment?

With that,

Reviewed-by: Sean Christopherson <seanjc@...gle.com>

---
 arch/x86/kvm/vmx/vmx.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fe06b02994e6..730df0e183d6 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3908,31 +3908,32 @@ static inline void kvm_vcpu_trigger_posted_interrupt(struct kvm_vcpu *vcpu,
 #ifdef CONFIG_SMP
 	if (vcpu->mode == IN_GUEST_MODE) {
 		/*
-		 * The vector of interrupt to be delivered to vcpu had
-		 * been set in PIR before this function.
+		 * The vector of the virtual has already been set in the PIR.
+		 * Send a notification event to deliver the virtual interrupt
+		 * unless the vCPU is the currently running vCPU, i.e. the
+		 * event is being sent from a fastpath VM-Exit handler, in
+		 * which case the PIR will be synced to the vIRR before
+		 * re-entering the guest.
 		 *
-		 * Following cases will be reached in this block, and
-		 * we always send a notification event in all cases as
-		 * explained below.
+		 * When the target is not the running vCPU, the following
+		 * possibilities emerge:
 		 *
-		 * Case 1: vcpu keeps in non-root mode. Sending a
-		 * notification event posts the interrupt to vcpu.
+		 * Case 1: vCPU stays in non-root mode. Sending a notification
+		 * event posts the interrupt to the vCPU.
 		 *
-		 * Case 2: vcpu exits to root mode and is still
-		 * runnable. PIR will be synced to vIRR before the
-		 * next vcpu entry. Sending a notification event in
-		 * this case has no effect, as vcpu is not in root
-		 * mode.
+		 * Case 2: vCPU exits to root mode and is still runnable. The
+		 * PIR will be synced to the vIRR before re-entering the guest.
+		 * Sending a notification event is ok as the host IRQ handler
+		 * will ignore the spurious event.
 		 *
-		 * Case 3: vcpu exits to root mode and is blocked.
-		 * vcpu_block() has already synced PIR to vIRR and
-		 * never blocks vcpu if vIRR is not cleared. Therefore,
-		 * a blocked vcpu here does not wait for any requested
-		 * interrupts in PIR, and sending a notification event
-		 * which has no effect is safe here.
+		 * Case 3: vCPU exits to root mode and is blocked. vcpu_block()
+		 * has already synced PIR to vIRR and never blocks the vCPU if
+		 * the vIRR is not empty. Therefore, a blocked vCPU here does
+		 * not wait for any requested interrupts in PIR, and sending a
+		 * notification event also results in a benign, spurious event.
 		 */
-
-		apic->send_IPI_mask(get_cpu_mask(vcpu->cpu), pi_vec);
+		if (vcpu != kvm_get_running_vcpu())
+			apic->send_IPI_mask(get_cpu_mask(vcpu->cpu), pi_vec);
 		return;
 	}
 #endif