[PATCH v2 0/4] xen/arm: fix rank/vgic lock inversion bug

[PATCH v2 0/4] xen/arm: fix rank/vgic lock inversion bug

[Stefano Stabellini]

Hi all,

this patch series fixes a lock inversion bug in Xen on ARM. The locking
order is: first rank lock, then vgic lock. The order is respected
everywhere, except for gic_update_one_lr.

gic_update_one_lr is called with the vgic lock held, but it calls
vgic_get_target_vcpu, which tries to obtain the rank lock. This can
cause deadlocks.

The series fixes the issue by avoiding the rank lock in
gic_update_one_lr. It makes it safe by ensuring that the right vgic lock
is always taken by vgic_migrate_irq.

The first patch is just a fix to a bug I discovered while testing this
series.


Stefano Stabellini (4):
      xen/arm: fix GIC_INVALID_LR
      arm: store vcpu id in struct irq_pending
      arm,vgic_migrate_irq: take the right vgic lock
      The locking order is: first rank lock, then vgic lock. The order is     respected everywhere, except for gic_update_one_lr.

 xen/arch/arm/gic.c         | 14 ++++++++++--
 xen/arch/arm/vgic-v2.c     | 12 +++-------
 xen/arch/arm/vgic-v3.c     |  6 +----
 xen/arch/arm/vgic.c        | 56 +++++++++++++++++++++++++++++++++++++---------
 xen/include/asm-arm/vgic.h |  8 +++++--
 5 files changed, 68 insertions(+), 28 deletions(-)

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

[PATCH v2 1/4] xen/arm: fix GIC_INVALID_LR

[Stefano Stabellini]

GIC_INVALID_LR should be 0xff, but actually, defined as ~(uint8_t)0, is
0xffffffff. Fix the problem by placing the ~ operator before the cast.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/include/asm-arm/vgic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 672f649..467333c 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -69,7 +69,7 @@ struct pending_irq
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     unsigned int irq;
-#define GIC_INVALID_LR         ~(uint8_t)0
+#define GIC_INVALID_LR         (uint8_t)~0
     uint8_t lr;
     uint8_t priority;
     /* inflight is used to append instances of pending_irq to
-- 
1.9.1


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

[PATCH v2 2/4] arm: store vcpu id in struct irq_pending

[Stefano Stabellini]

We currently store the LR register number for every irq in irq_pending,
but we don't store the vcpu id. Thus, we know in which LR they are, but
we don't know the LR of which cpu. Fix this gap by adding a vcpu field
in struct pending_irq. Use the bytes that are currently used for
padding within the struct to avoid increasing the size of the struct.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic.c         | 7 ++++++-
 xen/arch/arm/vgic.c        | 2 ++
 xen/include/asm-arm/vgic.h | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a5348f2..3189693 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -379,6 +379,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
     p->lr = lr;
+    p->vcpu = current->vcpu_id;
 }
 
 static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
@@ -501,14 +502,17 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
              test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
              !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+        {
+            p->vcpu = GIC_INVALID_VCPU;
             gic_raise_guest_irq(v, irq, p->priority);
-        else {
+        } else {
             list_del_init(&p->inflight);
             if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
             {
                 struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
                 irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             }
+            p->vcpu = GIC_INVALID_VCPU;
         }
     }
 }
@@ -574,6 +578,7 @@ static void gic_restore_pending_irqs(struct vcpu *v)
 found:
             lr = p_r->lr;
             p_r->lr = GIC_INVALID_LR;
+            p_r->vcpu = GIC_INVALID_VCPU;
             set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
             clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
             gic_add_to_lr_pending(v, p_r);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 364d5f0..f2e3eda 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -66,6 +66,8 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
     INIT_LIST_HEAD(&p->inflight);
     INIT_LIST_HEAD(&p->lr_queue);
     p->irq = virq;
+    p->lr = GIC_INVALID_LR;
+    p->vcpu = GIC_INVALID_VCPU;
 }
 
 static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 467333c..fde5b32 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -72,6 +72,8 @@ struct pending_irq
 #define GIC_INVALID_LR         (uint8_t)~0
     uint8_t lr;
     uint8_t priority;
+#define GIC_INVALID_VCPU       (uint8_t)~0
+    uint8_t vcpu;
     /* inflight is used to append instances of pending_irq to
      * vgic.inflight_irqs */
     struct list_head inflight;
