Re: [PATCH v4 09/10] xen/arm: don't protect GICH and lr_queue accesses with gic.lock

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > GICH is banked, protect accesses by disabling interrupts.
> > Protect lr_queue accesses with the vgic.lock only.
> 
> Does this rely on using the irq disabling spinlock_irq variants for this
> lock to also protect GICH?

Yes, specifically in gic_set_lr and gic_clear_one_lr.


> I don't see any actual calls to irq_disable so I suppose such things are
> always nested inside holding a vgic lock.

Yes, most of the times. However GICH changes are also made in
gic_save_state, gic_restore_state, gic_hyp_init, gic_hyp_disable and
gic_inject, where we can be sure that interrupts are disabled for other
reasons.


> > gic.lock only protects accesses to GICD now.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in v4:
> > - improved in code comments.
> > ---
> >  xen/arch/arm/gic.c           |   23 +++--------------------
> >  xen/arch/arm/vgic.c          |    9 +++++++--
> >  xen/include/asm-arm/domain.h |    5 ++++-
> >  3 files changed, 14 insertions(+), 23 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 128d071..bc9d66d 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -667,19 +667,15 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
> >  void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
> >  {
> >      struct pending_irq *p = irq_to_pending(v, virtual_irq);
> > -    unsigned long flags;
> >  
> > -    spin_lock_irqsave(&gic.lock, flags);
> >      if ( !list_empty(&p->lr_queue) )
> >          list_del_init(&p->lr_queue);
> 
> Where is vgic.lock held here? I looked back in the callchain and didn't
> see it.

Well spotted! This is a mistake! I'll fix it.


> > -    spin_unlock_irqrestore(&gic.lock, flags);
> >  }
> >  
> >  void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> >                           unsigned int priority)
> >  {
> >      int i;
> > -    unsigned long flags;
> >      struct pending_irq *n = irq_to_pending(v, irq);
> >  
> >      if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
> > @@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> >          return;
> >      }
> >  
> > -    spin_lock_irqsave(&gic.lock, flags);
> 
> This function requires the vgic lock to be held when it is called.
> 
> This locking (and implicit interrupt flag based locking) is getting
> pretty complex. I think it would be a good idea to start documenting
> this sort of requirement for this code in a comment at the top of the
> function, and perhaps with an assert in the entry path.
> 
> Likewise for functions which require interrupts to be disabled with a
> comment and an assert.
 
Good idea, I'll make the changes.