[1/2] KVM: x86: handle !lapic_in_kernel case in kvm_cpu_*_extint
diff mbox series

Message ID 20201127112114.3219360-2-pbonzini@redhat.com
State New, archived
Headers show
Series
  • KVM: x86: cleanup and fix userspace interrupt window
Related show

Commit Message

Paolo Bonzini Nov. 27, 2020, 11:21 a.m. UTC
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
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/irq.c   | 85 ++++++++++++++++++--------------------------
 arch/x86/kvm/lapic.c |  2 +-
 2 files changed, 35 insertions(+), 52 deletions(-)

Comments

David Woodhouse Nov. 27, 2020, 11:56 a.m. UTC | #1
On Fri, 2020-11-27 at 06:21 -0500, Paolo Bonzini wrote:
> +        * FIXME: interrupt.injected represents an interrupt that it's

You can drop the stray apostrophe from that "its" while you're moving
it...
Filippo Sironi Nov. 27, 2020, 12:52 p.m. UTC | #2
On 11/27/20 12:21 PM, Paolo Bonzini wrote:
> 
> 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
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/irq.c   | 85 ++++++++++++++++++--------------------------
>   arch/x86/kvm/lapic.c |  2 +-
>   2 files changed, 35 insertions(+), 52 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 99d118ffc67d..e2d49a506e7f 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -42,15 +42,27 @@ static int pending_userspace_extint(struct kvm_vcpu *v)
>    */
>   static int kvm_cpu_has_extint(struct kvm_vcpu *v)
>   {
> -       u8 accept = kvm_apic_accept_pic_intr(v);
> +       /*
> +        * FIXME: interrupt.injected represents an interrupt that it's
> +        * side-effects have already been applied (e.g. bit from IRR
> +        * already moved to ISR). Therefore, it is incorrect to rely
> +        * on interrupt.injected 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.
> +        */
> +       if (!lapic_in_kernel(v))
> +               return v->arch.interrupt.injected;
> 
> -       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;
>   }
> 
>   /*
> @@ -61,20 +73,6 @@ static int kvm_cpu_has_extint(struct kvm_vcpu *v)
>    */
>   int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>   {
> -       /*
> -        * FIXME: interrupt.injected represents an interrupt that it's
> -        * side-effects have already been applied (e.g. bit from IRR
> -        * already moved to ISR). Therefore, it is incorrect to rely
> -        * on interrupt.injected 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.
> -        */
> -       if (!lapic_in_kernel(v))
> -               return v->arch.interrupt.injected;
> -
>          if (kvm_cpu_has_extint(v))
>                  return 1;
> 
> @@ -91,20 +89,6 @@ EXPORT_SYMBOL_GPL(kvm_cpu_has_injectable_intr);
>    */
>   int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>   {
> -       /*
> -        * FIXME: interrupt.injected represents an interrupt that it's
> -        * side-effects have already been applied (e.g. bit from IRR
> -        * already moved to ISR). Therefore, it is incorrect to rely
> -        * on interrupt.injected 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.
> -        */
> -       if (!lapic_in_kernel(v))
> -               return v->arch.interrupt.injected;
> -
>          if (kvm_cpu_has_extint(v))
>                  return 1;
> 
> @@ -118,16 +102,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 */
>   }
> 
>   /*
> @@ -135,13 +124,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 105e7859d1f2..bb5ff761d5e2 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2465,7 +2465,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.28.0
> 
> 

Reviewed-by: Filippo Sironi <sironi@amazon.de>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Patch
diff mbox series

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 99d118ffc67d..e2d49a506e7f 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -42,15 +42,27 @@  static int pending_userspace_extint(struct kvm_vcpu *v)
  */
 static int kvm_cpu_has_extint(struct kvm_vcpu *v)
 {
-	u8 accept = kvm_apic_accept_pic_intr(v);
+	/*
+	 * FIXME: interrupt.injected represents an interrupt that it's
+	 * side-effects have already been applied (e.g. bit from IRR
+	 * already moved to ISR). Therefore, it is incorrect to rely
+	 * on interrupt.injected 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.
+	 */
+	if (!lapic_in_kernel(v))
+		return v->arch.interrupt.injected;
 
-	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;
 }
 
 /*
@@ -61,20 +73,6 @@  static int kvm_cpu_has_extint(struct kvm_vcpu *v)
  */
 int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
 {
-	/*
-	 * FIXME: interrupt.injected represents an interrupt that it's
-	 * side-effects have already been applied (e.g. bit from IRR
-	 * already moved to ISR). Therefore, it is incorrect to rely
-	 * on interrupt.injected 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.
-	 */
-	if (!lapic_in_kernel(v))
-		return v->arch.interrupt.injected;
-
 	if (kvm_cpu_has_extint(v))
 		return 1;
 
@@ -91,20 +89,6 @@  EXPORT_SYMBOL_GPL(kvm_cpu_has_injectable_intr);
  */
 int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 {
-	/*
-	 * FIXME: interrupt.injected represents an interrupt that it's
-	 * side-effects have already been applied (e.g. bit from IRR
-	 * already moved to ISR). Therefore, it is incorrect to rely
-	 * on interrupt.injected 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.
-	 */
-	if (!lapic_in_kernel(v))
-		return v->arch.interrupt.injected;
-
 	if (kvm_cpu_has_extint(v))
 		return 1;
 
@@ -118,16 +102,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 */
 }
 
 /*
@@ -135,13 +124,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 105e7859d1f2..bb5ff761d5e2 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2465,7 +2465,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);