-- 
1.9.1


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

[PATCH v2 3/4] arm, vgic_migrate_irq: take the right vgic lock

[Stefano Stabellini]

Always take the vgic lock of the old vcpu. When more than one irq
migration is requested before the first one completes, take the vgic
lock of the oldest vcpu.

Write the new vcpu id into the rank from vgic_migrate_irq, protected by
the oldest vgic vcpu lock.

Use barriers to ensure proper ordering between clearing inflight and
MIGRATING and setting vcpu to GIC_INVALID_VCPU.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic.c         |  5 +++++
 xen/arch/arm/vgic-v2.c     | 12 +++--------
 xen/arch/arm/vgic-v3.c     |  6 +-----
 xen/arch/arm/vgic.c        | 50 +++++++++++++++++++++++++++++++++++++++-------
 xen/include/asm-arm/vgic.h |  3 ++-
 5 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3189693..51148b4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -512,6 +512,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
                 struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
                 irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             }
+            /* 
+             * Clear MIGRATING, set new affinity, then clear vcpu. This
+             * barrier pairs with the one in vgic_migrate_irq.
+             */
+            smp_mb();
             p->vcpu = GIC_INVALID_VCPU;
         }
     }
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 3dbcfe8..38b1be1 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -154,15 +154,9 @@ static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank,
 
         old_target = rank->vcpu[offset];
 
-        /* Only migrate the vIRQ if the target vCPU has changed */
-        if ( new_target != old_target )
-        {
-            vgic_migrate_irq(d->vcpu[old_target],
-                             d->vcpu[new_target],
-                             virq);
-        }
-
-        rank->vcpu[offset] = new_target;
+        vgic_migrate_irq(d->vcpu[old_target],
+                d->vcpu[new_target],
+                virq, &rank->vcpu[offset]);
     }
 }
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index d61479d..6fb0fdd 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -150,11 +150,7 @@ static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank,
     if ( !new_vcpu )
         return;
 
-    /* Only migrate the IRQ if the target vCPU has changed */
-    if ( new_vcpu != old_vcpu )
-        vgic_migrate_irq(old_vcpu, new_vcpu, virq);
-
-    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 f2e3eda..cceac24 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -257,9 +257,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)
+static void __vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 {
-    unsigned long flags;
     struct pending_irq *p = irq_to_pending(old, irq);
 
     /* nothing to do for virtual interrupts */
@@ -272,12 +271,9 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 
     perfc_incr(vgic_irq_migrates);
 
-    spin_lock_irqsave(&old->arch.vgic.lock, flags);
-
     if ( list_empty(&p->inflight) )
     {
         irq_set_affinity(p->desc, cpumask_of(new->processor));
-        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         return;
     }
     /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
@@ -287,7 +283,6 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
         list_del_init(&p->lr_queue);
         list_del_init(&p->inflight);
         irq_set_affinity(p->desc, cpumask_of(new->processor));
-        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         vgic_vcpu_inject_irq(new, irq);
         return;
     }
@@ -296,7 +291,48 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     if ( !list_empty(&p->inflight) )
         set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
 
