[Xen-devel] [RFC PATCH] hvm/vpt: fix inconsistent views of vIOAPIC in vmx_intr_assist()

[Chao Gao <chao.gao@xxxxxxxxx>]

When injecting periodic timer interrupt in vmx_intr_assist(), multiple read
operation is operated during one event delivery and incur to inconsistent
views of vIOAPIC. For example, if a periodic timer interrupt is from PIT, when
set the corresponding bit in vIRR, the corresponding RTE is accessed in
pt_update_irq(). When this function returns, it accesses the RTE again to get
the vector it set in vIRR.  Between the two accesses, the content of RTE may
have been changed by another CPU for no protection method in use. This case
can incur the assertion failure in vmx_intr_assist().  Also after a periodic
timer interrupt is injected successfully, pt_irq_posted() will decrease the
count (pending_intr_nr). But if the corresponding RTE has been changed,
pt_irq_posted() will not decrease the count, resulting one more timer
interrupt.

More discussion and history can be found in
1.https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg00906.html
2.https://lists.xenproject.org/archives/html/xen-devel/2017-01/msg01019.html

This patch simply uses irq_lock to avoid these inconsistent views of vIOAPIC.
To fix the deadlock, provide locked version of several functions.

Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
---
 xen/arch/x86/hvm/irq.c      | 22 ++++++++++++++++------
 xen/arch/x86/hvm/svm/intr.c | 21 +++++++++++++++------
 xen/arch/x86/hvm/vmx/intr.c |  9 +++++++++
 xen/arch/x86/hvm/vpic.c     | 20 ++++++++++++++------
 xen/arch/x86/hvm/vpt.c      |  4 ++--
 xen/include/xen/hvm/irq.h   |  4 ++++
 6 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 8625584..a1af61e 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -126,37 +126,47 @@ void hvm_pci_intx_deassert(
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
-void hvm_isa_irq_assert(
+void hvm_isa_irq_assert_locked(
     struct domain *d, unsigned int isa_irq)
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
 
     ASSERT(isa_irq <= 15);
-
-    spin_lock(&d->arch.hvm_domain.irq_lock);
+    ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
 
     if ( !__test_and_set_bit(isa_irq, &hvm_irq->isa_irq.i) &&
          (hvm_irq->gsi_assert_count[gsi]++ == 0) )
         assert_irq(d, gsi, isa_irq);
+}
 
+void hvm_isa_irq_assert(
+    struct domain *d, unsigned int isa_irq)
+{
+    spin_lock(&d->arch.hvm_domain.irq_lock);
+    hvm_isa_irq_assert_locked(d, isa_irq);
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
-void hvm_isa_irq_deassert(
+void hvm_isa_irq_deassert_locked(
     struct domain *d, unsigned int isa_irq)
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
 
     ASSERT(isa_irq <= 15);
-
-    spin_lock(&d->arch.hvm_domain.irq_lock);
+    ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
 
     if ( __test_and_clear_bit(isa_irq, &hvm_irq->isa_irq.i) &&
          (--hvm_irq->gsi_assert_count[gsi] == 0) )
         deassert_irq(d, isa_irq);
+}
 
+void hvm_isa_irq_deassert(
+    struct domain *d, unsigned int isa_irq)
+{
+    spin_lock(&d->arch.hvm_domain.irq_lock);
+    hvm_isa_irq_deassert_locked(d, isa_irq);
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 8511ff0..d2a318c 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -137,18 +137,25 @@ void svm_intr_assist(void)
     struct hvm_intack intack;
     enum hvm_intblk intblk;
 
+    /*
+     * Avoid the vIOAPIC RTE being changed by another vCPU.
+     * Otherwise, pt_update_irq() may return a wrong vector which is not in
+     * vIRR and pt_irq_posted() may not recognize a timer interrupt has been
+     * injected.
+     */
+    spin_lock(&v->domain->arch.hvm_domain.irq_lock);
     /* Crank the handle on interrupt state. */
     pt_update_irq(v);
 
     do {
         intack = hvm_vcpu_has_pending_irq(v);
         if ( likely(intack.source == hvm_intsrc_none) )
-            return;
+            goto out;
 
         intblk = hvm_interrupt_blocked(v, intack);
         if ( intblk == hvm_intblk_svm_gif ) {
             ASSERT(nestedhvm_enabled(v->domain));
-            return;
+            goto out;
         }
 
         /* Interrupts for the nested guest are already
@@ -165,13 +172,13 @@ void svm_intr_assist(void)
             switch (rc) {
             case NSVM_INTR_NOTINTERCEPTED:
                 /* Inject interrupt into 2nd level guest directly. */
-                break; 
+                goto out;
             case NSVM_INTR_NOTHANDLED:
             case NSVM_INTR_FORCEVMEXIT:
-                return;
+                goto out;
             case NSVM_INTR_MASKED:
                 /* Guest already enabled an interrupt window. */
-                return;
+                goto out;
             default:
                 panic("%s: nestedsvm_vcpu_interrupt can't handle value %#x",
                     __func__, rc);
@@ -195,7 +202,7 @@ void svm_intr_assist(void)
         if ( unlikely(vmcb->eventinj.fields.v) || intblk )
         {
             svm_enable_intr_window(v, intack);
-            return;
+            goto out;
         }
 
         intack = hvm_vcpu_ack_pending_irq(v, intack);
@@ -216,6 +223,8 @@ void svm_intr_assist(void)
     intack = hvm_vcpu_has_pending_irq(v);
     if ( unlikely(intack.source != hvm_intsrc_none) )
         svm_enable_intr_window(v, intack);
+ out:
+    spin_unlock(&v->domain->arch.hvm_domain.irq_lock);
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 1eb9f38..8f3d1a7 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -234,6 +234,14 @@ void vmx_intr_assist(void)
         return;
     }
 
+    /*
+     * Avoid the vIOAPIC RTE being changed by another vCPU.
+     * Otherwise, pt_update_irq() may return a wrong vector which is not in
+     * vIRR and pt_irq_posted() may not recognize a timer interrupt has been
+     * injected.
+     */
+    spin_lock(&v->domain->arch.hvm_domain.irq_lock);
+
     /* Crank the handle on interrupt state. */
     if ( is_hvm_vcpu(v) )
         pt_vector = pt_update_irq(v);
@@ -396,6 +404,7 @@ void vmx_intr_assist(void)
     }
 
  out:
+    spin_unlock(&v->domain->arch.hvm_domain.irq_lock);
     if ( !nestedhvm_vcpu_in_guestmode(v) &&
          !cpu_has_vmx_virtual_intr_delivery &&
          cpu_has_vmx_tpr_shadow )
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index e160bbd..8d02e52 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -153,14 +153,13 @@ static void __vpic_intack(struct hvm_hw_vpic *vpic, int 
irq)
     vpic_update_int_output(vpic);
 }
 
-static int vpic_intack(struct hvm_hw_vpic *vpic)
+static int vpic_intack_locked(struct hvm_hw_vpic *vpic)
 {
     int irq = -1;
 
-    vpic_lock(vpic);
-
+    ASSERT(vpic_is_locked(vpic));
     if ( !vpic->int_output )
-        goto out;
+        return irq;
 
     irq = vpic_get_highest_priority_irq(vpic);
     BUG_ON(irq < 0);
@@ -175,7 +174,15 @@ static int vpic_intack(struct hvm_hw_vpic *vpic)
         irq += 8;
     }
 
- out:
+    return irq;
+}
+
+static int vpic_intack(struct hvm_hw_vpic *vpic)
+{
+    int irq;
+
+    vpic_lock(vpic);
+    irq = vpic_intack_locked(vpic);
     vpic_unlock(vpic);
     return irq;
 }
