(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?
+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.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