-    spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+}
+
+void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq,
+        uint8_t *rank_vcpu)
+{
+    struct pending_irq *p;
+    unsigned long flags;
+    struct vcpu *v;
+    uint8_t vcpu;
+
+    /* Only migrate the IRQ if the target vCPU has changed */
+    if ( new == old )
+        return;
+
+    /* 
+     * In most cases, p->vcpu is either invalid or the same as "old".
+     * The only exceptions are cases where the interrupt has already
+     * been migrated to a different vcpu, but the irq migration is still
+     * in progress (GIC_IRQ_GUEST_MIGRATING has been set). If that is
+     * the case, then "old" points to an intermediary vcpu we don't care
+     * about. We want to take the lock on the older vcpu instead,
+     * because that is the one gic_update_one_lr holds.
+     *
+     * The vgic lock is the only lock protecting accesses to rank_vcpu
+     * from gic_update_one_lr. However, writes to rank_vcpu are still
+     * protected by the rank lock.
+     */
+    p = irq_to_pending(old, irq);
+    vcpu = p->vcpu;
+
+    /* This pairs with the barrier in gic_update_one_lr. */
+    smp_mb();
+
+    if ( vcpu != GIC_INVALID_VCPU )
+        v = old->domain->vcpu[vcpu];
+    else
+        v = old;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+    __vgic_migrate_irq(old, new, irq);
+    *rank_vcpu = new->vcpu_id;
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 void arch_move_irqs(struct vcpu *v)
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index fde5b32..dce2f84 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 *rank_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@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 4/4] The locking order is: first rank lock, then vgic lock. The order is respected everywhere, except for gic_update_one_lr.

[Stefano Stabellini]

gic_update_one_lr is called with the vgic lock held, but it calls
vgic_get_target_vcpu, which tries to obtain the rank lock. This can
cause deadlocks.

We already have a version of vgic_get_target_vcpu that doesn't take the
rank lock: __vgic_get_target_vcpu.

Solve the lock inversion problem, by not taking the rank lock in
gic_update_one_lr (calling __vgic_get_target_vcpu instead of
vgic_get_target_vcpu).  This is safe, because vcpu target modifications
are protected by the same vgic vcpu lock.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic.c         | 2 +-
 xen/arch/arm/vgic.c        | 4 +---
 xen/include/asm-arm/vgic.h | 1 +
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 51148b4..28ab2f9 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -509,7 +509,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
             list_del_init(&p->inflight);
             if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
             {
-                struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
+                struct vcpu *v_target = __vgic_get_target_vcpu(v, irq);
                 irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             }
             /* 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index cceac24..c89b85f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -221,12 +221,10 @@ int vcpu_vgic_free(struct vcpu *v)
 }
 
 /* The function should be called by rank lock taken. */
-static struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
+struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
 {
     struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
 
-    ASSERT(spin_is_locked(&rank->lock));
-
     return v->domain->vcpu[rank->vcpu[virq & INTERRUPT_RANK_MASK]];
 }
 
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index dce2f84..26594b0 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -295,6 +295,7 @@ extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
 extern void domain_vgic_free(struct domain *d);
 extern int vcpu_vgic_init(struct vcpu *v);
 extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
+extern struct vcpu *__vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
 extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
 extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
-- 
1.9.1


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

Re: [PATCH v2 2/4] arm: store vcpu id in struct irq_pending

[Andrew Cooper]

On 22/12/16 02:15, Stefano Stabellini wrote:
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 467333c..fde5b32 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -72,6 +72,8 @@ struct pending_irq
>   #define GIC_INVALID_LR         (uint8_t)~0
>       uint8_t lr;
>       uint8_t priority;
> +#define GIC_INVALID_VCPU       (uint8_t)~0
> +    uint8_t vcpu;

You probably want a BUILD_BUG_ON() somewhere to trip when the maximum 
number of domain vcpus exceeds 255.

~Andrew

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

Re: [PATCH v2 3/4] arm, vgic_migrate_irq: take the right vgic lock

[Julien Grall]

Hi Stefano,

On 22/12/16 02:15, Stefano Stabellini wrote:
> Always take the vgic lock of the old vcpu. When more than one irq
> migration is requested before the first one completes, take the vgic
> lock of the oldest vcpu.
>
> Write the new vcpu id into the rank from vgic_migrate_irq, protected by
> the oldest vgic vcpu lock.
>
> Use barriers to ensure proper ordering between clearing inflight and
> MIGRATING and setting vcpu to GIC_INVALID_VCPU.

