| Message ID | 1488406545-26164-1-git-send-email-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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,
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.
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,
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()
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);
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(+)