[PATCH v5 0/3] xen/arm: remove race conditions in irq migration

[PATCH v5 0/3] xen/arm: remove race conditions in irq migration

[Stefano Stabellini]

Hi all,

this patch series removes three race conditions affecting the current
code base.

The first race condition is between gic_update_one_lr and
vgic_vcpu_inject_irq: as soon as gic_update_one_lr calls
irq_set_affinity a new interrupt could be injected in the new pcpu,
eventually vgic_vcpu_inject_irq is called which manipulates the inflight
list. The first patch solves this race by adding a barrier in
gic_update_one_lr. This patch was suggested by Julien.

The second race condition happens when gic_update_one_lr runs
simultaneously with vgic_store_itargetsr and vgic_migrate_irq. Setting
the new target is done after calling vgic_migrate_irq, which means that
gic_update_one_lr could end up setting the physical affinity to the one
of the old pcpu. The second patch solves the problem by moving the code
to set the new vcpu target from vgic_store_itargetsr to
vgic_migrate_irq. The code is still not entirely safe due to the last
race condition.

The third race condition happens again between gic_update_one_lr and
vgic_migrate_irq: when GIC_IRQ_GUEST_MIGRATING is already set and
vgic_migrate_irq is called again, it will take a different vgic lock
from the one that gic_update_one_lr is taking. The third patch addresses
this problem by adding barriers and busy waits to make sure that
vgic_migrate_irq and gic_update_one_lr can run safely at the same time,
even without taking the same lock.

For your reference, it is not possible to take the p->desc lock from
gic_update_one_lr, because the correct lock ordering is p->desc lock,
then vgic lock.


Stefano Stabellini (3):
      arm: remove irq from inflight, then change physical affinity
      xen/arm: move setting of new target vcpu to vgic_migrate_irq
      xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

 xen/arch/arm/gic.c         | 10 +++++++++-
 xen/arch/arm/vgic-v2.c     |  5 ++---
 xen/arch/arm/vgic-v3.c     |  4 +---
 xen/arch/arm/vgic.c        | 27 ++++++++++++++++++++++-----
 xen/include/asm-arm/vgic.h |  3 ++-
 5 files changed, 36 insertions(+), 13 deletions(-)

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

[PATCH v5 1/3] arm: remove irq from inflight, then change physical affinity

[Stefano Stabellini]

This patch fixes a potential race that could happen when
gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously.

When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq has
been removed from inflight before changing physical affinity, to avoid
concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take a
different vcpu lock.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 9522c6c..16bb150 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -503,6 +503,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
             gic_raise_guest_irq(v, irq, p->priority);
         else {
             list_del_init(&p->inflight);
+            /* Remove from inflight, then change physical affinity. It
+             * makes sure that when a new interrupt is received on the
+             * next pcpu, inflight is already cleared. No concurrent
+             * accesses to inflight. */
+            smp_mb();
             if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
             {
                 struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
-- 
1.9.1


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

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

[Stefano Stabellini]

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

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

[Julien Grall]

Hi Stefano,

On 01/03/17 22:15, Stefano Stabellini wrote:
> 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@kernel.org>
> ---
>  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);

With this change rank->vcpu[offset] will not be updated for virtual SPIs 
(e.g p->desc != NULL). And therefore affinity for them will not work.

However, from my understanding the problem you are trying to solve with 
this patch is having rank->vcpu[offset] to be set as soon as possible. 
It does not really matter if it is protected by the lock, what you care 
is rank->vcpu[offset] been seen before the lock has been released.

So if GIC_IRQ_GUEST_MIGRATE is set and gic_update_one_lr is running 
straight after the lock is released, rank->vcpu[offset] will contain the 
correct vCPU.

A better approach would be to move write_atomic(...) before 
vgic_migrated_irq(...). What do you think?

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Fri, 3 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/03/17 22:15, Stefano Stabellini wrote:
> > 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@kernel.org>
> > ---
> >  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);
> 
> With this change rank->vcpu[offset] will not be updated for virtual SPIs (e.g
> p->desc != NULL). And therefore affinity for them will not work.

Do you mean p->desc == NULL? I don't think we have any purely virtual
SPIs yet, only virtual PPIs (where the target cannot be changed).
However, I think you are right: moving it before the call would also
work and it's simpler. I'll do that.


