Re: [Xen-devel] [PATCH v4 2/2] arm: proper ordering for correct execution of gic_update_one_lr and vgic_store_itargetsr

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 10 Feb 2017, Stefano Stabellini wrote:
> Concurrent execution of gic_update_one_lr and vgic_store_itargetsr can
> result in the wrong pcpu being set as irq target, see
> http://marc.info/?l=xen-devel&m=148218667104072.
> 
> To solve the issue, add barriers, remove an irq from the inflight
> queue, only after the affinity has been set. On the other end, write the
> new vcpu target, before checking GIC_IRQ_GUEST_MIGRATING and inflight.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

For reference, with this patch, gic_update_one_lr and vgic_store_itargetsr
can still run simultaneously on two cpus without any lock protections. This
scenario can happen:

  <GIC_IRQ_GUEST_MIGRATING is set>      <GIC_IRQ_GUEST_MIGRATING is set>
  CPU0: gic_update_one_lr               CPU1: vgic_store_itargetsr
  ----------------------------------------------------------------------
  clear GIC_IRQ_GUEST_MIGRATING
                                        set rank->vcpu (final)
                                        vgic_migrate_irq
                                          if (inflight) set 
GIC_IRQ_GUEST_MIGRATING
  read rank->vcpu (final)
  irq_set_affinity (final)
  remove from inflight

At this point, the affinity is correctly set to the right pcpu and
everything else is OK too, except that GIC_IRQ_GUEST_MIGRATING is still
set, while inflight has been cleared.

What does that mean? Next time gic_update_one_lr is called again
cleaning up this interrupt, the GIC_IRQ_GUEST_MIGRATING path is taken
again, even though it's not necessary. irq_set_affinity is called one
more time, but should be harmless. However, GIC_IRQ_GUEST_QUEUED is
ignored, but I think that's acceptable: it is only set at this stage if
a second interrupt came through while the first one was active (not
possible for level interrupts) and the vcpu wasn't current. In that
case, the second interrupt will be lost, but next time we get an
interrupt it will be injected as usual.

If that's not acceptable, we can avoid this scenario by using this patch

http://marc.info/?l=xen-devel&m=148237295920492


> ---
>  xen/arch/arm/gic.c     | 3 ++-
>  xen/arch/arm/vgic-v2.c | 4 ++--
>  xen/arch/arm/vgic-v3.c | 4 +++-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index a5348f2..bb52959 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -503,12 +503,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>               !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>              gic_raise_guest_irq(v, irq, p->priority);
>          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));
>              }
> +            smp_mb();
> +            list_del_init(&p->inflight);
>          }
>      }
>  }
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b30379e..f47286e 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -153,6 +153,8 @@ static void vgic_store_itargetsr(struct domain *d, struct 
> vgic_irq_rank *rank,
>          new_target--;
>  
>          old_target = read_atomic(&rank->vcpu[offset]);
> +        write_atomic(&rank->vcpu[offset], new_target);
> +        smp_mb();
>  
>          /* Only migrate the vIRQ if the target vCPU has changed */
>          if ( new_target != old_target )
> @@ -161,8 +163,6 @@ static void vgic_store_itargetsr(struct domain *d, struct 
> vgic_irq_rank *rank,
>                               d->vcpu[new_target],
>                               virq);
>          }
> -
> -        write_atomic(&rank->vcpu[offset], new_target);
>      }
>  }
>  
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 7dc9b6f..e826666 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -150,11 +150,13 @@ static void vgic_store_irouter(struct domain *d, struct 
> vgic_irq_rank *rank,
>      if ( !new_vcpu )
>          return;
>  
> +    write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id);
> +    smp_mb();
> +
>      /* 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);
>  }
>  
>  static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
> -- 
> 1.9.1
> 

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