The field p->status was designed to be accessed without any lock using 
*_bit helpers.

Currently vgic_migrate_irq is protected by the rank lock (an ASSERT 
would probably useful for documentation) and can only be called once at 
the time.

Let's take the following example to describe the problem:
     1) IRQ has been injected into vCPU A (e.g present in the LR)
     2) IRQ is migrated to vCPU B
     3) IRQ is migrated to vCPU C
     4) IRQ is EOIed by the guest

When vgic_irq_migrate_irq is called for the first time (step 2), the 
vCPU A vgic lock will be taken and GIC_IRQ_GUEST_MIGRATED will be set at 
the end. The second time (step 3), GIC_IRQ_GUEST_MIGRATING is already 
set, so the function will return directly no lock needed.

So, I think the vgic lock taken is already correct.

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 4/4] The locking order is: first rank lock, then vgic lock. The order is respected everywhere, except for gic_update_one_lr.

[Julien Grall]

Hi Stefano,

On 22/12/16 02:15, Stefano Stabellini wrote:
> gic_update_one_lr is called with the vgic lock held, but it calls
> vgic_get_target_vcpu, which tries to obtain the rank lock. This can
> cause deadlocks.
>
> We already have a version of vgic_get_target_vcpu that doesn't take the
> rank lock: __vgic_get_target_vcpu.
>
> Solve the lock inversion problem, by not taking the rank lock in
> gic_update_one_lr (calling __vgic_get_target_vcpu instead of
> vgic_get_target_vcpu).  This is safe, because vcpu target modifications
> are protected by the same vgic vcpu lock.

I maintain what I said on the first version of this patch. All this 
patch could be simplified by moving the call to irq_set_affinity in 
vgic_irq_migrate.

There are no restriction to do that and no need to have any lock taken 
but the rank lock.

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 1/4] xen/arm: fix GIC_INVALID_LR

[Julien Grall]

Hi Stefano,

On 22/12/16 02:15, Stefano Stabellini wrote:
> GIC_INVALID_LR should be 0xff, but actually, defined as ~(uint8_t)0, is
> 0xffffffff. Fix the problem by placing the ~ operator before the cast.
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 4/4] The locking order is: first rank lock, then vgic lock. The order is respected everywhere, except for gic_update_one_lr.

[Stefano Stabellini]

On Wed, 28 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/12/16 02:15, Stefano Stabellini wrote:
> > gic_update_one_lr is called with the vgic lock held, but it calls
> > vgic_get_target_vcpu, which tries to obtain the rank lock. This can
> > cause deadlocks.
> > 
> > We already have a version of vgic_get_target_vcpu that doesn't take the
> > rank lock: __vgic_get_target_vcpu.
> > 
> > Solve the lock inversion problem, by not taking the rank lock in
> > gic_update_one_lr (calling __vgic_get_target_vcpu instead of
> > vgic_get_target_vcpu).  This is safe, because vcpu target modifications
> > are protected by the same vgic vcpu lock.
> 
> I maintain what I said on the first version of this patch. All this patch
> could be simplified by moving the call to irq_set_affinity in
> vgic_irq_migrate.
> 
> There are no restriction to do that and no need to have any lock taken but the
> rank lock.
 
All right. I'll submit a patch that does exactly that. It is not
perfect, because it drops interrupts in the problematic scenario, but it
should be a good place to start for a technical discussion.

If it turns out that this is the best approach, we can come back to it.

Cheers,

Stefano

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

Re: [PATCH v2 1/4] xen/arm: fix GIC_INVALID_LR

[Stefano Stabellini]

On Wed, 28 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/12/16 02:15, Stefano Stabellini wrote:
> > GIC_INVALID_LR should be 0xff, but actually, defined as ~(uint8_t)0, is
> > 0xffffffff. Fix the problem by placing the ~ operator before the cast.
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Thanks, I committed the fix

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