> However, from my understanding the problem you are trying to solve with this
> patch is having rank->vcpu[offset] to be set as soon as possible. It does not
> really matter if it is protected by the lock, what you care is
> rank->vcpu[offset] been seen before the lock has been released.
> 
> So if GIC_IRQ_GUEST_MIGRATE is set and gic_update_one_lr is running straight
> after the lock is released, rank->vcpu[offset] will contain the correct vCPU.
> 
> A better approach would be to move write_atomic(...) before
> vgic_migrated_irq(...). What do you think?



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

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

[Julien Grall]

Hi Stefano,

On 30/03/17 00:48, Stefano Stabellini wrote:
> On Fri, 3 Mar 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 01/03/17 22:15, Stefano Stabellini wrote:
>>> 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@kernel.org>
>>> ---
>>>  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);
>>
>> With this change rank->vcpu[offset] will not be updated for virtual SPIs (e.g
>> p->desc != NULL). And therefore affinity for them will not work.
>
> Do you mean p->desc == NULL? I don't think we have any purely virtual
> SPIs yet, only virtual PPIs (where the target cannot be changed).

Yes I meant p->desc == NULL. That's correct we don't have any but the 
vgic code should be able to handle both virtual and physical SPIs.
We will support virtual SPIs soon (see PL011 work) and it will be 
difficult to find the root of a such bug.

> However, I think you are right: moving it before the call would also
> work and it's simpler. I'll do that.

Thank you!

Cheers,

-- 
Julien Grall

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

[PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Stefano Stabellini]

A potential race condition occurs when vgic_migrate_irq is called a
second time, while GIC_IRQ_GUEST_MIGRATING is already set. In that case,
vgic_migrate_irq takes a different vgic lock from gic_update_one_lr.
vgic_migrate_irq running concurrently with gic_update_one_lr could cause
data corruptions, as they both access the inflight list.

This patch fixes this problem. In vgic_migrate_irq after setting the new
vcpu target, it checks both GIC_IRQ_GUEST_MIGRATING and
GIC_IRQ_GUEST_VISIBLE. If they are both set we can just return because
we have already set the new target: when gic_update_one_lr reaches
the GIC_IRQ_GUEST_MIGRATING test, it will do the right thing.

Otherwise, if GIC_IRQ_GUEST_MIGRATING is set but GIC_IRQ_GUEST_VISIBLE
is not, gic_update_one_lr is running at the very same time on another
pcpu, so it just waits until it completes (GIC_IRQ_GUEST_MIGRATING is
cleared).

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 16bb150..a805300 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -508,10 +508,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
              * next pcpu, inflight is already cleared. No concurrent
              * accesses to inflight. */
             smp_mb();
-            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+            if ( test_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));
+                /* Set the new affinity, then clear MIGRATING. */
+                smp_mb();
+                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
             }
         }
     }
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index a323e7e..9141b34 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -252,13 +252,25 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new,
     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) )
+    /* Set the new target, then check MIGRATING and VISIBLE, it pairs
+     * with the barrier in gic_update_one_lr. */
+    smp_mb();
+
+    /* no need to do anything, gic_update_one_lr will take care of it */
+    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
+         test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
     {
         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         return;
     }
 
+    /* gic_update_one_lr is currently running, wait until its completion */
+    while ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+    {
+        cpu_relax();
+        smp_rmb();
+    }
+
     if ( list_empty(&p->inflight) )
     {
         irq_set_affinity(p->desc, cpumask_of(new->processor));
-- 
1.9.1


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

Re: [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Julien Grall]

Hi Stefano,

On 01/03/17 22:15, Stefano Stabellini wrote:
> A potential race condition occurs when vgic_migrate_irq is called a
> second time, while GIC_IRQ_GUEST_MIGRATING is already set. In that case,
> vgic_migrate_irq takes a different vgic lock from gic_update_one_lr.

Hmmm, vgic_migrate_irq will bail out before accessing inflight list if 
GIC_IRQ_GUEST_MIGRATING is already set:

/* migrating already in progress, no need to do anything */
if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status )
   return;

And test_bit is atomic. So I don't understand what is the corruption 
problem you mention.

