| From 71cc849b7093bb83af966c0e60cb11b7f35cd746 Mon Sep 17 00:00:00 2001 |
| From: Paolo Bonzini <pbonzini@redhat.com> |
| Date: Fri, 27 Nov 2020 09:18:20 +0100 |
| Subject: KVM: x86: Fix split-irqchip vs interrupt injection window request |
| |
| From: Paolo Bonzini <pbonzini@redhat.com> |
| |
| commit 71cc849b7093bb83af966c0e60cb11b7f35cd746 upstream. |
| |
| kvm_cpu_accept_dm_intr and kvm_vcpu_ready_for_interrupt_injection are |
| a hodge-podge of conditions, hacked together to get something that |
| more or less works. But what is actually needed is much simpler; |
| in both cases the fundamental question is, do we have a place to stash |
| an interrupt if userspace does KVM_INTERRUPT? |
| |
| In userspace irqchip mode, that is !vcpu->arch.interrupt.injected. |
| Currently kvm_event_needs_reinjection(vcpu) covers it, but it is |
| unnecessarily restrictive. |
| |
| In split irqchip mode it's a bit more complicated, we need to check |
| kvm_apic_accept_pic_intr(vcpu) (the IRQ window exit is basically an INTACK |
| cycle and thus requires ExtINTs not to be masked) as well as |
| !pending_userspace_extint(vcpu). However, there is no need to |
| check kvm_event_needs_reinjection(vcpu), since split irqchip keeps |
| pending ExtINT state separate from event injection state, and checking |
| kvm_cpu_has_interrupt(vcpu) is wrong too since ExtINT has higher |
| priority than APIC interrupts. In fact the latter fixes a bug: |
| when userspace requests an IRQ window vmexit, an interrupt in the |
| local APIC can cause kvm_cpu_has_interrupt() to be true and thus |
| kvm_vcpu_ready_for_interrupt_injection() to return false. When this |
| happens, vcpu_run does not exit to userspace but the interrupt window |
| vmexits keep occurring. The VM loops without any hope of making progress. |
| |
| Once we try to fix these with something like |
| |
| return kvm_arch_interrupt_allowed(vcpu) && |
| - !kvm_cpu_has_interrupt(vcpu) && |
| - !kvm_event_needs_reinjection(vcpu) && |
| - kvm_cpu_accept_dm_intr(vcpu); |
| + (!lapic_in_kernel(vcpu) |
| + ? !vcpu->arch.interrupt.injected |
| + : (kvm_apic_accept_pic_intr(vcpu) |
| + && !pending_userspace_extint(v))); |
| |
| we realize two things. First, thanks to the previous patch the complex |
| conditional can reuse !kvm_cpu_has_extint(vcpu). Second, the interrupt |
| window request in vcpu_enter_guest() |
| |
| bool req_int_win = |
| dm_request_for_irq_injection(vcpu) && |
| kvm_cpu_accept_dm_intr(vcpu); |
| |
| should be kept in sync with kvm_vcpu_ready_for_interrupt_injection(): |
| it is unnecessary to ask the processor for an interrupt window |
| if we would not be able to return to userspace. Therefore, |
| kvm_cpu_accept_dm_intr(vcpu) is basically !kvm_cpu_has_extint(vcpu) |
| ANDed with the existing check for masked ExtINT. It all makes sense: |
| |
| - we can accept an interrupt from userspace if there is a place |
| to stash it (and, for irqchip split, ExtINTs are not masked). |
| Interrupts from userspace _can_ be accepted even if right now |
| EFLAGS.IF=0. |
| |
| - in order to tell userspace we will inject its interrupt ("IRQ |
| window open" i.e. kvm_vcpu_ready_for_interrupt_injection), both |
| KVM and the vCPU need to be ready to accept the interrupt. |
| |
| ... and this is what the patch implements. |
| |
| Reported-by: David Woodhouse <dwmw@amazon.co.uk> |
| Analyzed-by: David Woodhouse <dwmw@amazon.co.uk> |
| Cc: stable@vger.kernel.org |
| Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
| Reviewed-by: Nikos Tsironis <ntsironis@arrikto.com> |
| Reviewed-by: David Woodhouse <dwmw@amazon.co.uk> |
| Tested-by: David Woodhouse <dwmw@amazon.co.uk> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| arch/x86/include/asm/kvm_host.h | 1 + |
| arch/x86/kvm/irq.c | 2 +- |
| arch/x86/kvm/x86.c | 18 ++++++++++-------- |
| 3 files changed, 12 insertions(+), 9 deletions(-) |
| |
| --- a/arch/x86/include/asm/kvm_host.h |
| +++ b/arch/x86/include/asm/kvm_host.h |
| @@ -1472,6 +1472,7 @@ int kvm_test_age_hva(struct kvm *kvm, un |
| void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte); |
| int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v); |
| int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu); |
| +int kvm_cpu_has_extint(struct kvm_vcpu *v); |
| int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu); |
| int kvm_cpu_get_interrupt(struct kvm_vcpu *v); |
| void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event); |
| --- a/arch/x86/kvm/irq.c |
| +++ b/arch/x86/kvm/irq.c |
| @@ -52,7 +52,7 @@ static int pending_userspace_extint(stru |
| * check if there is pending interrupt from |
| * non-APIC source without intack. |
| */ |
| -static int kvm_cpu_has_extint(struct kvm_vcpu *v) |
| +int kvm_cpu_has_extint(struct kvm_vcpu *v) |
| { |
| /* |
| * FIXME: interrupt.injected represents an interrupt whose |
| --- a/arch/x86/kvm/x86.c |
| +++ b/arch/x86/kvm/x86.c |
| @@ -3351,21 +3351,23 @@ static int kvm_vcpu_ioctl_set_lapic(stru |
| |
| static int kvm_cpu_accept_dm_intr(struct kvm_vcpu *vcpu) |
| { |
| + /* |
| + * We can accept userspace's request for interrupt injection |
| + * as long as we have a place to store the interrupt number. |
| + * The actual injection will happen when the CPU is able to |
| + * deliver the interrupt. |
| + */ |
| + if (kvm_cpu_has_extint(vcpu)) |
| + return false; |
| + |
| + /* Acknowledging ExtINT does not happen if LINT0 is masked. */ |
| 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) && |
| kvm_cpu_accept_dm_intr(vcpu); |
| } |
| |
