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@lists.xen.org
http://lists.xen.org/xen-devel
