Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201127112114.3219360-1-pbonzini@redhat.com> <20201127112114.3219360-3-pbonzini@redhat.com>
In-Reply-To: <20201127112114.3219360-3-pbonzini@redhat.com>
From:   Lai Jiangshan <jiangshanlai+lkml@gmail.com>
Date:   Fri, 9 Apr 2021 15:14:10 +0800
Message-ID: <CAJhGHyCdqgtvK98_KieG-8MUfg1Jghd+H99q+FkgL0ZuqnvuAw@mail.gmail.com>
Subject: Re: [PATCH 2/2] KVM: x86: Fix split-irqchip vs interrupt injection
 window request
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, kvm@vger.kernel.org,
        Sean Christopherson <seanjc@google.com>,
        Filippo Sironi <sironi@amazon.de>,
        David Woodhouse <dwmw@amazon.co.uk>,
        "v4.7+" <stable@vger.kernel.org>,
        Wanpeng Li <wanpengli@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Nov 27, 2020 at 7:26 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> kvm_cpu_accept_dm_intr and kvm_vcpu_ready_for_interrupt_injection are
> a hodge-podge of conditions, hacked together to get something that
> more or less works.  But what is actually needed is much simpler;
> in both cases the fundamental question is, do we have a place to stash
> an interrupt if userspace does KVM_INTERRUPT?
>
> In userspace irqchip mode, that is !vcpu->arch.interrupt.injected.
> Currently kvm_event_needs_reinjection(vcpu) covers it, but it is
> unnecessarily restrictive.
>
> In split irqchip mode it's a bit more complicated, we need to check
> kvm_apic_accept_pic_intr(vcpu) (the IRQ window exit is basically an INTACK
> cycle and thus requires ExtINTs not to be masked) as well as
> !pending_userspace_extint(vcpu).  However, there is no need to
> check kvm_event_needs_reinjection(vcpu), since split irqchip keeps
> pending ExtINT state separate from event injection state, and checking
> kvm_cpu_has_interrupt(vcpu) is wrong too since ExtINT has higher
> priority than APIC interrupts.  In fact the latter fixes a bug:
> when userspace requests an IRQ window vmexit, an interrupt in the
> local APIC can cause kvm_cpu_has_interrupt() to be true and thus
> kvm_vcpu_ready_for_interrupt_injection() to return false.  When this
> happens, vcpu_run does not exit to userspace but the interrupt window
> vmexits keep occurring.  The VM loops without any hope of making progress.
>
> Once we try to fix these with something like
>
>      return kvm_arch_interrupt_allowed(vcpu) &&
> -        !kvm_cpu_has_interrupt(vcpu) &&
> -        !kvm_event_needs_reinjection(vcpu) &&
> -        kvm_cpu_accept_dm_intr(vcpu);
> +        (!lapic_in_kernel(vcpu)
> +         ? !vcpu->arch.interrupt.injected
> +         : (kvm_apic_accept_pic_intr(vcpu)
> +            && !pending_userspace_extint(v)));
>
> we realize two things.  First, thanks to the previous patch the complex
> conditional can reuse !kvm_cpu_has_extint(vcpu).  Second, the interrupt
> window request in vcpu_enter_guest()
>
>         bool req_int_win =
>                 dm_request_for_irq_injection(vcpu) &&
>                 kvm_cpu_accept_dm_intr(vcpu);
>
> should be kept in sync with kvm_vcpu_ready_for_interrupt_injection():
> it is unnecessary to ask the processor for an interrupt window
> if we would not be able to return to userspace.  Therefore, the
> complex conditional is really the correct implementation of
> kvm_cpu_accept_dm_intr(vcpu).  It all makes sense:
>
> - we can accept an interrupt from userspace if there is a place
>   to stash it (and, for irqchip split, ExtINTs are not masked).
>   Interrupts from userspace _can_ be accepted even if right now
>   EFLAGS.IF=0.

Hello, Paolo

If userspace does KVM_INTERRUPT, vcpu->arch.interrupt.injected is
set immediately, and in inject_pending_event(), we have

        else if (!vcpu->arch.exception.pending) {
                if (vcpu->arch.nmi_injected) {
                        kvm_x86_ops.set_nmi(vcpu);
                        can_inject = false;
                } else if (vcpu->arch.interrupt.injected) {
                        kvm_x86_ops.set_irq(vcpu);
                        can_inject = false;
                }
        }

I'm curious about that can the kvm_x86_ops.set_irq() here be possible
to queue the irq with EFLAGS.IF=0? If not, which code prevents it?

I'm asking about this because I just noticed that interrupt can
be queued when exception pending, and this patch relaxed it even
more.

Note: interrupt can NOT be queued when exception pending
until 664f8e26b00c7 ("KVM: X86: Fix loss of exception which
has not yet been injected") which I think is dangerous.

Thanks
Lai

>
> - in order to tell userspace we will inject its interrupt ("IRQ
>   window open" i.e. kvm_vcpu_ready_for_interrupt_injection), both
>   KVM and the vCPU need to be ready to accept the interrupt.
>
> ... and this is what the patch implements.
>
> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
> Analyzed-by: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/irq.c              |  2 +-
>  arch/x86/kvm/x86.c              | 17 +++++++----------
>  3 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d44858b69353..ddaf3e01a854 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1655,6 +1655,7 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
>  int kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
>  int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
>  int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
> +int kvm_cpu_has_extint(struct kvm_vcpu *v);
>  int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
>  int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
>  void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event);
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index e2d49a506e7f..fa01f07e449e 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -40,7 +40,7 @@ static int pending_userspace_extint(struct kvm_vcpu *v)
>   * check if there is pending interrupt from
>   * non-APIC source without intack.
>   */
> -static int kvm_cpu_has_extint(struct kvm_vcpu *v)
> +int kvm_cpu_has_extint(struct kvm_vcpu *v)
>  {
>         /*
>          * FIXME: interrupt.injected represents an interrupt that it's
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 447edc0d1d5a..54124b6211df 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4051,21 +4051,22 @@ static int kvm_vcpu_ioctl_set_lapic(struct kvm_vcpu *vcpu,
>
>  static int kvm_cpu_accept_dm_intr(struct kvm_vcpu *vcpu)
>  {
> -       return (!lapic_in_kernel(vcpu) ||
> -               kvm_apic_accept_pic_intr(vcpu));
> +       /*
> +        * We can accept userspace's request for interrupt injection
> +        * as long as we have a place to store the interrupt number.
> +        * The actual injection will happen when the CPU is able to
> +        * deliver the interrupt.
> +        */
> +       if (kvm_cpu_has_extint(vcpu))
> +               return false;
> +
> +       /* Acknowledging ExtINT does not happen if LINT0 is masked.  */
> +       return !(lapic_in_kernel(vcpu) && !kvm_apic_accept_pic_intr(vcpu));
>  }
>
> -/*
> - * if userspace requested an interrupt window, check that the
> - * interrupt window is open.
> - *
> - * No need to exit to userspace if we already have an interrupt queued.
> - */
>  static int kvm_vcpu_ready_for_interrupt_injection(struct kvm_vcpu *vcpu)
>  {
>         return kvm_arch_interrupt_allowed(vcpu) &&
> -               !kvm_cpu_has_interrupt(vcpu) &&
> -               !kvm_event_needs_reinjection(vcpu) &&
>                 kvm_cpu_accept_dm_intr(vcpu);
>  }
>
> --
> 2.28.0
>