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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
> gic_events_need_delivery() reads the cur_priority field twice, also
This does not seem inline with what we discussed. I.e cur_priority can 
be read without the pending_irq lock, assuming proper barrier are around.
If the problem is reading it twice, then an ACCESS_ONCE(...) should fix it.

> relies on the consistency of status bits.
status has been designed to be used without lock. If this is not the 
case anymore, then we should document it.
In this particular case, I need a bit more context to see why this lock 
is necessary. IHMO, there are other way to avoid it, such as reading 
both the priority and the enabled bit before hand.
> So it should take pending_irq lock.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index df89530..9637682 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -666,7 +666,7 @@ int gic_events_need_delivery(void)
>  {
>      struct vcpu *v = current;
>      struct pending_irq *p;
> -    unsigned long flags;
> +    unsigned long flags, vcpu_flags;
>      const unsigned long apr = gic_hw_ops->read_apr(0);
>      int mask_priority;
>      int active_priority;
> @@ -675,7 +675,7 @@ int gic_events_need_delivery(void)
>      mask_priority = gic_hw_ops->read_vmcr_priority();
>      active_priority = find_next_bit(&apr, 32, 0);
>
> -    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +    spin_lock_irqsave(&v->arch.vgic.lock, vcpu_flags);
>
>      /* TODO: We order the guest irqs by priority, but we don't change
>       * the priority of host irqs. */
> @@ -684,19 +684,21 @@ int gic_events_need_delivery(void)
>       * ordered by priority */
>      list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
>      {
> -        if ( GIC_PRI_TO_GUEST(p->cur_priority) >= mask_priority )
> -            goto out;
> -        if ( GIC_PRI_TO_GUEST(p->cur_priority) >= active_priority )
> -            goto out;
> -        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +        vgic_irq_lock(p, flags);
> +        if ( GIC_PRI_TO_GUEST(p->cur_priority) < mask_priority &&
> +             GIC_PRI_TO_GUEST(p->cur_priority) < active_priority &&
> +             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>          {
> -            rc = 1;
> -            goto out;
> +            vgic_irq_unlock(p, flags);
> +            continue;
>          }
> +
> +        rc = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +        vgic_irq_unlock(p, flags);
> +        break;
>      }
>
> -out:
> -    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags);
>      return rc;
>  }
Cheers,

--
Julien Grall

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