@@ -487,13 +494,14 @@ int vpic_ack_pending_irq(struct vcpu *v)
     struct hvm_hw_vpic *vpic = &v->domain->arch.hvm_domain.vpic[0];
 
     ASSERT(has_vpic(v->domain));
+    ASSERT(vpic_is_locked(vpic));
 
     TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v),
              vpic->int_output);
     if ( !vlapic_accept_pic_intr(v) || !vpic->int_output )
         return -1;
 
-    irq = vpic_intack(vpic);
+    irq = vpic_intack_locked(vpic);
     if ( irq == -1 )
         return -1;
 
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index e3f2039..d048f26 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -296,8 +296,8 @@ int pt_update_irq(struct vcpu *v)
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
     else
     {
-        hvm_isa_irq_deassert(v->domain, irq);
-        hvm_isa_irq_assert(v->domain, irq);
+        hvm_isa_irq_deassert_locked(v->domain, irq);
+        hvm_isa_irq_assert_locked(v->domain, irq);
     }
 
     /*
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index f041252..a8f36f8 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -119,8 +119,12 @@ void hvm_pci_intx_deassert(
 /* Modify state of an ISA device's IRQ wire. */
 void hvm_isa_irq_assert(
     struct domain *d, unsigned int isa_irq);
+void hvm_isa_irq_assert_locked(
+    struct domain *d, unsigned int isa_irq);
 void hvm_isa_irq_deassert(
     struct domain *d, unsigned int isa_irq);
+void hvm_isa_irq_deassert_locked(
+    struct domain *d, unsigned int isa_irq);
 
 int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
 
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel