[PATCH v2 3/4] KVM: x86: request interrupt window when IRQ chip is split

[PATCH v2 3/4] KVM: x86: request interrupt window when IRQ chip is split

[Matt Gingell]

Before this patch, we incorrectly enter the guest without requesting an
interrupt window if the IRQ chip is split between user space and the
kernel.

Because lapic_in_kernel no longer implies the PIC is in the kernel, this
patch tests pic_in_kernel to determining whether an interrupt window
should be requested when entering the guest.

If the APIC is in the kernel and we request an interrupt window the
guest will return immediately. If the APIC is masked the guest will not
not make forward progress and unmask it, leading to a loop when KVM
reenters and requests again. This patch adds a check to ensure the APIC
is ready to accept an interrupt before requesting a window.

Reviewed-by: Steve Rutherford <srutherford@google.com>
Signed-off-by: Matt Gingell <gingell@google.com>
---
 arch/x86/kvm/x86.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c370eef..d57bdd9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6259,8 +6259,11 @@ void kvm_arch_mmu_notifier_invalidate_page(struct kvm *kvm,
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;
-	bool req_int_win = !lapic_in_kernel(vcpu) &&
-		vcpu->run->request_interrupt_window;
+	bool req_int_win =
+		vcpu->run->request_interrupt_window &&
+		likely(!pic_in_kernel(vcpu->kvm)) &&
+		(!lapic_in_kernel(vcpu) || kvm_apic_accept_pic_intr(vcpu));
+
 	bool req_immediate_exit = false;
 
 	if (vcpu->requests) {
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v2 3/4] KVM: x86: request interrupt window when IRQ chip is split

[Paolo Bonzini]

On 17/11/2015 00:26, Matt Gingell wrote:
> Before this patch, we incorrectly enter the guest without requesting an
> interrupt window if the IRQ chip is split between user space and the
> kernel.
> 
> Because lapic_in_kernel no longer implies the PIC is in the kernel, this
> patch tests pic_in_kernel to determining whether an interrupt window
> should be requested when entering the guest.
> 
> If the APIC is in the kernel and we request an interrupt window the
> guest will return immediately. If the APIC is masked the guest will not
> not make forward progress and unmask it, leading to a loop when KVM
> reenters and requests again. This patch adds a check to ensure the APIC
> is ready to accept an interrupt before requesting a window.
> 
> Reviewed-by: Steve Rutherford <srutherford@google.com>
> Signed-off-by: Matt Gingell <gingell@google.com>
> ---
>  arch/x86/kvm/x86.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c370eef..d57bdd9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6259,8 +6259,11 @@ void kvm_arch_mmu_notifier_invalidate_page(struct kvm *kvm,
>  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  {
>  	int r;
> -	bool req_int_win = !lapic_in_kernel(vcpu) &&
> -		vcpu->run->request_interrupt_window;
> +	bool req_int_win =
> +		vcpu->run->request_interrupt_window &&
> +		likely(!pic_in_kernel(vcpu->kvm)) &&
> +		(!lapic_in_kernel(vcpu) || kvm_apic_accept_pic_intr(vcpu));
> +

This can be

        bool req_int_win =
                dm_request_for_irq_injection(vcpu) &&
                (!lapic_in_kernel(vcpu) || kvm_apic_accept_pic_intr(vcpu));

I'll apply the patches and send them to Linus for 4.4-rc2, thanks!
These cleanups can go on top:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2e16068bba51..0bca1ec199df 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2661,12 +2661,24 @@ static int kvm_vcpu_ioctl_set_lapic(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static int kvm_vcpu_ready_for_interrupt_injection(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));
+}
+
+/*
+ * 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) &&
-		(!lapic_in_kernel(vcpu) ||
-		 kvm_apic_accept_pic_intr(vcpu));
+		kvm_cpu_accept_dm_intr(vcpu);
 }
 
 static int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu,
@@ -5817,12 +5829,6 @@ static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
 	return emulator_write_emulated(ctxt, rip, instruction, 3, NULL);
 }
 
-/*
- * Check if userspace requested an interrupt window, and that the
- * interrupt window is open.
- *
- * No need to exit to userspace if we already have an interrupt queued.
- */
 static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu)
 {
 	return vcpu->run->request_interrupt_window &&
@@ -6253,9 +6259,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;
 	bool req_int_win =
-		vcpu->run->request_interrupt_window &&
-		likely(!pic_in_kernel(vcpu->kvm)) &&
-		(!lapic_in_kernel(vcpu) || kvm_apic_accept_pic_intr(vcpu));
+		dm_request_for_irq_injection(vcpu) &&
+		kvm_cpu_accept_dm_intr(vcpu);
 
 	bool req_immediate_exit = false;
 

A couple questions, that can be fixed by separate patches:

- should kvm_cpu_accept_dm_intr check that pending_external_vector == -1?

- should kvm_vcpu_ioctl_interrupt then use kvm_cpu_accept_dm_intr
instead of checking pending_external_vector == -1 directly?

Paolo