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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 11/02/17 02:05, 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>
> ---
>  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);
I don't understand why you remove from the inflight list afterwards. If 
you do that you introduce that same problem as discussed in
<7a78c859-fa6f-ba10-b574-d8edd46ea934@xxxxxxx>

As long as the interrupt is routed to the pCPU running 
gic_update_one_lr, the interrupt cannot fired because the interrupts are 
masked. However, as soon as irq_set_affinity is called the interrupt may 
fire on the other pCPU.
However, list_del_init is not atomic and not protected by any lock. So 
vgic_vcpu_inject_irq may see a corrupted version of {p,n}->inflight.
Did I miss anything?

Cheers,

--
Julien Grall

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