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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-07 at 11:49 +0100, Stefano Stabellini wrote:
> On Fri, 1 Jun 2012, Ian Campbell wrote:
> > In particular it is taken by gic_set_guest_irq which is called by
> > vgic_vcpu_inject_irq
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/gic.c |   20 ++++++++++----------
> >  1 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index a398f92..ededa99 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -329,19 +329,19 @@ int __init gic_init(void)
> >  /* Set up the per-CPU parts of the GIC for a secondary CPU */
> >  void __cpuinit gic_init_secondary_cpu(void)
> >  {
> > -    spin_lock(&gic.lock);
> > +    spin_lock_irq(&gic.lock);
> >      gic_cpu_init();
> >      gic_hyp_init();
> > -    spin_unlock(&gic.lock);
> > +    spin_unlock_irq(&gic.lock);
> >  }
> >  
> >  /* Shut down the per-CPU GIC interface */
> >  void gic_disable_cpu(void)
> >  {
> > -    spin_lock(&gic.lock);
> > +    spin_lock_irq(&gic.lock);
> >      gic_cpu_disable();
> >      gic_hyp_disable();
> > -    spin_unlock(&gic.lock);
> > +    spin_unlock_irq(&gic.lock);
> >  }
> >  
> >  void gic_route_irqs(void)
> > @@ -439,7 +439,7 @@ void gic_set_guest_irq(unsigned int virtual_irq,
> >  
> >      events_maintenance(current);
> >  
> > -    spin_lock(&gic.lock);
> > +    spin_lock_irq(&gic.lock);
> >  
> >      if ( list_empty(&gic.lr_pending) )
> >      {
> > @@ -465,7 +465,7 @@ void gic_set_guest_irq(unsigned int virtual_irq,
> >      list_add_tail(&n->lr_queue, &gic.lr_pending);
> >  
> >  out:
> > -    spin_unlock(&gic.lock);
> > +    spin_unlock_irq(&gic.lock);
> >      return;
> >  }
> >  
> > @@ -559,7 +559,7 @@ static void events_maintenance(struct vcpu *v)
> >              (unsigned long *)&vcpu_info(v, evtchn_upcall_pending));
> >  
> >      if (!already_pending && gic.event_mask != 0) {
> > -        spin_lock(&gic.lock);
> > +        spin_lock_irq(&gic.lock);
> >          while ((i = find_next_bit((const long unsigned int *) 
> > &gic.event_mask,
> >                          sizeof(uint64_t), i)) < sizeof(uint64_t)) {
> >  
> > @@ -569,7 +569,7 @@ static void events_maintenance(struct vcpu *v)
> >  
> >              i++;
> >          }
> > -        spin_unlock(&gic.lock);
> > +        spin_unlock_irq(&gic.lock);
> >      }
> >  }
> >  
> > @@ -585,7 +585,7 @@ static void maintenance_interrupt(int irq, void 
> > *dev_id, struct cpu_user_regs *r
> >                                sizeof(eisr), i)) < sizeof(eisr)) {
> >          struct pending_irq *p;
> >  
> > -        spin_lock(&gic.lock);
> > +        spin_lock_irq(&gic.lock);
> >          lr = GICH[GICH_LR + i];
> >          virq = lr & GICH_LR_VIRTUAL_MASK;
> >          GICH[GICH_LR + i] = 0;
> > @@ -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?

Ian.



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