> vgic_migrate_irq running concurrently with gic_update_one_lr could cause
> data corruptions, as they both access the inflight list.
>
> This patch fixes this problem. In vgic_migrate_irq after setting the new
> vcpu target, it checks both GIC_IRQ_GUEST_MIGRATING and
> GIC_IRQ_GUEST_VISIBLE. If they are both set we can just return because
> we have already set the new target: when gic_update_one_lr reaches
> the GIC_IRQ_GUEST_MIGRATING test, it will do the right thing.
>
> Otherwise, if GIC_IRQ_GUEST_MIGRATING is set but GIC_IRQ_GUEST_VISIBLE
> is not, gic_update_one_lr is running at the very same time on another
> pcpu, so it just waits until it completes (GIC_IRQ_GUEST_MIGRATING is
> cleared).
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/arch/arm/gic.c  |  5 ++++-
>  xen/arch/arm/vgic.c | 16 ++++++++++++++--
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 16bb150..a805300 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -508,10 +508,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>               * next pcpu, inflight is already cleared. No concurrent
>               * accesses to inflight. */
>              smp_mb();
> -            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +            if ( test_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));
> +                /* Set the new affinity, then clear MIGRATING. */
> +                smp_mb();
> +                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
>              }
>          }
>      }
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a323e7e..9141b34 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -252,13 +252,25 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new,
>      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) )
> +    /* Set the new target, then check MIGRATING and VISIBLE, it pairs
> +     * with the barrier in gic_update_one_lr. */
> +    smp_mb();
> +
> +    /* no need to do anything, gic_update_one_lr will take care of it */
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
> +         test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>      {
>          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>          return;
>      }
>
> +    /* gic_update_one_lr is currently running, wait until its completion */
> +    while ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +    {
> +        cpu_relax();
> +        smp_rmb();
> +    }
> +
>      if ( list_empty(&p->inflight) )
>      {
>          irq_set_affinity(p->desc, cpumask_of(new->processor));
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Stefano Stabellini]

On Fri, 3 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/03/17 22:15, Stefano Stabellini wrote:
> > A potential race condition occurs when vgic_migrate_irq is called a
> > second time, while GIC_IRQ_GUEST_MIGRATING is already set. In that case,
> > vgic_migrate_irq takes a different vgic lock from gic_update_one_lr.
> 
> Hmmm, vgic_migrate_irq will bail out before accessing inflight list if
> GIC_IRQ_GUEST_MIGRATING is already set:
> 
> /* migrating already in progress, no need to do anything */
> if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status )
>   return;
> 
> And test_bit is atomic. So I don't understand what is the corruption problem
> you mention.

The scenario is a bit convoluted: GIC_IRQ_GUEST_MIGRATING is already set
and vgic_migrate_irq is called to move the irq again, even though the
first migration is not complete yet. This could happen:


      CPU 0                                    CPU 1
  gic_update_one_lr
  test_and_clear_bit MIGRATING
  read target (old)
                                            write target (new)
                                            vgic_migrate_irq
                                              test_bit MIGRATING
                                              irq_set_affinity (new)
                                              return
  irq_set_affinity (old)


After this patch this would happen:

      CPU 0                                    CPU 1
  gic_update_one_lr
  test_bit MIGRATING
  read target (old)
                                            write target (new)
                                            vgic_migrate_irq
                                              test MIGRATING && GIC_IRQ_GUEST_VISIBLE (false)
                                              wait until !MIGRATING
  irq_set_affinity (old)
  clear_bit MIGRATING
                                              irq_set_affinity (new)


