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

[Julien Grall <julien.grall@xxxxxxx>]

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