Re: [PATCH v2 3/4] arm, vgic_migrate_irq: take the right vgic lock

[Stefano Stabellini]

On Wed, 28 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/12/16 02:15, Stefano Stabellini wrote:
> > Always take the vgic lock of the old vcpu. When more than one irq
> > migration is requested before the first one completes, take the vgic
> > lock of the oldest vcpu.
> > 
> > Write the new vcpu id into the rank from vgic_migrate_irq, protected by
> > the oldest vgic vcpu lock.
> > 
> > Use barriers to ensure proper ordering between clearing inflight and
> > MIGRATING and setting vcpu to GIC_INVALID_VCPU.
> 
> The field p->status was designed to be accessed without any lock using *_bit
> helpers.
> 
> Currently vgic_migrate_irq is protected by the rank lock (an ASSERT would
> probably useful for documentation) and can only be called once at the time.
> 
> Let's take the following example to describe the problem:
>     1) IRQ has been injected into vCPU A (e.g present in the LR)
>     2) IRQ is migrated to vCPU B
>     3) IRQ is migrated to vCPU C
>     4) IRQ is EOIed by the guest
> 
> When vgic_irq_migrate_irq is called for the first time (step 2), the vCPU A
> vgic lock will be taken and GIC_IRQ_GUEST_MIGRATED will be set at the end. The
> second time (step 3), GIC_IRQ_GUEST_MIGRATING is already set, so the function
> will return directly no lock needed.

Right, but the caller, vgic_store_itargetsr, will still write to
rank->vcpu.


> So, I think the vgic lock taken is already correct.

The problem arises by 3) and 4) running simultaneously, and specifically
from the rank->vcpu write in vgic_store_itargetsr running concurrently
with gic_update_one_lr, as described in
alpine.DEB.2.10.1612191337370.13831@sstabellini-ThinkPad-X260:

  CPU0                                  CPU1
  <GIC_IRQ_GUEST_MIGRATING is set>      <GIC_IRQ_GUEST_MIGRATING is set>
  ----------------------------------------------------------------------
                                        vgic_migrate_irq C (does nothing)
  clear GIC_IRQ_GUEST_MIGRATING
  read rank->vcpu B
                                        set rank->vcpu C
  irq_set_affinity B


Result: the irq physical affinity is B instead of C.

Please note that the new patch
(1483486167-24607-1-git-send-email-sstabellini@kernel.org) doesn't have
this problem because it removes the call to irq_set_affinity in
gic_update_one_lr.

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

Re: [PATCH v2 3/4] arm, vgic_migrate_irq: take the right vgic lock

[Julien Grall]

Hi Stefano,

On 03/01/17 23:30, Stefano Stabellini wrote:
> On Wed, 28 Dec 2016, Julien Grall wrote:
>> On 22/12/16 02:15, Stefano Stabellini wrote:
>>> Always take the vgic lock of the old vcpu. When more than one irq
>>> migration is requested before the first one completes, take the vgic
>>> lock of the oldest vcpu.
>>>
>>> Write the new vcpu id into the rank from vgic_migrate_irq, protected by
>>> the oldest vgic vcpu lock.
>>>
>>> Use barriers to ensure proper ordering between clearing inflight and
>>> MIGRATING and setting vcpu to GIC_INVALID_VCPU.
>>
>> The field p->status was designed to be accessed without any lock using *_bit
>> helpers.
>>
>> Currently vgic_migrate_irq is protected by the rank lock (an ASSERT would
>> probably useful for documentation) and can only be called once at the time.
>>
>> Let's take the following example to describe the problem:
>>     1) IRQ has been injected into vCPU A (e.g present in the LR)
>>     2) IRQ is migrated to vCPU B
>>     3) IRQ is migrated to vCPU C
>>     4) IRQ is EOIed by the guest
>>
>> When vgic_irq_migrate_irq is called for the first time (step 2), the vCPU A
>> vgic lock will be taken and GIC_IRQ_GUEST_MIGRATED will be set at the end. The
>> second time (step 3), GIC_IRQ_GUEST_MIGRATING is already set, so the function
>> will return directly no lock needed.
>
> Right, but the caller, vgic_store_itargetsr, will still write to
> rank->vcpu.
>
>
>> So, I think the vgic lock taken is already correct.
>
> The problem arises by 3) and 4) running simultaneously, and specifically
> from the rank->vcpu write in vgic_store_itargetsr running concurrently
> with gic_update_one_lr, as described in
> alpine.DEB.2.10.1612191337370.13831@sstabellini-ThinkPad-X260:
>
>   CPU0                                  CPU1
>   <GIC_IRQ_GUEST_MIGRATING is set>      <GIC_IRQ_GUEST_MIGRATING is set>
>   ----------------------------------------------------------------------
>                                         vgic_migrate_irq C (does nothing)
>   clear GIC_IRQ_GUEST_MIGRATING
>   read rank->vcpu B
>                                         set rank->vcpu C
>   irq_set_affinity B
>
>
> Result: the irq physical affinity is B instead of C.
>
> Please note that the new patch
> (1483486167-24607-1-git-send-email-sstabellini@kernel.org) doesn't have
> this problem because it removes the call to irq_set_affinity in
> gic_update_one_lr.

