Re: [RFC PATCH] Fix split-irqchip vs interrupt injection window request.

From: Sean Christopherson <seanjc@google.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: kvm <kvm@vger.kernel.org>, "Sironi, Filippo" <sironi@amazon.de>,
	"Raslan, KarimAllah" <karahmed@amazon.de>,
	Matt Gingell <gingell@google.com>,
	Steve Rutherford <srutherford@google.com>,
	liran@amazon.com
Subject: Re: [RFC PATCH] Fix split-irqchip vs interrupt injection window request.
Date: Wed, 25 Nov 2020 21:19:55 +0000	[thread overview]
Message-ID: <20201125211955.GA450871@google.com> (raw)
In-Reply-To: <017de9019136b5d2ec34132b96b9f0273c21d6f1.camel@infradead.org>

On Wed, Nov 25, 2020, David Woodhouse wrote:
> On Thu, 2020-11-12 at 13:03 +0000, David Woodhouse wrote:
> > I'm using nested VMX for testing, while I add split-irqchip support to
> > my VMM. I see the vCPU lock up when attempting to deliver an interrupt.
> 
> Turns out I don't need nesting or my own VMM to reproduce this; all I
> need to do is boot a guest in qemu with split-irqchip and 'noapic' on
> the guest command line. It locks up before getting to a login prompt,
> every time.
> 
> qemu-system-x86_64 -serial mon:stdio -machine q35,accel=kvm,kernel-irqchip=split -m 2G -display none -drive file=foo.qcow2,if=virtio
> 
> Commit 782d422bc ("KVM: x86: split kvm_vcpu_ready_for_interrupt_injection
> out of dm_request_for_irq_injection") made dm_request_for_irq_injection()
> return true even when kvm_cpu_has_interrupt() is true.
> 
> So we enable the vmexit on interrupt window because userspace asked for
> it, but then kvm_vcpu_ready_for_interrupt_injection() returns false,
> causing us *not* to exit all the way to userspace but just to loop in
> vcpu_run() instead.
> 
> But we *didn't* have an injectable interrupt from the kernel, so we
> just go straight back into the guest, vmexit again, loop again, ad
> infinitum.
> 
> This appears to fix it:
> 
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4028,7 +4028,7 @@ static int kvm_cpu_accept_dm_intr(struct kvm_vcpu *vcpu)
>  static int kvm_vcpu_ready_for_interrupt_injection(struct kvm_vcpu *vcpu)
>  {
>         return kvm_arch_interrupt_allowed(vcpu) &&
> -               !kvm_cpu_has_interrupt(vcpu) &&
> +               !kvm_cpu_has_injectable_intr(vcpu) &&

Interrupt window exiting explicitly has priority over virtual interrupt delivery,
so this is correct from that perspective.

But I wonder if would make sense to be more precise so that KVM behavior is
consistent for APICv and legacy IRQ injection.  If the LAPIC is in-kernel,
KVM_INTERRUPT can only be used for EXTINT; whether or not there's an IRQ in the
LAPIC should be irrelevant when deciding to exit to userspace.  Note, the
reinjection check covers vcpu->arch.interrupt.injected for the case where LAPIC
is in userspace.

	return kvm_arch_interrupt_allowed(vcpu) &&
	       (!lapic_in_kernel(vcpu) || !kvm_cpu_has_extint(vcpu)) &&
	       !kvm_event_needs_reinjection(vcpu) &&
	       kvm_cpu_accept_dm_intr(vcpu);
}

>                 !kvm_event_needs_reinjection(vcpu) &&
>                 kvm_cpu_accept_dm_intr(vcpu);
>  }
> 