Re: [Xen-devel] [PATCH 31/38] arm: gic.lock can be taken in interrupt context, so lock appropriately.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-06-26 at 13:25 +0100, Ian Campbell wrote:
> On Tue, 2012-06-26 at 12:41 +0100, Stefano Stabellini wrote:
> > On Tue, 26 Jun 2012, Ian Campbell wrote:
> > > > > @@ -601,7 +601,7 @@ static void maintenance_interrupt(int irq, void 
> > > > > *dev_id, struct cpu_user_regs *r
> > > > >          } else {
> > > > >              gic_inject_irq_stop();
> > > > >          }
> > > > > -        spin_unlock(&gic.lock);
> > > > > +        spin_unlock_irq(&gic.lock);
> > > > >  
> > > > >          spin_lock(&current->arch.vgic.lock);
> > > >                ^
> > > > shouldn't you change this into spin_lock_irq too?
> > > 
> > > 
> > > If so then that should be in "arm: use interrupt safe spin locks in
> > > vgic_vcpu_inject_irq" rather than here?
> > > 
> > > I think you've reworked this stuff a bit in one of your follow up series
> > > -- is it worth me changing this here or do you handle it / make it
> > > irrelevant?
> >  
> > No, I am not, I am only removing everything related to
> > events_maintenance. I think it is worth fixing it in one of your patches
> > or in a follow up patch.
> 
> OK, I'll do it in another patch.

Actually, this should have been in "arm: use interrupt safe spin locks
in vgic_vcpu_inject_irq" after all. I also noticed a bug in that patch
(using spin_unlock_irq not irqrestore in one exit path from
vgic_vcpu_inject_irq).

The new combined patch is (this is against my v2 series and replaces
"arm: use interrupt safe spin locks in vgic_vcpu_inject_irq" there):

8<--------------------------------------------------------------

>From 33832172c77d72cd87505f4926f380ee800ff0c7 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@xxxxxxxxxx>
Date: Tue, 26 Jun 2012 09:00:55 +0000
Subject: [PATCH] arm: make vgic lock safe for use in interrupt context.

In particular vgic_vcpu_inject_irq can be called in both interupt and regular
context.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
---
 xen/arch/arm/gic.c  |    4 ++--
 xen/arch/arm/vgic.c |   11 ++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 339c327..1e60d3b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -604,14 +604,14 @@ static void maintenance_interrupt(int irq, void *dev_id, 
struct cpu_user_regs *r
         }
         spin_unlock(&gic.lock);
 
-        spin_lock(&current->arch.vgic.lock);
+        spin_lock_irq(&current->arch.vgic.lock);
         p = irq_to_pending(current, virq);
         if ( p->desc != NULL ) {
             p->desc->status &= ~IRQ_INPROGRESS;
             GICC[GICC_DIR] = virq;
         }
         list_del_init(&p->inflight);
-        spin_unlock(&current->arch.vgic.lock);
+        spin_unlock_irq(&current->arch.vgic.lock);
 
         i++;
     }
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index af3523f..a217151 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -114,8 +114,8 @@ int vcpu_vgic_init(struct vcpu *v)
     return 0;
 }
 
-#define vgic_lock(v)   spin_lock(&(v)->domain->arch.vgic.lock)
-#define vgic_unlock(v) spin_unlock(&(v)->domain->arch.vgic.lock)
+#define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
+#define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
 
 #define vgic_lock_rank(v, r) spin_lock(&(r)->lock)
 #define vgic_unlock_rank(v, r) spin_unlock(&(r)->lock)
@@ -550,6 +550,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, 
int virtual)
     uint8_t priority;
     struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, idx);
     struct pending_irq *iter, *n = irq_to_pending(v, irq);
+    unsigned long flags;
 
     /* irq still pending */
     if (!list_empty(&n->inflight))
@@ -566,18 +567,18 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
irq, int virtual)
 
     gic_set_guest_irq(irq, GICH_LR_PENDING, priority);
 
-    spin_lock(&v->arch.vgic.lock);
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
     {
         if ( iter->priority > priority )
         {
             list_add_tail(&n->inflight, &iter->inflight);
-            spin_unlock(&v->arch.vgic.lock);
+            spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
             return;
         }
     }
     list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
-    spin_unlock(&v->arch.vgic.lock);
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
     /* we have a new higher priority irq, inject it into the guest */
 }
 
-- 
1.7.9.1




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