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

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



Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
When putting a (pending) IRQ into an LR, we should better make sure that
no-one changes it behind our back. So make sure we take the pending_irq
lock. This bubbles up to all users of gic_add_to_lr_pending() and
gic_raise_guest_irq().

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8dec736..df89530 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -383,6 +383,7 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, 
struct pending_irq *n)
     struct pending_irq *iter;

     ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&n->lock));

I think we need a similar assert in gic_raise_guest_irq and gic_set_lr.


     if ( !list_empty(&n->lr_queue) )
         return;
@@ -480,6 +481,7 @@ void gic_update_one_lr(struct vcpu *v, int i)
     struct pending_irq *p;
     int irq;
     struct gic_lr lr_val;
+    unsigned long flags;

     ASSERT(spin_is_locked(&v->arch.vgic.lock));
     ASSERT(!local_irq_is_enabled());
@@ -534,6 +536,7 @@ void gic_update_one_lr(struct vcpu *v, int i)
         gic_hw_ops->clear_lr(i);
         clear_bit(i, &this_cpu(lr_mask));

+        vgic_irq_lock(p, flags);
         if ( p->desc != NULL )
             clear_bit(_IRQ_INPROGRESS, &p->desc->status);
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
@@ -559,6 +562,7 @@ void gic_update_one_lr(struct vcpu *v, int i)
                 clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
             }
         }
+        vgic_irq_unlock(p, flags);
     }
 }

@@ -592,11 +596,11 @@ static void gic_restore_pending_irqs(struct vcpu *v)
     int lr = 0;
     struct pending_irq *p, *t, *p_r;
     struct list_head *inflight_r;
-    unsigned long flags;
+    unsigned long flags, vcpu_flags;
     unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
     int lrs = nr_lrs;

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

See my comment on previous patches about the renaming.


     if ( list_empty(&v->arch.vgic.lr_pending) )
         goto out;
@@ -621,16 +625,20 @@ static void gic_restore_pending_irqs(struct vcpu *v)
             goto out;

 found:
+            vgic_irq_lock(p_r, flags);
             lr = p_r->lr;
             p_r->lr = GIC_INVALID_LR;
             set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
             clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
             gic_add_to_lr_pending(v, p_r);
             inflight_r = &p_r->inflight;
+            vgic_irq_unlock(p_r, flags);

Some description in the commit message is necessary to explain why the lock is protecting more than what the patch is meant to do (i.e just protect gic_set_lr).

         }

+        vgic_irq_lock(p, flags);
         gic_set_lr(lr, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
+        vgic_irq_unlock(p, flags);

Ditto. In this case, I thought the lists were protected by the the vCPU lock. So technically list_del_init(...) could be outside of the lock.

         set_bit(lr, &this_cpu(lr_mask));

         /* We can only evict nr_lrs entries */
@@ -640,7 +648,7 @@ found:
     }

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

 void gic_clear_pending_irqs(struct vcpu *v)


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®.