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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 4 May 2017, Julien Grall wrote:
> On 04/05/17 16:31, Andre Przywara wrote:
> > 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);
> > +
> >          if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE,
> > &p->status) )
> >              gic_raise_guest_irq(v_target, p);
> > +        spin_unlock(&p->lock);
> 
> Why does the lock not cover p->desc below?

Indeed. The main problem with this patch is that it doesn't say what
this lock is supposed to cover. It is OK for the lock not to cover
everything pending_irq related as long as it is clear.

As it stands, it is not clear.

For example, why the lock is not added to following functions?

- gic_route_irq_to_guest
- gic_remove_irq_from_guest
- gic_remove_from_queues
- gic_raise_inflight_irq
- vgic_migrate_irq
- arch_move_irqs
- vgic_disable_irqs

I came up with this list by grepping for irq_to_pending and listing the
function where fields of the pending_irq struct are accessed.



> >          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> >          if ( p->desc != NULL )
> >          {

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