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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 31, 2016 at 10:40:13AM +0100, Stefano Stabellini wrote:
> On Mon, 30 May 2016, Julien Grall wrote:
> > (CC Wei Liu)
> > 
> > Hi Stefano,
> > 
> > On 30/05/2016 14:16, Stefano Stabellini wrote:
> > > On Fri, 27 May 2016, Julien Grall wrote:
> > > > Hello Shanker,
> > > > 
> > > > On 27/05/16 01:39, Shanker Donthineni wrote:
> > > > > Commit 9d77b3c01d1261c (Configure SPI interrupt type and route to
> > > > > Dom0 dynamically) causing dead loop inside the spinlock function.
> > > > > Note that spinlocks in XEN are not recursive. Re-acquiring a spinlock
> > > > > that has already held by calling CPU leads to deadlock. This happens
> > > > > whenever dom0 does writes to GICD regs ISENABLER/ICENABLER.
> > > > 
> > > > Thank you for spotting it, I have not noticed it while I was  reviewing,
> > > > only
> > > > tested on a model without any SPIs.
> > > > 
> > > > > The following call trace explains the problem.
> > > > > 
> > > > > DOM0 writes GICD_ISENABLER/GICD_ICENABLER
> > > > >    vgic_v3_distr_common_mmio_write()
> > > > >      vgic_lock_rank()  -->  acquiring first time
> > > > >        vgic_enable_irqs()
> > > > >          route_irq_to_guest()
> > > > >            gic_route_irq_to_guest()
> > > > >              vgic_get_target_vcpu()
> > > > >                vgic_lock_rank()  -->  attemping acquired lock
> > > > > 
> > > > > The simple fix release spinlock before calling vgic_enable_irqs()
> > > > > and vgic_disable_irqs().
> > > > 
> > > > You should explain why you think it is valid to release the lock 
> > > > earlier.
> > > > 
> > > > In this case, I think the fix is not correct because the lock is
> > > > protecting
> > > > both the register value and the internal state in Xen (modified by
> > > > vgic_enable_irqs). By releasing the lock earlier, they may become
> > > > inconsistent
> > > > if another vCPU is disabling the IRQs at the same time.
> > > 
> > > I agree, the vgic_enable_irqs call need to stay within the
> > > vgic_lock_rank/vgic_unlock_rank region.
> > > 
> > > 
> > > > I cannot find an easy fix which does not involve release the lock. When 
> > > > I
> > > > was
> > > > reviewing this patch, I suggested to split the IRQ configuration from 
> > > > the
> > > > routing.
> > > 
> > > Yes, the routing doesn't need to be done from vgic_enable_irqs. It is
> > > not nice. That would be the ideal fix, but it is not trivial.
> > > 
> > > For 4.7 we could consider reverting 9d77b3c01d1261c. The only other
> > > thing that I can come up with which is simple would be improving
> > > gic_route_irq_to_guest to cope with callers that have the vgic rank lock
> > > already held (see below, untested) but it's pretty ugly.
> > 
> > We are close to release Xen 4.7, so I think we should avoid to touch the
> > common interrupt code (i.e not only used by ACPI).
> 
> Agreed. Wei, are you OK with this?
> 

Bare in mind that I haven't looked into the issue in details, but in
principle I agree we should avoid touching common code at this stage.

Wei.

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