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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@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?

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