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

[Julien Grall <julien.grall@xxxxxxx>]

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.
Regards,

--
Julien Grall

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