[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v2 01/22] ARM: vGIC: introduce and initialize pending_irq lock



Hi,

On 21/07/17 20:59, Andre Przywara wrote:
Currently we protect the pending_irq structure with the corresponding
VGIC VCPU lock. There are problems in certain corner cases (for
instance if an IRQ is migrating), so let's introduce a per-IRQ lock,
which will protect the consistency of this structure independent from
any VCPU.
For now this just introduces and initializes the lock, also adds
wrapper macros to simplify its usage (and help debugging).

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/vgic.c        |  1 +
 xen/include/asm-arm/vgic.h | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 1e5107b..38dacd3 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -69,6 +69,7 @@ void vgic_init_pending_irq(struct pending_irq *p, unsigned 
int virq)
     memset(p, 0, sizeof(*p));
     INIT_LIST_HEAD(&p->inflight);
     INIT_LIST_HEAD(&p->lr_queue);
+    spin_lock_init(&p->lock);
     p->irq = virq;
     p->lpi_vcpu_id = INVALID_VCPU_ID;
 }
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index d4ed23d..1c38b9a 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -90,6 +90,14 @@ struct pending_irq
      * TODO: when implementing irq migration, taking only the current
      * vgic lock is not going to be enough. */
     struct list_head lr_queue;
+    /* The lock protects the consistency of this structure. A single status bit
+     * can be read and/or set without holding the lock using the atomic
+     * set_bit/clear_bit/test_bit functions, however accessing multiple bits or
+     * relating to other members in this struct requires the lock.
+     * The list_head members are protected by their corresponding VCPU lock,
+     * it is not sufficient to hold this pending_irq lock here to query or
+     * change list order or affiliation. */

Actually, I have on question here. Do the vCPU lock sufficient to protect the list_head members. Or do you also mandate the pending_irq to be locked as well?

Also, it would be good to have the locking order documented maybe in docs/misc?

+    spinlock_t lock;
 };

 #define NR_INTERRUPT_PER_RANK   32
@@ -156,6 +164,9 @@ struct vgic_ops {
 #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
 #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)

+#define vgic_irq_lock(p, flags) spin_lock_irqsave(&(p)->lock, flags)
+#define vgic_irq_unlock(p, flags) spin_unlock_irqrestore(&(p)->lock, flags)
+
 #define vgic_lock_rank(v, r, flags)   spin_lock_irqsave(&(r)->lock, flags)
 #define vgic_unlock_rank(v, r, flags) spin_unlock_irqrestore(&(r)->lock, flags)



Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.