Re: [Xen-devel] [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 08/05/17 10:15, Andre Przywara wrote:
> On 04/05/17 16:53, Julien Grall wrote:
> > Hi Andre,
> >
> > On 04/05/17 16:31, Andre Przywara wrote:
> > > gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
> > > lock, however never actually access the rank structure.
> > > Avoid taking the lock in those two functions and remove some more
> > > unneeded code on the way.
> > The rank is here to protect p->desc when checking that the virtual
> > interrupt was not yet routed to another physical interrupt.
> Really? To me that sounds quite surprising.
> My understanding is that the VGIC VCPU lock protected the pending_irq
> (and thus the desc pointer?) so far, and the desc itself has its own lock.
> According to the comment in the struct irq_rank declaration the lock
> protects the members of this struct only.
>
> Looking briefly at users of pending_irq->desc (for instance
> gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
> they care about the lock.
>
> So should that be fixed or at least documented?
My understanding is this rank lock is preventing race between two 
updates of p->desc. This can happen if gic_route_irq_to_guest(...) is 
called concurrently with the same vIRQ but different pIRQ.
If you drop this lock, nothing will protect that anymore.

> > Without this locking, you can have two concurrent call of
> > gic_route_irq_to_guest that will update the same virtual interrupt but
> > with different physical interrupts.
> >
> > You would have to replace the rank lock by the per-pending_irq lock to
> > keep the code safe.
> That indeed sounds reasonable.
As you mentioned IRL, the current code may lead to a deadlock due to 
locking order.
Indeed routing an IRQ (route_irq_to_guest) will take:
        1) desc lock (in route_irq_to_guest)
        2) rank lock (in gic_route_irq_to_guest)

Whilst the MMIO emulation of ISENABLER_* will take:
        1) rank lock
        2) desc lock (in vgic_enable_irqs)

Using the per-pending_irq lock will not solve the deadlock. I though a 
bit more to the code. I believe the routing of SPIs/PPIs after domain 
creation time is a call to mistake and locking nightmare. Similarly an 
interrupt should stay routed for the duration of the domain life.
So I would forbid IRQ routing after domain creation (see 
d->creation_finished) and remove IRQ whilst routing (I think with 
d->is_dying). This would have an race between the routing and the rest 
of the vGIC code.
However, this would not prevent the routing function to race against 
itself. For that I would take the vgic domain lock, it is fine because 
routing is not something we expect to happen often.
Any opinions?

Cheers,

--
Julien Grall

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