[Xen-devel] [PATCH] x86/vpt: fix a bug in pt_update_irq()

[Chao Gao <chao.gao@xxxxxxxxx>]

pt_update_irq() is expected to return the vector number of periodic
timer interrupt, which should be set in vIRR of vlapic. Otherwise it
would trigger the assertion in vmx_intr_assist(), please seeing
https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.

But it fails to achieve that in the following two case:
1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
mask field of IOAPIC RTE is set. Please refer to the call tree
vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
checks whether the vector is set or not in vIRR of vlapic before
returning.

2. someone changes the vector field of IOAPIC RTE between asserting
the irq and getting the vector of the irq, leading to setting the
old vector number but returning a different vector number. This patch
holds the irq_lock when doing the two operations to prevent the case.

Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
---
 xen/arch/x86/hvm/irq.c           | 11 ++++++----
 xen/arch/x86/hvm/vlapic.c        | 11 ++++++++++
 xen/arch/x86/hvm/vpt.c           | 43 ++++++++++++++++++++++++++++------------
 xen/include/asm-x86/hvm/irq.h    |  1 +
 xen/include/asm-x86/hvm/vlapic.h |  1 +
 5 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index e425df9..7b0c0b1 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -168,20 +168,23 @@ void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
-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)
 {
     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);
 }
 
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4bfc53e..b27b15b 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -137,6 +137,17 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
int errmask)
     spin_unlock_irqrestore(&vlapic->esr_lock, flags);
 }
 
+bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec)
+{
+    if ( unlikely(vec < 16) )
+        return false;
+
+    if ( hvm_funcs.sync_pir_to_irr )
+        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
+
+    return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
+}
+
 void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
 {
     struct vcpu *target = vlapic_vcpu(vlapic);
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 3841140..f4451fd 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -252,7 +252,7 @@ int pt_update_irq(struct vcpu *v)
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
     struct periodic_time *pt, *temp, *earliest_pt;
     uint64_t max_lag;
-    int irq, is_lapic;
+    int irq, is_lapic, pt_vector;
 
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
@@ -292,25 +292,42 @@ int pt_update_irq(struct vcpu *v)
 
     spin_unlock(&v->arch.hvm_vcpu.tm_lock);
 
+    /*
+     * If periodic timer interrut is handled by lapic, its vector in
+     * IRR is returned and used to set eoi_exit_bitmap for virtual
+     * interrupt delivery case. Otherwise return -1 to do nothing.
+     */
     if ( is_lapic )
+    {
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
+        pt_vector = irq;
+    }
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
-        hvm_isa_irq_assert(v->domain, irq);
+        /*
+         * Hold 'irq_lock' to prevent changing the interrupt vector between
+         * asserting the irq and getting the interrupt vector of the irq.
+         */
+        spin_lock(&v->domain->arch.hvm_domain.irq_lock);
+        hvm_isa_irq_assert_locked(v->domain, irq);
+        if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
+             v->domain->arch.hvm_domain.vpic[irq >> 3].int_output )
+            pt_vector = -1;
+        else
+        {
+            pt_vector = pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
+            /*
+             * hvm_isa_irq_assert_locked() may not set the corresponding bit
+             * in vIRR when mask field of IOAPIC RTE is set. Check it again.
+             */
+            if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
+                pt_vector = -1;
+        }
+        spin_unlock(&v->domain->arch.hvm_domain.irq_lock);
     }
 
-    /*
-     * If periodic timer interrut is handled by lapic, its vector in
-     * IRR is returned and used to set eoi_exit_bitmap for virtual
-     * interrupt delivery case. Otherwise return -1 to do nothing.  
-     */ 
-    if ( !is_lapic &&
-         platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
-         (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
-        return -1;
-    else 
-        return pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
+    return pt_vector;
 }
 
 static struct periodic_time *is_pt_irq(
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 3b6b4bd..d20131a 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -189,6 +189,7 @@ void hvm_pci_intx_deassert(struct domain *d, unsigned int 
device,
 
 /* 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);
 
 /* Modify state of GSIs. */
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index a63fcd5..1f849c4 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -110,6 +110,7 @@ static inline void vlapic_set_reg(
 
 bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
 
+bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec);
 void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
 
 int vlapic_has_pending_irq(struct vcpu *v);
-- 
1.8.3.1


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