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

[Paolo Bonzini <pbonzini@xxxxxxxxxx>]

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