ACK: [B][PATCH 1/1] KVM: x86: handle !lapic_in_kernel case in kvm_cpu_*_extint

Thu Mar 4 10:52:19 UTC 2021

On Wed, Mar 03, 2021 at 06:33:48PM -0300, Guilherme G. Piccoli wrote:
> From: Paolo Bonzini <pbonzini at redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1917138
> 
> Centralize handling of interrupts from the userspace APIC
> in kvm_cpu_has_extint and kvm_cpu_get_extint, since
> userspace APIC interrupts are handled more or less the
> same as ExtINTs are with split irqchip.  This removes
> duplicated code from kvm_cpu_has_injectable_intr and
> kvm_cpu_has_interrupt, and makes the code more similar
> between kvm_cpu_has_{extint,interrupt} on one side
> and kvm_cpu_get_{extint,interrupt} on the other.
> 
> Cc: stable at vger.kernel.org
> Reviewed-by: Filippo Sironi <sironi at amazon.de>
> Reviewed-by: David Woodhouse <dwmw at amazon.co.uk>
> Tested-by: David Woodhouse <dwmw at amazon.co.uk>
> Signed-off-by: Paolo Bonzini <pbonzini at redhat.com>
> (backported from commit 72c3bcdcda494cbd600712a32e67702cdee60c07)
> 
> [gpiccoli: Besides context adjustments, it's very important to notice
> that Ubuntu 4.15 tree lacks the following commit, which renames the
> struct member interrupt.pending to interrupt.injected:
> 04140b4144cd ("KVM: x86: Rename interrupt.pending to interrupt.injected")
> This may cause a very confusing comment in kvm_cpu_has_extint(),
> hence I've changed both code and comment in order they do make sense.]
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at canonical.com>
> ---
>  arch/x86/kvm/irq.c   | 65 ++++++++++++++++++++++++--------------------
>  arch/x86/kvm/lapic.c |  2 +-
>  2 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 8978988e735d..842fcc043a8f 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -54,15 +54,29 @@ static int pending_userspace_extint(struct kvm_vcpu *v)
>   */
>  int kvm_cpu_has_extint(struct kvm_vcpu *v)
>  {
> -	u8 accept = kvm_apic_accept_pic_intr(v);
> +	/*
> +	 * FIXME: interrupt.pending represents an interrupt whose
> +	 * side-effects have already been applied (e.g. bit from IRR
> +	 * already moved to ISR). Therefore, it is incorrect to rely
> +	 * on interrupt.pending to know if there is a pending
> +	 * interrupt in the user-mode LAPIC.
> +	 * This leads to nVMX/nSVM not be able to distinguish
> +	 * if it should exit from L2 to L1 on EXTERNAL_INTERRUPT on
> +	 * pending interrupt or should re-inject an injected
> +	 * interrupt.
> +	 * [backport note: interrupt.pending was renamed interrupt.injected
> +	 * in upstream commit 04140b4144cd , not present in this tree.]
> +	 */

The comment really comes from 04140b4144cd. Which explains how the backport is
not that easy to compare with the upstream version. Applying the backport and
comparing to 71cc849b7093 shows really well the differences there and I am
happy with the backport as it is.

I appreciate all the writeup as well too.
Cascardo.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>

> +	if (!lapic_in_kernel(v))
> +		return v->arch.interrupt.pending;
>  
> -	if (accept) {
> -		if (irqchip_split(v->kvm))
> -			return pending_userspace_extint(v);
> -		else
> -			return v->kvm->arch.vpic->output;
> -	} else
> +	if (!kvm_apic_accept_pic_intr(v))
>  		return 0;
> +
> +	if (irqchip_split(v->kvm))
> +		return pending_userspace_extint(v);
> +	else
> +		return v->kvm->arch.vpic->output;
>  }
>  
>  /*
> @@ -73,9 +87,6 @@ int kvm_cpu_has_extint(struct kvm_vcpu *v)
>   */
>  int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>  {
> -	if (!lapic_in_kernel(v))
> -		return v->arch.interrupt.pending;
> -
>  	if (kvm_cpu_has_extint(v))
>  		return 1;
>  
> @@ -91,9 +102,6 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>   */
>  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>  {
> -	if (!lapic_in_kernel(v))
> -		return v->arch.interrupt.pending;
> -
>  	if (kvm_cpu_has_extint(v))
>  		return 1;
>  
> @@ -107,16 +115,21 @@ EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
>   */
>  static int kvm_cpu_get_extint(struct kvm_vcpu *v)
>  {
> -	if (kvm_cpu_has_extint(v)) {
> -		if (irqchip_split(v->kvm)) {
> -			int vector = v->arch.pending_external_vector;
> -
> -			v->arch.pending_external_vector = -1;
> -			return vector;
> -		} else
> -			return kvm_pic_read_irq(v->kvm); /* PIC */
> -	} else
> +	if (!kvm_cpu_has_extint(v)) {
> +		WARN_ON(!lapic_in_kernel(v));
>  		return -1;
> +	}
> +
> +	if (!lapic_in_kernel(v))
> +		return v->arch.interrupt.nr;
> +
> +	if (irqchip_split(v->kvm)) {
> +		int vector = v->arch.pending_external_vector;
> +
> +		v->arch.pending_external_vector = -1;
> +		return vector;
> +	} else
> +		return kvm_pic_read_irq(v->kvm); /* PIC */
>  }
>  
>  /*
> @@ -124,13 +137,7 @@ static int kvm_cpu_get_extint(struct kvm_vcpu *v)
>   */
>  int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
>  {
> -	int vector;
> -
> -	if (!lapic_in_kernel(v))
> -		return v->arch.interrupt.nr;
> -
> -	vector = kvm_cpu_get_extint(v);
> -
> +	int vector = kvm_cpu_get_extint(v);
>  	if (vector != -1)
>  		return vector;			/* PIC */
>  
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index e1116584105d..a6084e5750ab 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2217,7 +2217,7 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	u32 ppr;
>  
> -	if (!kvm_apic_hw_enabled(apic))
> +	if (!kvm_apic_present(vcpu))
>  		return -1;
>  
>  	__apic_update_ppr(apic, &ppr);
> -- 
> 2.29.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team