Sorry I think there was a bit of confusion with my previous e-mail. I 
was describing the behavior with a change in the code similar to your v3.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 4/4] The locking order is: first rank lock, then vgic lock. The order is respected everywhere, except for gic_update_one_lr.

[Julien Grall]

Hi Stefano,

On 03/01/17 22:51, Stefano Stabellini wrote:
> On Wed, 28 Dec 2016, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/12/16 02:15, Stefano Stabellini wrote:
>>> gic_update_one_lr is called with the vgic lock held, but it calls
>>> vgic_get_target_vcpu, which tries to obtain the rank lock. This can
>>> cause deadlocks.
>>>
>>> We already have a version of vgic_get_target_vcpu that doesn't take the
>>> rank lock: __vgic_get_target_vcpu.
>>>
>>> Solve the lock inversion problem, by not taking the rank lock in
>>> gic_update_one_lr (calling __vgic_get_target_vcpu instead of
>>> vgic_get_target_vcpu).  This is safe, because vcpu target modifications
>>> are protected by the same vgic vcpu lock.
>>
>> I maintain what I said on the first version of this patch. All this patch
>> could be simplified by moving the call to irq_set_affinity in
>> vgic_irq_migrate.
>>
>> There are no restriction to do that and no need to have any lock taken but the
>> rank lock.
>
> All right. I'll submit a patch that does exactly that. It is not
> perfect, because it drops interrupts in the problematic scenario, but it
> should be a good place to start for a technical discussion.

I think I have missed something. What is the problematic scenario you 
are describing? Looking at the v3, I don't see any mention of this 
problem. Does it mean it has been solved?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 4/4] The locking order is: first rank lock, then vgic lock. The order is respected everywhere, except for gic_update_one_lr.

[Stefano Stabellini]

On Mon, 16 Jan 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/01/17 22:51, Stefano Stabellini wrote:
> > On Wed, 28 Dec 2016, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 22/12/16 02:15, Stefano Stabellini wrote:
> > > > gic_update_one_lr is called with the vgic lock held, but it calls
> > > > vgic_get_target_vcpu, which tries to obtain the rank lock. This can
> > > > cause deadlocks.
> > > > 
> > > > We already have a version of vgic_get_target_vcpu that doesn't take the
> > > > rank lock: __vgic_get_target_vcpu.
> > > > 
> > > > Solve the lock inversion problem, by not taking the rank lock in
> > > > gic_update_one_lr (calling __vgic_get_target_vcpu instead of
> > > > vgic_get_target_vcpu).  This is safe, because vcpu target modifications
> > > > are protected by the same vgic vcpu lock.
> > > 
> > > I maintain what I said on the first version of this patch. All this patch
> > > could be simplified by moving the call to irq_set_affinity in
> > > vgic_irq_migrate.
> > > 
> > > There are no restriction to do that and no need to have any lock taken but
> > > the
> > > rank lock.
> > 
> > All right. I'll submit a patch that does exactly that. It is not
> > perfect, because it drops interrupts in the problematic scenario, but it
> > should be a good place to start for a technical discussion.
> 
> I think I have missed something. What is the problematic scenario you are
> describing? Looking at the v3, I don't see any mention of this problem. Does
> it mean it has been solved?

