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

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2016/5/31 3:45, 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).
> 
> 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?

I agree and I'm so sorry for this problem.

Thanks,
-- 
Shannon


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