diff mbox series

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

Message ID 20210303213348.31319-2-gpiccoli@canonical.com
State New
Headers show
Series Bionic kernel 4.15.0-136 causes VMM freezes due to lack of KVM patch | expand

Commit Message

Guilherme G. Piccoli March 3, 2021, 9:33 p.m. UTC
From: Paolo Bonzini <pbonzini@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@vger.kernel.org
Reviewed-by: Filippo Sironi <sironi@amazon.de>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Paolo Bonzini <pbonzini@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@canonical.com>
---
 arch/x86/kvm/irq.c   | 65 ++++++++++++++++++++++++--------------------
 arch/x86/kvm/lapic.c |  2 +-
 2 files changed, 37 insertions(+), 30 deletions(-)

Comments

Stefan Bader March 4, 2021, 9:45 a.m. UTC | #1
On 03.03.21 22:33, Guilherme G. Piccoli wrote:
> From: Paolo Bonzini <pbonzini@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@vger.kernel.org
> Reviewed-by: Filippo Sironi <sironi@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Tested-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Paolo Bonzini <pbonzini@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@canonical.com>
Acked-by: Stefan Bader <stefan.bader@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);
>
Thadeu Lima de Souza Cascardo March 4, 2021, 10:52 a.m. UTC | #2
On Wed, Mar 03, 2021 at 06:33:48PM -0300, Guilherme G. Piccoli wrote:
> From: Paolo Bonzini <pbonzini@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@vger.kernel.org
> Reviewed-by: Filippo Sironi <sironi@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Tested-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Paolo Bonzini <pbonzini@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@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@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@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kleber Souza March 4, 2021, 10:56 a.m. UTC | #3
On 03.03.21 22:33, Guilherme G. Piccoli wrote:
> From: Paolo Bonzini <pbonzini@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@vger.kernel.org
> Reviewed-by: Filippo Sironi <sironi@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Tested-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Paolo Bonzini <pbonzini@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@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@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.]
> +	 */
> +	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);
>
Guilherme G. Piccoli March 4, 2021, 12:59 p.m. UTC | #4
On Thu, Mar 4, 2021 at 6:45 AM Stefan Bader <stefan.bader@canonical.com> wrote:
> [...]
> 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

Thanks Stefan, Cascardo and Kleber for the ACKs!
I agree, it's a Bionic-only issue, I forgot to add the nomination,
thanks for that.

I suggest we merge this patch in the tree as soon as possible, it's
not urgent but we can grab it if, for some reason, a Bionic respin
happens.
Cheers,

Guilherme
Stefan Bader March 5, 2021, 9:44 a.m. UTC | #5
On 03.03.21 22:33, Guilherme G. Piccoli wrote:
> From: Paolo Bonzini <pbonzini@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@vger.kernel.org
> Reviewed-by: Filippo Sironi <sironi@amazon.de>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Tested-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Paolo Bonzini <pbonzini@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@canonical.com>
> ---

Applied to bionic:linux/master-next. Thanks.

-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);
>
diff mbox series

Patch

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