On Fri, Nov 27, 2020 at 7:26 PM Paolo Bonzini pbonzini@redhat.com wrote:
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, the complex conditional is really the correct implementation of kvm_cpu_accept_dm_intr(vcpu). 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.
Hello, Paolo
If userspace does KVM_INTERRUPT, vcpu->arch.interrupt.injected is set immediately, and in inject_pending_event(), we have
else if (!vcpu->arch.exception.pending) { if (vcpu->arch.nmi_injected) { kvm_x86_ops.set_nmi(vcpu); can_inject = false; } else if (vcpu->arch.interrupt.injected) { kvm_x86_ops.set_irq(vcpu); can_inject = false; } }
I'm curious about that can the kvm_x86_ops.set_irq() here be possible to queue the irq with EFLAGS.IF=0? If not, which code prevents it?
I'm asking about this because I just noticed that interrupt can be queued when exception pending, and this patch relaxed it even more.
Note: interrupt can NOT be queued when exception pending until 664f8e26b00c7 ("KVM: X86: Fix loss of exception which has not yet been injected") which I think is dangerous.
Thanks Lai
- 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
arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/irq.c | 2 +- arch/x86/kvm/x86.c | 17 +++++++---------- 3 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index d44858b69353..ddaf3e01a854 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1655,6 +1655,7 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva); int 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); diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c index e2d49a506e7f..fa01f07e449e 100644 --- a/arch/x86/kvm/irq.c +++ b/arch/x86/kvm/irq.c @@ -40,7 +40,7 @@ static int pending_userspace_extint(struct kvm_vcpu *v)
- 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 that it's diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 447edc0d1d5a..54124b6211df 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4051,21 +4051,22 @@ 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));
/** 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);}
-- 2.28.0