Re: [Xen-devel] [PATCH v3 14/39] ARM: new VGIC: Add GICv2 world switch backend

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

This is a "patch to the patch" mentioned above, to make it clear what
changed:
We now take the desc lock in vgic_v2_fold_lr_state() when we are dealing
with a hardware IRQ. This is a bit complicated, because we have to obey
the existing locking order, so do our infamous "drop-take-retake" dance.
Also I print a message about using the new VGIC and fix that last
remaining "u32" usage.

Please note that I had to initialise "desc" to NULL because my compiler
(GCC 5.3) is not smart enough to see that we only use it with irq->hw
set and it's safe. Please let me know if it's me not being smart enough
here instead ;-)

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
---
Hi,

will send a proper, merged v3a version of the patch separately.

Cheers,
Andre

 xen/arch/arm/vgic/vgic-v2.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 5516a8534f..3424a4a66f 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -43,6 +43,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t 
csize,
     gic_v2_hw_data.csize = csize;
     gic_v2_hw_data.vbase = vbase;
     gic_v2_hw_data.aliased_offset = aliased_offset;
+
+    printk("Using the new VGIC implementation.\n");
 }
 
 /*
@@ -69,6 +71,8 @@ void vgic_v2_fold_lr_state(struct vcpu *vcpu)
         struct gic_lr lr_val;
         uint32_t intid;
         struct vgic_irq *irq;
+        struct irq_desc *desc = NULL;
+        bool have_desc_lock = false;
 
         gic_hw_ops->read_lr(lr, &lr_val);
 
@@ -88,18 +92,30 @@ void vgic_v2_fold_lr_state(struct vcpu *vcpu)
         intid = lr_val.virq;
         irq = vgic_get_irq(vcpu->domain, vcpu, intid);
 
-        spin_lock_irqsave(&irq->irq_lock, flags);
+        local_irq_save(flags);
+        spin_lock(&irq->irq_lock);
+
+        /* The locking order forces us to drop and re-take the locks here. */
+        if ( irq->hw )
+        {
+            spin_unlock(&irq->irq_lock);
+
+            desc = irq_to_desc(irq->hwintid);
+            spin_lock(&desc->lock);
+            spin_lock(&irq->irq_lock);
+
+            /* This h/w IRQ should still be assigned to the virtual IRQ. */
+            ASSERT(irq->hw && desc->irq == irq->hwintid);
+
+            have_desc_lock = true;
+        }
 
         /*
          * If a hardware mapped IRQ has been handled for good, we need to
          * clear the _IRQ_INPROGRESS bit to allow handling of new IRQs.
          */
         if ( irq->hw && !lr_val.active && !lr_val.pending )
-        {
-            struct irq_desc *irqd = irq_to_desc(irq->hwintid);
-
-            clear_bit(_IRQ_INPROGRESS, &irqd->status);
-        }
+            clear_bit(_IRQ_INPROGRESS, &desc->status);
 
         /* Always preserve the active bit */
         irq->active = lr_val.active;
@@ -132,18 +148,19 @@ void vgic_v2_fold_lr_state(struct vcpu *vcpu)
          */
         if ( vgic_irq_is_mapped_level(irq) && lr_val.pending )
         {
-            struct irq_desc *irqd;
-
             ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
 
-            irqd = irq_to_desc(irq->hwintid);
-            irq->line_level = gic_read_pending_state(irqd);
+            irq->line_level = gic_read_pending_state(desc);
 
             if ( !irq->line_level )
-                gic_set_active_state(irqd, false);
+                gic_set_active_state(desc, false);
         }
 
-        spin_unlock_irqrestore(&irq->irq_lock, flags);
+        spin_unlock(&irq->irq_lock);
+        if ( have_desc_lock )
+            spin_unlock(&desc->lock);
+        local_irq_restore(flags);
+
         vgic_put_irq(vcpu->domain, irq);
     }
 
@@ -184,7 +201,7 @@ void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq 
*irq, int lr)
 
         if ( vgic_irq_is_sgi(irq->intid) )
         {
-            u32 src = ffs(irq->source);
+            uint32_t src = ffs(irq->source);
 
             BUG_ON(!src);
             lr_val.virt.source = (src - 1);
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel