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

[Paolo Bonzini <pbonzini@redhat.com>]

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