| Message ID | 20181023181709.11883-5-julien.grall@arm.com |
|---|---|
| State | Accepted |
| Commit | 3b439f636ee9a9588203cf0aa0edfa18ccdc60b9 |
| Headers | show |
| Series | xen/arm: GIC fixes and improvement | expand |
Hello Julien, On 23.10.18 21:17, Julien Grall wrote: > 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. > > For GICv2, rather than using dsb(sy) before writel_gicd, 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. > > A DMB instruction can be used to ensure the relative order of only > memory accesses before and after the barrier. Since writes to system > registers are not memory operations, barrier DMB is not sufficient for > observalibility of memory accesses that occur before ICC_SGI1R_EL1 > (GICv3). > > For GICv3, a DSB instruction ensures that no instructions that appear in > program order after the DSB instruction, can execute until the DSB > instruction has completed. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/gic-v2.c | 6 ++++++ > xen/arch/arm/gic-v3.c | 6 ++++++ > xen/arch/arm/gic.c | 18 ------------------ > 3 files changed, 12 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index e7eb01f30a..1a744c576f 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -455,6 +455,12 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode, > unsigned int mask = 0; > cpumask_t online_mask; > > + /* > + * Ensure that stores to Normal memory are visible to the other CPUs > + * before they observe us issuing the IPI. > + */ > + dmb(ishst); > + > switch ( irqmode ) > { > case SGI_TARGET_OTHERS: > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 2952335d05..a0a1a45ba7 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -986,6 +986,12 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const cpumask_t *cpumask) > static void gicv3_send_sgi(enum gic_sgi sgi, enum gic_sgi_mode mode, > const cpumask_t *cpumask) > { > + /* > + * Ensure that stores to Normal memory are visible to the other CPUs > + * before issuing the IPI. > + */ > + wmb(); > + > switch ( mode ) > { > case SGI_TARGET_OTHERS: > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 0108e9603c..077b941b79 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -300,12 +300,6 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > { > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - /* > - * Ensure that stores to Normal memory are visible to the other CPUs > - * before issuing the IPI. > - * Matches the read barrier in do_sgi. > - */ > - dsb(sy); Here you remove the comment you've just added within this patch series, and the same below. Wouldn't it better to reorder patches, first relax the barrier, then ensure ordering between read of INTACK and shared data? > gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask); > } > > @@ -318,12 +312,6 @@ void send_SGI_self(enum gic_sgi sgi) > { > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - /* > - * Ensure that stores to Normal memory are visible to the other CPUs > - * before issuing the IPI. > - * Matches the read barrier in do_sgi. > - */ > - dsb(sy); > gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL); > } > > @@ -331,12 +319,6 @@ void send_SGI_allbutself(enum gic_sgi sgi) > { > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - /* > - * Ensure that stores to Normal memory are visible to the other CPUs > - * before issuing the IPI. > - * Matches the read barrier in do_sgi. > - */ > - dsb(sy); > gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL); > } > -- *Andrii Anisov*
On 10/24/18 2:32 PM, Andrii Anisov wrote: > Hello Julien, Hi Andrii, > On 23.10.18 21:17, Julien Grall wrote: >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 0108e9603c..077b941b79 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -300,12 +300,6 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) >> { >> ASSERT(sgi < 16); /* There are only 16 SGIs */ >> >> - /* >> - * Ensure that stores to Normal memory are visible to the other CPUs >> - * before issuing the IPI. >> - * Matches the read barrier in do_sgi. >> - */ >> - dsb(sy); > Here you remove the comment you've just added within this patch series, > and the same below. > Wouldn't it better to reorder patches, first relax the barrier, then > ensure ordering between read of INTACK and shared data? patch #1 and #2 should be backported. This patch is technically not a bug fix but a relaxation. So I am not comfortable to suggest backporting it up to Xen 4.9. Hence the ordering of the series. Cheers,
On 24.10.18 17:46, Julien Grall wrote: > patch #1 and #2 should be backported. This patch is technically not a > bug fix but a relaxation. So I am not comfortable to suggest > backporting it up to Xen 4.9. Hence the ordering of the series. OK, I understand the motivation, but still feel some oddness in that. Anyway, Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
On 10/24/18 4:40 PM, Andrii Anisov wrote: > > On 24.10.18 17:46, Julien Grall wrote: >> patch #1 and #2 should be backported. This patch is technically not a >> bug fix but a relaxation. So I am not comfortable to suggest >> backporting it up to Xen 4.9. Hence the ordering of the series. > OK, I understand the motivation, but still feel some oddness in that. We don't usually backport patch that are not bug fix. So people can upgrade to point release without been worrying of behavior change. Yes it is odd to write comments and then remove them in a old patch. But I felt comments were useful for backporting. > > Anyway, > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> Thank you! Cheers,
On Tue, 23 Oct 2018, Julien Grall wrote: > 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. > > For GICv2, rather than using dsb(sy) before writel_gicd, 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. > > A DMB instruction can be used to ensure the relative order of only > memory accesses before and after the barrier. Since writes to system > registers are not memory operations, barrier DMB is not sufficient for > observalibility of memory accesses that occur before ICC_SGI1R_EL1 > (GICv3). > > For GICv3, a DSB instruction ensures that no instructions that appear in > program order after the DSB instruction, can execute until the DSB > instruction has completed. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/gic-v2.c | 6 ++++++ > xen/arch/arm/gic-v3.c | 6 ++++++ > xen/arch/arm/gic.c | 18 ------------------ > 3 files changed, 12 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index e7eb01f30a..1a744c576f 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -455,6 +455,12 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode, > unsigned int mask = 0; > cpumask_t online_mask; > > + /* > + * Ensure that stores to Normal memory are visible to the other CPUs > + * before they observe us issuing the IPI. > + */ > + dmb(ishst); > + > switch ( irqmode ) > { > case SGI_TARGET_OTHERS: > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 2952335d05..a0a1a45ba7 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -986,6 +986,12 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const cpumask_t *cpumask) > static void gicv3_send_sgi(enum gic_sgi sgi, enum gic_sgi_mode mode, > const cpumask_t *cpumask) > { > + /* > + * Ensure that stores to Normal memory are visible to the other CPUs > + * before issuing the IPI. > + */ > + wmb(); NIT: do we want to use dsb(st) instead of wmb() for consistency with gic-v2.c and to make the difference more obvious? In any case Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> FYI I committed the first three patches. > switch ( mode ) > { > case SGI_TARGET_OTHERS: > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 0108e9603c..077b941b79 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -300,12 +300,6 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > { > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - /* > - * Ensure that stores to Normal memory are visible to the other CPUs > - * before issuing the IPI. > - * Matches the read barrier in do_sgi. > - */ > - dsb(sy); > gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask); > } > > @@ -318,12 +312,6 @@ void send_SGI_self(enum gic_sgi sgi) > { > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - /* > - * Ensure that stores to Normal memory are visible to the other CPUs > - * before issuing the IPI. > - * Matches the read barrier in do_sgi. > - */ > - dsb(sy); > gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL); > } > > @@ -331,12 +319,6 @@ void send_SGI_allbutself(enum gic_sgi sgi) > { > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - /* > - * Ensure that stores to Normal memory are visible to the other CPUs > - * before issuing the IPI. > - * Matches the read barrier in do_sgi. > - */ > - dsb(sy); > gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL); > } > > -- > 2.11.0 >
Hi Stefano, On 11/9/18 11:14 PM, Stefano Stabellini wrote: > On Tue, 23 Oct 2018, Julien Grall wrote: > >> 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. >> >> For GICv2, rather than using dsb(sy) before writel_gicd, 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. >> >> A DMB instruction can be used to ensure the relative order of only >> memory accesses before and after the barrier. Since writes to system >> registers are not memory operations, barrier DMB is not sufficient for >> observalibility of memory accesses that occur before ICC_SGI1R_EL1 >> (GICv3). >> >> For GICv3, a DSB instruction ensures that no instructions that appear in >> program order after the DSB instruction, can execute until the DSB >> instruction has completed. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/gic-v2.c | 6 ++++++ >> xen/arch/arm/gic-v3.c | 6 ++++++ >> xen/arch/arm/gic.c | 18 ------------------ >> 3 files changed, 12 insertions(+), 18 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index e7eb01f30a..1a744c576f 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -455,6 +455,12 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode, >> unsigned int mask = 0; >> cpumask_t online_mask; >> >> + /* >> + * Ensure that stores to Normal memory are visible to the other CPUs >> + * before they observe us issuing the IPI. >> + */ >> + dmb(ishst); >> + >> switch ( irqmode ) >> { >> case SGI_TARGET_OTHERS: >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >> index 2952335d05..a0a1a45ba7 100644 >> --- a/xen/arch/arm/gic-v3.c >> +++ b/xen/arch/arm/gic-v3.c >> @@ -986,6 +986,12 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const cpumask_t *cpumask) >> static void gicv3_send_sgi(enum gic_sgi sgi, enum gic_sgi_mode mode, >> const cpumask_t *cpumask) >> { >> + /* >> + * Ensure that stores to Normal memory are visible to the other CPUs >> + * before issuing the IPI. >> + */ >> + wmb(); > > NIT: do we want to use dsb(st) instead of wmb() for consistency with > gic-v2.c and to make the difference more obvious? Good point, I will use dsb(st). > > In any case > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > FYI I committed the first three patches. Thank you! I committed this patch with the change suggested above as your gave your reviewed-by. Cheers,
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index e7eb01f30a..1a744c576f 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -455,6 +455,12 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode, unsigned int mask = 0; cpumask_t online_mask; + /* + * Ensure that stores to Normal memory are visible to the other CPUs + * before they observe us issuing the IPI. + */ + dmb(ishst); + switch ( irqmode ) { case SGI_TARGET_OTHERS: diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 2952335d05..a0a1a45ba7 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -986,6 +986,12 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const cpumask_t *cpumask) static void gicv3_send_sgi(enum gic_sgi sgi, enum gic_sgi_mode mode, const cpumask_t *cpumask) { + /* + * Ensure that stores to Normal memory are visible to the other CPUs + * before issuing the IPI. + */ + wmb(); + switch ( mode ) { case SGI_TARGET_OTHERS: diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 0108e9603c..077b941b79 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -300,12 +300,6 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) { ASSERT(sgi < 16); /* There are only 16 SGIs */ - /* - * Ensure that stores to Normal memory are visible to the other CPUs - * before issuing the IPI. - * Matches the read barrier in do_sgi. - */ - dsb(sy); gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask); } @@ -318,12 +312,6 @@ void send_SGI_self(enum gic_sgi sgi) { ASSERT(sgi < 16); /* There are only 16 SGIs */ - /* - * Ensure that stores to Normal memory are visible to the other CPUs - * before issuing the IPI. - * Matches the read barrier in do_sgi. - */ - dsb(sy); gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL); } @@ -331,12 +319,6 @@ void send_SGI_allbutself(enum gic_sgi sgi) { ASSERT(sgi < 16); /* There are only 16 SGIs */ - /* - * Ensure that stores to Normal memory are visible to the other CPUs - * before issuing the IPI. - * Matches the read barrier in do_sgi. - */ - dsb(sy); gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL); }
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. For GICv2, rather than using dsb(sy) before writel_gicd, 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. A DMB instruction can be used to ensure the relative order of only memory accesses before and after the barrier. Since writes to system registers are not memory operations, barrier DMB is not sufficient for observalibility of memory accesses that occur before ICC_SGI1R_EL1 (GICv3). For GICv3, a DSB instruction ensures that no instructions that appear in program order after the DSB instruction, can execute until the DSB instruction has completed. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/gic-v2.c | 6 ++++++ xen/arch/arm/gic-v3.c | 6 ++++++ xen/arch/arm/gic.c | 18 ------------------ 3 files changed, 12 insertions(+), 18 deletions(-)