No, it hasn't been solved. Sorry for not having been clearer here. The
issue is described in the commit message of
http://marc.info/?l=xen-devel&m=148348625720995:

  After migrating an interrupt from vcpu/pcpu 0 to vcpu/pcpu 1, it is
  possible to receive a physical interrupt on pcpu 1, which Xen is
  supposed to inject into vcpu 1, before the LR on pcpu 0 has been
  cleared. In this case the irq is still marked as
  GIC_IRQ_GUEST_MIGRATING, and struct pending_irq is still "inflight" on
  vcpu 0. As the irq cannot be "inflight" on vcpu 0 and vcpu 1
  simultaneously, drop the interrupt.

This is not the right behavior: the interrupt should be injected into
vcpu 1.

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

Re: [PATCH v2 4/4] The locking order is: first rank lock, then vgic lock. The order is respected everywhere, except for gic_update_one_lr.

[Julien Grall]

Hi Stefano,

On 16/01/2017 19:10, Stefano Stabellini wrote:
> On Mon, 16 Jan 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/01/17 22:51, Stefano Stabellini wrote:
>>> On Wed, 28 Dec 2016, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 22/12/16 02:15, Stefano Stabellini wrote:
>>>>> gic_update_one_lr is called with the vgic lock held, but it calls
>>>>> vgic_get_target_vcpu, which tries to obtain the rank lock. This can
>>>>> cause deadlocks.
>>>>>
>>>>> We already have a version of vgic_get_target_vcpu that doesn't take the
>>>>> rank lock: __vgic_get_target_vcpu.
>>>>>
>>>>> Solve the lock inversion problem, by not taking the rank lock in
>>>>> gic_update_one_lr (calling __vgic_get_target_vcpu instead of
>>>>> vgic_get_target_vcpu).  This is safe, because vcpu target modifications
>>>>> are protected by the same vgic vcpu lock.
>>>>
>>>> I maintain what I said on the first version of this patch. All this patch
>>>> could be simplified by moving the call to irq_set_affinity in
>>>> vgic_irq_migrate.
>>>>
>>>> There are no restriction to do that and no need to have any lock taken but
>>>> the
>>>> rank lock.
>>>
>>> All right. I'll submit a patch that does exactly that. It is not
>>> perfect, because it drops interrupts in the problematic scenario, but it
>>> should be a good place to start for a technical discussion.
>>
>> I think I have missed something. What is the problematic scenario you are
>> describing? Looking at the v3, I don't see any mention of this problem. Does
>> it mean it has been solved?
>
> No, it hasn't been solved. Sorry for not having been clearer here. The
> issue is described in the commit message of
> http://marc.info/?l=xen-devel&m=148348625720995:
>
>   After migrating an interrupt from vcpu/pcpu 0 to vcpu/pcpu 1, it is
>   possible to receive a physical interrupt on pcpu 1, which Xen is
>   supposed to inject into vcpu 1, before the LR on pcpu 0 has been
>   cleared. In this case the irq is still marked as
>   GIC_IRQ_GUEST_MIGRATING, and struct pending_irq is still "inflight" on
>   vcpu 0. As the irq cannot be "inflight" on vcpu 0 and vcpu 1
>   simultaneously, drop the interrupt.
>
> This is not the right behavior: the interrupt should be injected into
> vcpu 1.

I haven't yet reviewed v3, hence why I missed this paragraph, sorry. As 
you suggested on an earlier mail, I will start the discussion there.

Cheers,

-- 
Julien Grall

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