[PATCH 3/3] KVM: x86: fix ready_for_interrupt reporting in split IRQ chip case

[PATCH 3/3] KVM: x86: fix ready_for_interrupt reporting in split IRQ chip case

[Matt Gingell]

This patch adds a call to kvm_arch_interrupt_allowed to ensure ready for
interrupt is reported to user space correctly. This addresses a problem
observed in QEMU when kvm->ready_for_interrupt is set but the x86
interrupt flag is clear.

Additionally, test that the APIC is ready to accept an interrupt before
reporting we are ready for injection.

Reviewed-by: Andy Honig <ahonig@google.com>
Signed-off-by: Matt Gingell <gingell@google.com>
---
 arch/x86/kvm/x86.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fd64dee..962003b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5835,15 +5835,13 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu)
 	kvm_run->flags = is_smm(vcpu) ? KVM_RUN_X86_SMM : 0;
 	kvm_run->cr8 = kvm_get_cr8(vcpu);
 	kvm_run->apic_base = kvm_get_apic_base(vcpu);
-	if (!irqchip_in_kernel(vcpu->kvm))
+	if (!pic_in_kernel(vcpu->kvm))
 		kvm_run->ready_for_interrupt_injection =
 			kvm_arch_interrupt_allowed(vcpu) &&
 			!kvm_cpu_has_interrupt(vcpu) &&
-			!kvm_event_needs_reinjection(vcpu);
-	else if (!pic_in_kernel(vcpu->kvm))
-		kvm_run->ready_for_interrupt_injection =
-			kvm_apic_accept_pic_intr(vcpu) &&
-			!kvm_cpu_has_interrupt(vcpu);
+			!kvm_event_needs_reinjection(vcpu) &&
+			(!lapic_in_kernel(vcpu) ||
+			 kvm_apic_accept_pic_intr(vcpu));
 	else
 		kvm_run->ready_for_interrupt_injection = 1;
 }
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 3/3] KVM: x86: fix ready_for_interrupt reporting in split IRQ chip case

[Paolo Bonzini]

On 12/11/2015 20:07, Matt Gingell wrote:
> This patch adds a call to kvm_arch_interrupt_allowed to ensure ready for
> interrupt is reported to user space correctly. This addresses a problem
> observed in QEMU when kvm->ready_for_interrupt is set but the x86
> interrupt flag is clear.
> 
> Additionally, test that the APIC is ready to accept an interrupt before
> reporting we are ready for injection.
> 
> Reviewed-by: Andy Honig <ahonig@google.com>
> Signed-off-by: Matt Gingell <gingell@google.com>

I think you need to add the same call to dm_request_for_irq_injection, like

-	return (irqchip_split(vcpu->kvm)
-		? kvm_apic_accept_pic_intr(vcpu)
-		: kvm_arch_interrupt_allowed(vcpu));
+	if (!kvm_arch_interrupt_allowed(vcpu))
+		return false;
+
+	return !lapic_in_kernel(vcpu) || kvm_apic_accept_pic_intr(vcpu);

At this point, just to err on the safe side, we probably should test
kvm_event_needs_reinjection(vcpu) as well in dm_request_for_irq_injection.

We can then make a new function kvm_vcpu_ready_for_interrupt_injection
with the sequence of tests (kvm_cpu_has_interrupt,
kvm_arch_interrupt_allowed, kvm_event_needs_reinjection, possibly
kvm_apic_accept_pic_intr) so that:

- dm_request_for_irq_injection becomes simply

	return (vcpu->run->request_interrupt_window &&
		likely(!pic_in_kernel(vcpu->kvm));

- the caller of dm_request_for_irq_injection does

	if (dm_request_for_irq_injection(vcpu) &&
	    kvm_vcpu_ready_for_interrupt_injection(vcpu))

- post_kvm_run_save's assignment becomes

	kvm_run->ready_for_interrupt_injection =
		!pic_in_kernel(vcpu->kvm) ||
		kvm_vcpu_ready_for_interrupt_injection(vcpu);

The code would make a lot of sense then; I hope it will work too. :)

Paolo "ceci n'est pas une patch"

Re: [PATCH 3/3] KVM: x86: fix ready_for_interrupt reporting in split IRQ chip case

[Steve Rutherford]

On Fri, Nov 13, 2015 at 12:52 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 12/11/2015 20:07, Matt Gingell wrote:
>> This patch adds a call to kvm_arch_interrupt_allowed to ensure ready for
>> interrupt is reported to user space correctly. This addresses a problem
>> observed in QEMU when kvm->ready_for_interrupt is set but the x86
>> interrupt flag is clear.
>>
>> Additionally, test that the APIC is ready to accept an interrupt before
>> reporting we are ready for injection.
>>
>> Reviewed-by: Andy Honig <ahonig@google.com>
>> Signed-off-by: Matt Gingell <gingell@google.com>
>
> I think you need to add the same call to dm_request_for_irq_injection, like
>
> -       return (irqchip_split(vcpu->kvm)
> -               ? kvm_apic_accept_pic_intr(vcpu)
> -               : kvm_arch_interrupt_allowed(vcpu));
> +       if (!kvm_arch_interrupt_allowed(vcpu))
> +               return false;
> +
> +       return !lapic_in_kernel(vcpu) || kvm_apic_accept_pic_intr(vcpu);
>
> At this point, just to err on the safe side, we probably should test
> kvm_event_needs_reinjection(vcpu) as well in dm_request_for_irq_injection.
This is definitely necessary. Without it, it's possible to bounce back
and forth between userspace and the kernel.

(Actually ran into this in testing yesterday evening. If you ever want
to stress test legacy interrupt handling devices, try booting Plan9)
>
> We can then make a new function kvm_vcpu_ready_for_interrupt_injection
> with the sequence of tests (kvm_cpu_has_interrupt,
> kvm_arch_interrupt_allowed, kvm_event_needs_reinjection, possibly
> kvm_apic_accept_pic_intr) so that:
>
> - dm_request_for_irq_injection becomes simply
>
>         return (vcpu->run->request_interrupt_window &&
>                 likely(!pic_in_kernel(vcpu->kvm));
>
> - the caller of dm_request_for_irq_injection does
>
>         if (dm_request_for_irq_injection(vcpu) &&
>             kvm_vcpu_ready_for_interrupt_injection(vcpu))
>
> - post_kvm_run_save's assignment becomes
>
>         kvm_run->ready_for_interrupt_injection =
>                 !pic_in_kernel(vcpu->kvm) ||
>                 kvm_vcpu_ready_for_interrupt_injection(vcpu);
>
> The code would make a lot of sense then; I hope it will work too. :)
>
> Paolo "ceci n'est pas une patch"