Date:   Sat, 8 Jan 2022 10:08:59 +0800
From:   Wanpeng Li <kernellwp@...il.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, kvm <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

On Sat, 8 Jan 2022 at 08:17, Sean Christopherson <seanjc@...gle.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@...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?

Thanks for these updates, Sean.

    Wanpeng

>
> 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
>

Powered by blists - more mailing lists