[Xen-devel] [PATCH v5 2/3] xen/arm: move setting of new target vcpu to vgic_migrate_irq

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Move the atomic write of rank->vcpu, which sets the new vcpu target, to
vgic_migrate_irq, at the beginning of the lock protected area (protected
by the vgic lock).

This code movement reduces race conditions between vgic_migrate_irq and
setting rank->vcpu on one pcpu and gic_update_one_lr on another pcpu.

When gic_update_one_lr and vgic_migrate_irq take the same vgic lock,
there are no more race conditions with this patch. When vgic_migrate_irq
is called multiple times while GIC_IRQ_GUEST_MIGRATING is already set, a
race condition still exists because in that case gic_update_one_lr and
vgic_migrate_irq take different vgic locks.

Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
---
 xen/arch/arm/vgic-v2.c     |  5 ++---
 xen/arch/arm/vgic-v3.c     |  4 +---
 xen/arch/arm/vgic.c        | 15 ++++++++++-----
 xen/include/asm-arm/vgic.h |  3 ++-
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 0674f7b..43b4ac3 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -158,10 +158,9 @@ static void vgic_store_itargetsr(struct domain *d, struct 
vgic_irq_rank *rank,
         {
             vgic_migrate_irq(d->vcpu[old_target],
                              d->vcpu[new_target],
-                             virq);
+                             virq,
+                             &rank->vcpu[offset]);
         }
-
-        write_atomic(&rank->vcpu[offset], new_target);
     }
 }
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 0679e76..a6fc6cb 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -151,9 +151,7 @@ static void vgic_store_irouter(struct domain *d, struct 
vgic_irq_rank *rank,
 
     /* Only migrate the IRQ if the target vCPU has changed */
     if ( new_vcpu != old_vcpu )
-        vgic_migrate_irq(old_vcpu, new_vcpu, virq);
-
-    write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id);
+        vgic_migrate_irq(old_vcpu, new_vcpu, virq, &rank->vcpu[offset]);
 }
 
 static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 67d75a6..a323e7e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -237,7 +237,8 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned 
int virq)
     return priority;
 }
 
-void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
+void vgic_migrate_irq(struct vcpu *old, struct vcpu *new,
+                      unsigned int irq, uint8_t *t_vcpu)
 {
     unsigned long flags;
     struct pending_irq *p = irq_to_pending(old, irq);
@@ -246,13 +247,17 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, 
unsigned int irq)
     if ( p->desc == NULL )
         return;
 
-    /* migration already in progress, no need to do anything */
-    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
-        return;
-
     perfc_incr(vgic_irq_migrates);
 
     spin_lock_irqsave(&old->arch.vgic.lock, flags);
+    write_atomic(t_vcpu, new->vcpu_id);
+
+    /* migration already in progress, no need to do anything */
+    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+    {
+        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        return;
+    }
 
     if ( list_empty(&p->inflight) )
     {
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 894c3f1..5d8b8c8 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -314,7 +314,8 @@ extern int vcpu_vgic_free(struct vcpu *v);
 extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
                         enum gic_sgi_mode irqmode, int virq,
                         const struct sgi_target *target);
-extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int 
irq);
+extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int 
irq,
+                             uint8_t *t_vcpu);
 
 /* Reserve a specific guest vIRQ */
 extern bool vgic_reserve_virq(struct domain *d, unsigned int virq);
-- 
1.9.1


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