Re: [Xen-devel] [PATCH] arm/acpi: Fix the deadlock in function vgic_lock_rank()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 31 May 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 31/05/16 10:40, Stefano Stabellini wrote:
> > On Mon, 30 May 2016, Julien Grall wrote:
> > > ACPI can only be enabled in expert mode and will be a tech-preview for Xen
> > > 4.7. So I would revert the patch.  SPIs will not be routed, but it is
> > > better
> > > than a deadlock.
> > > 
> > > I would also replace the patch with a warning until the issue will be
> > > fixed in
> > > Xen 4.8.
> > > 
> > > Any opinions?
> > > 
> > > > +int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> > > > +                           struct irq_desc *desc, unsigned int
> > > > priority)
> > > > +{
> > > > +    unsigned long flags;
> > > > +    int lock = 0, retval;
> > > > +    struct vgic_irq_rank *rank;
> > > > +
> > > > +    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> > > > +     * route SPIs to guests, it doesn't make any difference. */
> > > > +    rank = vgic_rank_irq(d->vcpu[0], virq);
> > > > +
> > > > +    /* Take the rank spinlock unless it has already been taken by the
> > > > +     * caller. */
> > > > +    if ( !spin_is_locked(&rank->lock) ) {
> > > 
> > > AFAICT, spin_is_locked only tell us that someone has locked the rank. So
> > > this
> > > would be unsafe.
> > 
> > The code is checking if the lock is already taken, and if it is not
> > taken, it will take the lock. The purpose of this code is to
> > allow gic_route_irq_to_guest to be called by both functions which
> > already have the lock held and functions that do not. The same goal
> > could be achieved by duplicating gic_route_irq_to_guest into two
> > identical functions except for the lock taking. That would be
> > admittedly a more obvious fix but also a particularly ugly one.
> 
> spin_is_locked does not work as you expect. The function will not tell you if
> the lock was taken by the current CPU, but if the lock was taken by *a* CPU.
> 
> It would be possible to have CPU A calling this function and have CPU B with
> the lock taken. So the data structure would be accessed by 2 CPUs
> concurrently, which is unsafe.

Damn, you are right. I don't think we have a spin_lock function which
tells us if the spin_lock was taken by us.

The only other option I see would be duplicating route_irq_to_guest and
gic_route_irq_to_guest, introducing a second version of those functions
which assume that the rank lock was already taken. Very very ugly. I'll
just revert the commit and wait for better patches from Shannon.

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