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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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

> gic.lock only protects accesses to GICD now.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> ---
> 
> 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.

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

Ian.


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