[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v2 11/22] ARM: vGIC: protect gic_events_need_delivery() with pending_irq lock



Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
gic_events_need_delivery() reads the cur_priority field twice, also

This does not seem inline with what we discussed. I.e cur_priority can be read without the pending_irq lock, assuming proper barrier are around.

If the problem is reading it twice, then an ACCESS_ONCE(...) should fix it.

relies on the consistency of status bits.

status has been designed to be used without lock. If this is not the case anymore, then we should document it.

In this particular case, I need a bit more context to see why this lock is necessary. IHMO, there are other way to avoid it, such as reading both the priority and the enabled bit before hand.

So it should take pending_irq lock.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/gic.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index df89530..9637682 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -666,7 +666,7 @@ int gic_events_need_delivery(void)
 {
     struct vcpu *v = current;
     struct pending_irq *p;
-    unsigned long flags;
+    unsigned long flags, vcpu_flags;
     const unsigned long apr = gic_hw_ops->read_apr(0);
     int mask_priority;
     int active_priority;
@@ -675,7 +675,7 @@ int gic_events_need_delivery(void)
     mask_priority = gic_hw_ops->read_vmcr_priority();
     active_priority = find_next_bit(&apr, 32, 0);

-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+    spin_lock_irqsave(&v->arch.vgic.lock, vcpu_flags);

     /* TODO: We order the guest irqs by priority, but we don't change
      * the priority of host irqs. */
@@ -684,19 +684,21 @@ int gic_events_need_delivery(void)
      * ordered by priority */
     list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
     {
-        if ( GIC_PRI_TO_GUEST(p->cur_priority) >= mask_priority )
-            goto out;
-        if ( GIC_PRI_TO_GUEST(p->cur_priority) >= active_priority )
-            goto out;
-        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+        vgic_irq_lock(p, flags);
+        if ( GIC_PRI_TO_GUEST(p->cur_priority) < mask_priority &&
+             GIC_PRI_TO_GUEST(p->cur_priority) < active_priority &&
+             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
         {
-            rc = 1;
-            goto out;
+            vgic_irq_unlock(p, flags);
+            continue;
         }
+
+        rc = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
+        vgic_irq_unlock(p, flags);
+        break;
     }

-out:
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags);
     return rc;
 }



Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.