Re: [Xen-devel] [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 04/05/17 16:31, Andre Przywara wrote:
> Introduce the proper locking sequence for the new pending_irq lock.
> This takes the lock around multiple accesses to struct members,
> also makes sure we observe the locking order (VGIC VCPU lock first,
> then pending_irq lock).
This locking order should be explained in the code. Likely in vgic.h.

> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c  | 26 ++++++++++++++++++++++++++
>  xen/arch/arm/vgic.c | 12 +++++++++++-
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 67375a2..e175e9b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -351,6 +351,7 @@ void gic_disable_cpu(void)
>  static inline void gic_set_lr(int lr, struct pending_irq *p,
>                                unsigned int state)
>  {
> +    ASSERT(spin_is_locked(&p->lock));
>      ASSERT(!local_irq_is_enabled());
>
>      gic_hw_ops->update_lr(lr, p, state);
> @@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct pending_irq 
> *p)
>      unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
> +    ASSERT(spin_is_locked(&p->lock));
>
>      if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
>      {
> @@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>      gic_hw_ops->read_lr(i, &lr_val);
>      irq = lr_val.virq;
>      p = irq_to_pending(v, irq);
> +    spin_lock(&p->lock);
>      if ( lr_val.state & GICH_LR_ACTIVE )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> @@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>              }
>          }
>      }
> +    spin_unlock(&p->lock);
>  }
>
>  void gic_clear_lrs(struct vcpu *v)
> @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu *v)
>              /* No more free LRs: find a lower priority irq to evict */
>              list_for_each_entry_reverse( p_r, inflight_r, inflight )
>              {
> +                if ( p_r->irq < p->irq )
> +                {
> +                    spin_lock(&p_r->lock);
> +                    spin_lock(&p->lock);
> +                }
> +                else
> +                {
> +                    spin_lock(&p->lock);
> +                    spin_lock(&p_r->lock);
> +                }
Please explain in the commit message and the code why this locking order.

>                  if ( p_r->priority == p->priority )
> +                {
> +                    spin_unlock(&p->lock);
> +                    spin_unlock(&p_r->lock);
>                      goto out;
> +                }
>                  if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
>                       !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
>                      goto found;
>              }
>              /* We didn't find a victim this time, and we won't next
>               * time, so quit */
> +            spin_unlock(&p->lock);
> +            spin_unlock(&p_r->lock);
>              goto out;
>
>  found:
> @@ -562,12 +582,18 @@ found:
>              clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
>              gic_add_to_lr_pending(v, p_r);
>              inflight_r = &p_r->inflight;
> +
> +            spin_unlock(&p_r->lock);
>          }
> +        else
> +            spin_lock(&p->lock);
>
>          gic_set_lr(lr, p, GICH_LR_PENDING);
>          list_del_init(&p->lr_queue);
>          set_bit(lr, &this_cpu(lr_mask));
>
> +        spin_unlock(&p->lock);
> +
>          /* We can only evict nr_lrs entries */
>          lrs--;
>          if ( lrs == 0 )
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index f4ae454..44363bb 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          v_target = vgic_get_target_vcpu(v, irq);
> +
> +        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>          p = irq_to_pending(v_target, irq);
> +        spin_lock(&p->lock);
> +
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> -        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
IHMO, this should belong to a separate patch as not strictly relate to 
this one.
> +
Spurious change.

>          if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, 
> &p->status) )
>              gic_raise_guest_irq(v_target, p);
> +        spin_unlock(&p->lock);
>          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>          if ( p->desc != NULL )
>          {
> @@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> virq)
>          return;
>      }
>
> +    spin_lock(&n->lock);
>      set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>
>      if ( !list_empty(&n->inflight) )
>      {
> +        spin_unlock(&n->lock);
>          gic_raise_inflight_irq(v, n);
Any reason to not code gic_raise_inflight_irq with the lock? This would 
also simplify quite a lot the function and avoid unlock in pretty much 
all the exit path.
>          goto out;
>      }
> @@ -501,10 +508,13 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> virq)
>          if ( iter->priority > priority )
>          {
>              list_add_tail(&n->inflight, &iter->inflight);
> +            spin_unlock(&n->lock);
>              goto out;
>          }
>      }
>      list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
> +    spin_unlock(&n->lock);
> +
>  out:
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>      /* we have a new higher priority irq, inject it into the guest */
Cheers,

--
Julien Grall

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