Re: [PATCH 4.19 08/57] KVM: x86: Fix split-irqchip vs interrupt injection window request

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/12/20 09:53, Greg Kroah-Hartman wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> commit 71cc849b7093bb83af966c0e60cb11b7f35cd746 upstream.

Since you did not apply "KVM: x86: handle !lapic_in_kernel case in 
kvm_cpu_*_extint" to 4.19 or earlier, please leave this one out as well.

Thanks,

Paolo

> 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);
>   }
>   
> 
>