> > vgic_migrate_irq running concurrently with gic_update_one_lr could cause
> > data corruptions, as they both access the inflight list.
> > 
> > This patch fixes this problem. In vgic_migrate_irq after setting the new
> > vcpu target, it checks both GIC_IRQ_GUEST_MIGRATING and
> > GIC_IRQ_GUEST_VISIBLE. If they are both set we can just return because
> > we have already set the new target: when gic_update_one_lr reaches
> > the GIC_IRQ_GUEST_MIGRATING test, it will do the right thing.
> > 
> > Otherwise, if GIC_IRQ_GUEST_MIGRATING is set but GIC_IRQ_GUEST_VISIBLE
> > is not, gic_update_one_lr is running at the very same time on another
> > pcpu, so it just waits until it completes (GIC_IRQ_GUEST_MIGRATING is
> > cleared).
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> >  xen/arch/arm/gic.c  |  5 ++++-
> >  xen/arch/arm/vgic.c | 16 ++++++++++++++--
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 16bb150..a805300 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -508,10 +508,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >               * next pcpu, inflight is already cleared. No concurrent
> >               * accesses to inflight. */
> >              smp_mb();
> > -            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> > +            if ( test_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));
> > +                /* Set the new affinity, then clear MIGRATING. */
> > +                smp_mb();
> > +                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
> >              }
> >          }
> >      }
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index a323e7e..9141b34 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -252,13 +252,25 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu
> > *new,
> >      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) )
> > +    /* Set the new target, then check MIGRATING and VISIBLE, it pairs
> > +     * with the barrier in gic_update_one_lr. */
> > +    smp_mb();
> > +
> > +    /* no need to do anything, gic_update_one_lr will take care of it */
> > +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
> > +         test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> >      {
> >          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> >          return;
> >      }
> > 
> > +    /* gic_update_one_lr is currently running, wait until its completion */
> > +    while ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> > +    {
> > +        cpu_relax();
> > +        smp_rmb();
> > +    }
> > +
> >      if ( list_empty(&p->inflight) )
> >      {
> >          irq_set_affinity(p->desc, cpumask_of(new->processor));
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Julien Grall]

Hi Stefano,

On 30/03/17 00:47, Stefano Stabellini wrote:
> On Fri, 3 Mar 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 01/03/17 22:15, Stefano Stabellini wrote:
>>> A potential race condition occurs when vgic_migrate_irq is called a
>>> second time, while GIC_IRQ_GUEST_MIGRATING is already set. In that case,
>>> vgic_migrate_irq takes a different vgic lock from gic_update_one_lr.
>>
>> Hmmm, vgic_migrate_irq will bail out before accessing inflight list if
>> GIC_IRQ_GUEST_MIGRATING is already set:
>>
>> /* migrating already in progress, no need to do anything */
>> if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status )
>>   return;
>>
>> And test_bit is atomic. So I don't understand what is the corruption problem
>> you mention.
>
> The scenario is a bit convoluted: GIC_IRQ_GUEST_MIGRATING is already set
> and vgic_migrate_irq is called to move the irq again, even though the
> first migration is not complete yet.

What you described is not a data corruption to me. The host IRQ will be 
routed to the wrong pCPU and then what? The IRQ will still trigger, ok 
on the wrong pCPU, it will be slower but we are capable to handle that.

The use case you describe would only happen if a guest is trying to 
change the routing multiple times while an interrupt is pending. So to 
be honest, a sane guest would not do that. But this would only affect 
stupid guest.

So I don't think this is worth to support considering how this patch 
will increase the code complexity in a component that is already a 
nightmare to handle.

> This could happen:
>
>
>       CPU 0                                    CPU 1
>   gic_update_one_lr
>   test_and_clear_bit MIGRATING
>   read target (old)
>                                             write target (new)
>                                             vgic_migrate_irq
>                                               test_bit MIGRATING
>                                               irq_set_affinity (new)
>                                               return
>   irq_set_affinity (old)

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Stefano Stabellini]

On Fri, 31 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/03/17 00:47, Stefano Stabellini wrote:
> > On Fri, 3 Mar 2017, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 01/03/17 22:15, Stefano Stabellini wrote:
> > > > A potential race condition occurs when vgic_migrate_irq is called a
> > > > second time, while GIC_IRQ_GUEST_MIGRATING is already set. In that case,
> > > > vgic_migrate_irq takes a different vgic lock from gic_update_one_lr.
> > > 
> > > Hmmm, vgic_migrate_irq will bail out before accessing inflight list if
> > > GIC_IRQ_GUEST_MIGRATING is already set:
> > > 
> > > /* migrating already in progress, no need to do anything */
> > > if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status )
> > >   return;
> > > 
> > > And test_bit is atomic. So I don't understand what is the corruption
> > > problem
> > > you mention.
> > 
> > The scenario is a bit convoluted: GIC_IRQ_GUEST_MIGRATING is already set
> > and vgic_migrate_irq is called to move the irq again, even though the
> > first migration is not complete yet.
> 
> What you described is not a data corruption to me.

