ACK/Cmnt: [[SCRIPT=remove_re|Re: [B][PATCH 1/1] KVM: x86: handle !lapic_in_kernel case in kvm_cpu_*_extint]]

[Stefan Bader]

On 03.03.21 22:33, 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>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---

The patch looks consistent and there is good testing results in the bug report. 
I just modified the report to have a bionic nomination and marked the devel task 
invalid as the comments suggest that this only affects bionic.

-Stefan

>   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.]
> +	 */
> +	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);
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210304/34ea6791/attachment-0001.sig>