diff mbox series

KVM: VMX: Dont' deliver posted IRQ if vCPU == this vCPU and vCPU is IN_GUEST_MODE

Message ID 1641471171-34232-1-git-send-email-wanpengli@tencent.com (mailing list archive)
State New, archived
Headers show
Series KVM: VMX: Dont' deliver posted IRQ if vCPU == this vCPU and vCPU is IN_GUEST_MODE | expand

Commit Message

Wanpeng Li Jan. 6, 2022, 12:12 p.m. UTC 
From: Wanpeng Li <wanpengli@tencent.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.

Suggested-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/vmx/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Jan. 8, 2022, 12:17 a.m. UTC | #1 
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@tencent.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@google.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
Wanpeng Li Jan. 8, 2022, 2:08 a.m. UTC | #2 
On Sat, 8 Jan 2022 at 08:17, Sean Christopherson <seanjc@google.com> wrote:
>
> 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@tencent.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?

Thanks for these updates, Sean.

    Wanpeng

>
> With that,
>
> Reviewed-by: Sean Christopherson <seanjc@google.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
>
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fe06b02..71e8afc 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3932,7 +3932,8 @@  static inline void kvm_vcpu_trigger_posted_interrupt(struct kvm_vcpu *vcpu,
 		 * which has no effect is safe here.
 		 */
 
-		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