No, it is not, thanks to the previous two patches. The commit
description needs an update.


> The host IRQ will be routed
> to the wrong pCPU and then what? The IRQ will still trigger, ok on the wrong
> pCPU, it will be slower but we are capable to handle that.
> 
> The use case you describe would only happen if a guest is trying to change the
> routing multiple times while an interrupt is pending. So to be honest, a sane
> guest would not do that. But this would only affect stupid guest.
> 
> So I don't think this is worth to support considering how this patch will
> increase the code complexity in a component that is already a nightmare to
> handle.

I think we have to fix this because it is not predictable. Latency could
be much higher, depending on who wins the race. It also uses more Xen
resources -- the time that Xen spends to send and to handle SGIs could
be used for something  else. I think it is more important to be
predictable than correct. Especially given that a sane guest shouldn't
do this, I prefer to refuse a "nested" migration we cannot handle (even
though it is a mistake) than provide unreliable latency.

In other words, I think we need to fix this one way or another, but we
might be able to come up with a better fix than this.



> > This could happen:
> > 
> > 
> >       CPU 0                                    CPU 1
> >   gic_update_one_lr
> >   test_and_clear_bit MIGRATING
> >   read target (old)
> >                                             write target (new)
> >                                             vgic_migrate_irq
> >                                               test_bit MIGRATING
> >                                               irq_set_affinity (new)
> >                                               return
> >   irq_set_affinity (old)

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

Re: [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Julien Grall]

Hi Stefano,

On 31/03/17 21:24, Stefano Stabellini wrote:
> On Fri, 31 Mar 2017, Julien Grall wrote:
>> On 30/03/17 00:47, Stefano Stabellini wrote:
>>> On Fri, 3 Mar 2017, Julien Grall wrote:
>> What you described is not a data corruption to me.
>
> No, it is not, thanks to the previous two patches. The commit
> description needs an update.
>
>
>> The host IRQ will be routed
>> to the wrong pCPU and then what? The IRQ will still trigger, ok on the wrong
>> pCPU, it will be slower but we are capable to handle that.
>>
>> The use case you describe would only happen if a guest is trying to change the
>> routing multiple times while an interrupt is pending. So to be honest, a sane
>> guest would not do that. But this would only affect stupid guest.
>>
>> So I don't think this is worth to support considering how this patch will
>> increase the code complexity in a component that is already a nightmare to
>> handle.
>
> I think we have to fix this because it is not predictable. Latency could
> be much higher, depending on who wins the race. It also uses more Xen
> resources -- the time that Xen spends to send and to handle SGIs could
> be used for something  else.  I think it is more important to be
> predictable than correct. Especially given that a sane guest shouldn't
> do this, I prefer to refuse a "nested" migration we cannot handle (even
> though it is a mistake) than provide unreliable latency.

Good point. We already have a couple of place in the vGIC we don't 
handle and print a message instead (see ACTIVER, I*PENDR registers).

I would prefer to refuse "nested" migration and warn the guest. If 
someone complain, then we can think about it.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Stefano Stabellini]

On Mon, 3 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 31/03/17 21:24, Stefano Stabellini wrote:
> > On Fri, 31 Mar 2017, Julien Grall wrote:
> > > On 30/03/17 00:47, Stefano Stabellini wrote:
> > > > On Fri, 3 Mar 2017, Julien Grall wrote:
> > > What you described is not a data corruption to me.
> > 
> > No, it is not, thanks to the previous two patches. The commit
> > description needs an update.
> > 
> > 
> > > The host IRQ will be routed
> > > to the wrong pCPU and then what? The IRQ will still trigger, ok on the
> > > wrong
> > > pCPU, it will be slower but we are capable to handle that.
> > > 
> > > The use case you describe would only happen if a guest is trying to change
> > > the
> > > routing multiple times while an interrupt is pending. So to be honest, a
> > > sane
> > > guest would not do that. But this would only affect stupid guest.
> > > 
> > > So I don't think this is worth to support considering how this patch will
> > > increase the code complexity in a component that is already a nightmare to
> > > handle.
> > 
> > I think we have to fix this because it is not predictable. Latency could
> > be much higher, depending on who wins the race. It also uses more Xen
> > resources -- the time that Xen spends to send and to handle SGIs could
> > be used for something  else.  I think it is more important to be
> > predictable than correct. Especially given that a sane guest shouldn't
> > do this, I prefer to refuse a "nested" migration we cannot handle (even
> > though it is a mistake) than provide unreliable latency.
> 
> Good point. We already have a couple of place in the vGIC we don't handle and
> print a message instead (see ACTIVER, I*PENDR registers).
> 
> I would prefer to refuse "nested" migration and warn the guest. If someone
> complain, then we can think about it.

