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

From: Paolo Bonzini <pbonzini@redhat.com>
To: David Woodhouse <dwmw2@infradead.org>,
	Sean Christopherson <seanjc@google.com>,
	"Borghorst, Hendrik" <hborghor@amazon.com>
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: Fri, 27 Nov 2020 05:37:43 +0100	[thread overview]
Message-ID: <49d8ac07-1745-e2af-a3a2-a0d8010c3914@redhat.com> (raw)
In-Reply-To: <6e7060415fe321a3969a76330b643116a5ab44d1.camel@infradead.org>

On 26/11/20 22:48, David Woodhouse wrote:
> Although I do kind of like the symmetry of my original version using
> kvm_cpu_has_injectable_intr(), which is the condition used in
> vcpu_enter_guest() for enabling the interrupt window vmexit in the
> first place. It makes sense for those to match.

In inject_pending_event, actually.

However there's also an interrupt window request in vcpu_enter_guest():

         bool req_int_win =
                 dm_request_for_irq_injection(vcpu) &&
                 kvm_cpu_accept_dm_intr(vcpu);

and this one definitely should indeed stay in sync with 
kvm_vcpu_ready_for_interrupt_injection.  This gives an even neater 
version of the patch:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 447edc0d1d5a..a05a2be05552 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4052,7 +4052,8 @@ 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));
+		(kvm_apic_accept_pic_intr(vcpu)
+		 && !pending_userspace_extint(vcpu));
  }

  /*
@@ -4064,7 +4065,6 @@ 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_event_needs_reinjection(vcpu) &&
  		kvm_cpu_accept_dm_intr(vcpu);
  }

or even better:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 447edc0d1d5a..adbb519eece4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4051,8 +4051,10 @@ 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));
+	if (lapic_in_kernel(vcpu))
+		return !v->arch.interrupt.injected;
+
+	return !kvm_cpu_has_extint(vcpu);
  }

  /*
@@ -4064,8 +4066,6 @@ 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_event_needs_reinjection(vcpu) &&
  		kvm_cpu_accept_dm_intr(vcpu);
  }


since the call to kvm_event_needs_reinjection(vcpu) isn't really needed 
(maybe it was when Matt sent his original patches, but since then 
inject_pending_event has seen a significant overhaul).

Now this second possibility is very similar to Sean's suggestion, but 
it's actually code that I can understand.

> We enable the irq window if kvm_cpu_has_injectable_intr() or if
> userspace asks. And when the exit happens, we feed it to userspace
> unless kvm_cpu_has_injectable_intr().

What I don't like about it is that kvm_cpu_has_injectable_intr() has the 
more complicated handling of APIC interrupts.  By definition they don't 
matter here, we're considering whether to exit to userspace.

Paolo

