Re: [Xen-devel] [RFC PATCH v2 10/22] ARM: vGIC: protect gic_set_lr() with pending_irq lock

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
> When putting a (pending) IRQ into an LR, we should better make sure that
> no-one changes it behind our back. So make sure we take the pending_irq
> lock. This bubbles up to all users of gic_add_to_lr_pending() and
> gic_raise_guest_irq().
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8dec736..df89530 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -383,6 +383,7 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, 
> struct pending_irq *n)
>      struct pending_irq *iter;
>
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
> +    ASSERT(spin_is_locked(&n->lock));
I think we need a similar assert in gic_raise_guest_irq and gic_set_lr.

>      if ( !list_empty(&n->lr_queue) )
>          return;
> @@ -480,6 +481,7 @@ void gic_update_one_lr(struct vcpu *v, int i)
>      struct pending_irq *p;
>      int irq;
>      struct gic_lr lr_val;
> +    unsigned long flags;
>
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>      ASSERT(!local_irq_is_enabled());
> @@ -534,6 +536,7 @@ void gic_update_one_lr(struct vcpu *v, int i)
>          gic_hw_ops->clear_lr(i);
>          clear_bit(i, &this_cpu(lr_mask));
>
> +        vgic_irq_lock(p, flags);
>          if ( p->desc != NULL )
>              clear_bit(_IRQ_INPROGRESS, &p->desc->status);
>          clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> @@ -559,6 +562,7 @@ void gic_update_one_lr(struct vcpu *v, int i)
>                  clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
>              }
>          }
> +        vgic_irq_unlock(p, flags);
>      }
>  }
>
> @@ -592,11 +596,11 @@ static void gic_restore_pending_irqs(struct vcpu *v)
>      int lr = 0;
>      struct pending_irq *p, *t, *p_r;
>      struct list_head *inflight_r;
> -    unsigned long flags;
> +    unsigned long flags, vcpu_flags;
>      unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>      int lrs = nr_lrs;
>
> -    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +    spin_lock_irqsave(&v->arch.vgic.lock, vcpu_flags);
See my comment on previous patches about the renaming.

>      if ( list_empty(&v->arch.vgic.lr_pending) )
>          goto out;
> @@ -621,16 +625,20 @@ static void gic_restore_pending_irqs(struct vcpu *v)
>              goto out;
>
>  found:
> +            vgic_irq_lock(p_r, flags);
>              lr = p_r->lr;
>              p_r->lr = GIC_INVALID_LR;
>              set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
>              clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
>              gic_add_to_lr_pending(v, p_r);
>              inflight_r = &p_r->inflight;
> +            vgic_irq_unlock(p_r, flags);
Some description in the commit message is necessary to explain why the 
lock is protecting more than what the patch is meant to do (i.e just 
protect gic_set_lr).
>          }
>
> +        vgic_irq_lock(p, flags);
>          gic_set_lr(lr, p, GICH_LR_PENDING);
>          list_del_init(&p->lr_queue);
> +        vgic_irq_unlock(p, flags);
Ditto. In this case, I thought the lists were protected by the the vCPU 
lock. So technically list_del_init(...) could be outside of the lock.
>          set_bit(lr, &this_cpu(lr_mask));
>
>          /* We can only evict nr_lrs entries */
> @@ -640,7 +648,7 @@ found:
>      }
>
>  out:
> -    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags);
>  }
>
>  void gic_clear_pending_irqs(struct vcpu *v)
Cheers,

--
Julien Grall

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