[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.