That's fine by me.

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

Re: [PATCH v5 1/3] arm: remove irq from inflight, then change physical affinity

[Julien Grall]

Hi Stefano,

On 01/03/2017 22:15, Stefano Stabellini wrote:
> This patch fixes a potential race that could happen when
> gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously.
>
> When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq has
> been removed from inflight before changing physical affinity, to avoid
> concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take a
> different vcpu lock.
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/arch/arm/gic.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 9522c6c..16bb150 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -503,6 +503,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>              gic_raise_guest_irq(v, irq, p->priority);
>          else {
>              list_del_init(&p->inflight);
> +            /* Remove from inflight, then change physical affinity. It
> +             * makes sure that when a new interrupt is received on the
> +             * next pcpu, inflight is already cleared. No concurrent
> +             * accesses to inflight. */

Coding style:

/*
  * ...
  */

> +            smp_mb();

Barriers are working in pair. So where is the associated barrier?

Also, I am still unsure why you use a dmb(ish) (implementation of 
smp_mb) and not dmb(sy).

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 1/3] arm: remove irq from inflight, then change physical affinity

[Julien Grall]

Hi,

On 01/03/17 23:24, Julien Grall wrote:
> On 01/03/2017 22:15, Stefano Stabellini wrote:
>> This patch fixes a potential race that could happen when
>> gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously.
>>
>> When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq has
>> been removed from inflight before changing physical affinity, to avoid
>> concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take a
>> different vcpu lock.
>>
>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>  xen/arch/arm/gic.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 9522c6c..16bb150 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -503,6 +503,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>              gic_raise_guest_irq(v, irq, p->priority);
>>          else {
>>              list_del_init(&p->inflight);
>> +            /* Remove from inflight, then change physical affinity. It
>> +             * makes sure that when a new interrupt is received on the
>> +             * next pcpu, inflight is already cleared. No concurrent
>> +             * accesses to inflight. */
>
> Coding style:
>
> /*
>  * ...
>  */
>
>> +            smp_mb();
>
> Barriers are working in pair. So where is the associated barrier?
>
> Also, I am still unsure why you use a dmb(ish) (implementation of
> smp_mb) and not dmb(sy).

I looked a bit more into this. Quoting a commit message in Linux (see 
8adbf57fc429 "irqchip: gic: use dmb ishst instead of dsb when raising a 
softirq"):

"When sending an SGI to another CPU, we require a barrier to ensure that 
any pending stores to normal memory are made visible to the recipient 
before the interrupt arrives.

Rather than use a vanilla dsb() (which will soon cause an assembly error 
on arm64) before the writel_relaxed, we can instead use dsb(ishst), 
since we just need to ensure that any pending normal writes are visible 
within the inner-shareable domain before we poke the GIC.

With this observation, we can then further weaken the barrier to a
dmb(ishst), since other CPUs in the inner-shareable domain must observe 
the write to the distributor before the SGI is generated."

So smp_mb() is fine here. You could even use smp_wmb() because you only 
care you write access.

Also, I think the barrier on the other side is not necessary. Because if 
you received an interrupt it means the processor as observed the change 
in the distributor. What do you think?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 1/3] arm: remove irq from inflight, then change physical affinity

[Stefano Stabellini]

On Fri, 3 Mar 2017, Julien Grall wrote:
> On 01/03/17 23:24, Julien Grall wrote:
> > On 01/03/2017 22:15, Stefano Stabellini wrote:
> > > This patch fixes a potential race that could happen when
> > > gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously.
> > > 
> > > When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq has
> > > been removed from inflight before changing physical affinity, to avoid
> > > concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take a
> > > different vcpu lock.
> > > 
> > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > > ---
> > >  xen/arch/arm/gic.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index 9522c6c..16bb150 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -503,6 +503,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> > >              gic_raise_guest_irq(v, irq, p->priority);
> > >          else {
> > >              list_del_init(&p->inflight);
> > > +            /* Remove from inflight, then change physical affinity. It
> > > +             * makes sure that when a new interrupt is received on the
> > > +             * next pcpu, inflight is already cleared. No concurrent
> > > +             * accesses to inflight. */
> > 
> > Coding style:
> > 
> > /*
> >  * ...
> >  */
> > 
> > > +            smp_mb();
> > 
> > Barriers are working in pair. So where is the associated barrier?
> > 
> > Also, I am still unsure why you use a dmb(ish) (implementation of
> > smp_mb) and not dmb(sy).
> 
> I looked a bit more into this. Quoting a commit message in Linux (see
> 8adbf57fc429 "irqchip: gic: use dmb ishst instead of dsb when raising a
> softirq"):
> 
> "When sending an SGI to another CPU, we require a barrier to ensure that any
> pending stores to normal memory are made visible to the recipient before the
> interrupt arrives.
> 
> Rather than use a vanilla dsb() (which will soon cause an assembly error on
> arm64) before the writel_relaxed, we can instead use dsb(ishst), since we just
> need to ensure that any pending normal writes are visible within the
> inner-shareable domain before we poke the GIC.
> 
> With this observation, we can then further weaken the barrier to a
> dmb(ishst), since other CPUs in the inner-shareable domain must observe the
> write to the distributor before the SGI is generated."
> 
> So smp_mb() is fine here. You could even use smp_wmb() because you only care
> you write access.
> 
> Also, I think the barrier on the other side is not necessary. Because if you
> received an interrupt it means the processor as observed the change in the
> distributor. What do you think?

I agree with you, I think you are right. My reasoning was the same as
yours.

I didn't use smp_wmb() because that's used between two writes, while
here, after the barrier we could have a read (in fact we have
test_and_clear_bit, but the following patches turn it into a test_bit,
which is a read).

So I think this patch should be OK as is.

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

Re: [PATCH v5 1/3] arm: remove irq from inflight, then change physical affinity

[Julien Grall]

Hi Stefano,

On 03/03/17 19:34, Stefano Stabellini wrote:
> On Fri, 3 Mar 2017, Julien Grall wrote:
>> On 01/03/17 23:24, Julien Grall wrote:
>>> On 01/03/2017 22:15, Stefano Stabellini wrote:
>>>> This patch fixes a potential race that could happen when
>>>> gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously.
>>>>
>>>> When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq has
>>>> been removed from inflight before changing physical affinity, to avoid
>>>> concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take a
>>>> different vcpu lock.
>>>>
>>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>>> ---
>>>>  xen/arch/arm/gic.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index 9522c6c..16bb150 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -503,6 +503,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>>>              gic_raise_guest_irq(v, irq, p->priority);
>>>>          else {
>>>>              list_del_init(&p->inflight);
>>>> +            /* Remove from inflight, then change physical affinity. It
>>>> +             * makes sure that when a new interrupt is received on the
>>>> +             * next pcpu, inflight is already cleared. No concurrent
>>>> +             * accesses to inflight. */
>>>
>>> Coding style:
>>>
>>> /*
>>>  * ...
>>>  */
>>>
>>>> +            smp_mb();
>>>
>>> Barriers are working in pair. So where is the associated barrier?
>>>
>>> Also, I am still unsure why you use a dmb(ish) (implementation of
>>> smp_mb) and not dmb(sy).
>>
>> I looked a bit more into this. Quoting a commit message in Linux (see
>> 8adbf57fc429 "irqchip: gic: use dmb ishst instead of dsb when raising a
>> softirq"):
>>
>> "When sending an SGI to another CPU, we require a barrier to ensure that any
>> pending stores to normal memory are made visible to the recipient before the
>> interrupt arrives.
>>
>> Rather than use a vanilla dsb() (which will soon cause an assembly error on
>> arm64) before the writel_relaxed, we can instead use dsb(ishst), since we just
>> need to ensure that any pending normal writes are visible within the
>> inner-shareable domain before we poke the GIC.
>>
>> With this observation, we can then further weaken the barrier to a
>> dmb(ishst), since other CPUs in the inner-shareable domain must observe the
>> write to the distributor before the SGI is generated."
>>
>> So smp_mb() is fine here. You could even use smp_wmb() because you only care
>> you write access.
>>
>> Also, I think the barrier on the other side is not necessary. Because if you
>> received an interrupt it means the processor as observed the change in the
>> distributor. What do you think?
>
> I agree with you, I think you are right. My reasoning was the same as
> yours.
>
> I didn't use smp_wmb() because that's used between two writes, while
> here, after the barrier we could have a read (in fact we have
> test_and_clear_bit, but the following patches turn it into a test_bit,
> which is a read).

Why do you care about the read? You only want to ensure the ordering 
between list_del and writing the new affinity into the GIC.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 1/3] arm: remove irq from inflight, then change physical affinity

[Stefano Stabellini]

On Fri, 3 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/03/17 19:34, Stefano Stabellini wrote:
> > On Fri, 3 Mar 2017, Julien Grall wrote:
> > > On 01/03/17 23:24, Julien Grall wrote:
> > > > On 01/03/2017 22:15, Stefano Stabellini wrote:
> > > > > This patch fixes a potential race that could happen when
> > > > > gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously.
> > > > > 
> > > > > When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq
> > > > > has
> > > > > been removed from inflight before changing physical affinity, to avoid
> > > > > concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take
> > > > > a
> > > > > different vcpu lock.
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > > > > ---
> > > > >  xen/arch/arm/gic.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > > > index 9522c6c..16bb150 100644
> > > > > --- a/xen/arch/arm/gic.c
> > > > > +++ b/xen/arch/arm/gic.c
> > > > > @@ -503,6 +503,11 @@ static void gic_update_one_lr(struct vcpu *v, int
> > > > > i)
> > > > >              gic_raise_guest_irq(v, irq, p->priority);
> > > > >          else {
> > > > >              list_del_init(&p->inflight);
> > > > > +            /* Remove from inflight, then change physical affinity.
> > > > > It
> > > > > +             * makes sure that when a new interrupt is received on
> > > > > the
> > > > > +             * next pcpu, inflight is already cleared. No concurrent
> > > > > +             * accesses to inflight. */
> > > > 
> > > > Coding style:
> > > > 
> > > > /*
> > > >  * ...
> > > >  */
> > > > 
> > > > > +            smp_mb();
> > > > 
> > > > Barriers are working in pair. So where is the associated barrier?
> > > > 
> > > > Also, I am still unsure why you use a dmb(ish) (implementation of
> > > > smp_mb) and not dmb(sy).
> > > 
> > > I looked a bit more into this. Quoting a commit message in Linux (see
> > > 8adbf57fc429 "irqchip: gic: use dmb ishst instead of dsb when raising a
> > > softirq"):
> > > 
> > > "When sending an SGI to another CPU, we require a barrier to ensure that
> > > any
> > > pending stores to normal memory are made visible to the recipient before
> > > the
> > > interrupt arrives.
> > > 
> > > Rather than use a vanilla dsb() (which will soon cause an assembly error
> > > on
> > > arm64) before the writel_relaxed, we can instead use dsb(ishst), since we
> > > just
> > > need to ensure that any pending normal writes are visible within the
> > > inner-shareable domain before we poke the GIC.
> > > 
> > > With this observation, we can then further weaken the barrier to a
> > > dmb(ishst), since other CPUs in the inner-shareable domain must observe
> > > the
> > > write to the distributor before the SGI is generated."
> > > 
> > > So smp_mb() is fine here. You could even use smp_wmb() because you only
> > > care
> > > you write access.
> > > 
> > > Also, I think the barrier on the other side is not necessary. Because if
> > > you
> > > received an interrupt it means the processor as observed the change in the
> > > distributor. What do you think?
> > 
> > I agree with you, I think you are right. My reasoning was the same as
> > yours.
> > 
> > I didn't use smp_wmb() because that's used between two writes, while
> > here, after the barrier we could have a read (in fact we have
> > test_and_clear_bit, but the following patches turn it into a test_bit,
> > which is a read).
> 
> Why do you care about the read? You only want to ensure the ordering between
> list_del and writing the new affinity into the GIC.

Yes, you are right. I'll change the smp_mb() into smp_wmb()

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