| Message ID | 1389797901.3793.71.camel@kazak.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
On 01/15/2014 02:58 PM, Ian Campbell wrote: > On Tue, 2014-01-14 at 18:48 +0000, Julien Grall wrote: >> On 01/14/2014 04:55 PM, Ian Campbell wrote: >>> flush_xen_data_tlb_range_va() is clearly bogus since it flushes the tlb, not >>> the data cache. Perhaps what was meant was flush_xen_dcache(), but the address >>> was mapped with ioremap_nocache and hence isn't cached in the first place. >>> Accesses should be via writeq though, so do that. >>> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >> Acked-by: Julien Grall <julien.grall@linaro.org> > > Thanks. I think release wise this can wait for 4.5, the flush is > unnecessary but not harmful. > > While the use of writeq is needed for the additional barriers which it > adds it's not strictly needed here because there is no prior write to > sequence against (and ioremap_nocache has a barrier in it). > > Unless removing this pointless flush makes some analysis of the use of > the functions less confusing perhaps? For code comprehension, it's better. I think this patch is like "arm: flush TLB on all CPUs when setting and ...". It's not harmful when Xen is used but it helps readability. > However thinking about the writeq barriers to lead me to notice the lack > of a recommended dsb() before the subsequent use of sev(), which is > needed to ensure that the write has occurred before we wake the other > processor. We get away with this because the iounmap in the middle does > a write_pte which has a dsb in it. Phew! Still. for 4.5: > > 8<----- > > From aab391b98760cc8417e06512848cf83dd5d71b5c Mon Sep 17 00:00:00 2001 > From: Ian Campbell <ian.campbell@citrix.com> > Date: Wed, 15 Jan 2014 14:49:04 +0000 > Subject: [PATCH] xen: arm: add barrier before sev in smp_spin_table_cpu_up > > This ensures that the writeq to the release address has occurred. > > In reality there is a dsb() in the iounmap() (in the e entual write_pte()) but e entual? Did you function write_pte()? > make it explicit. > > The ARMv8 ARM recommends that sev() is usually accompanied by a dsb(), the > only other uses are in the v7 spinlock code which includes a dsb() already. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > xen/arch/arm/arm64/smpboot.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c > index 9146476..9c7bf29 100644 > --- a/xen/arch/arm/arm64/smpboot.c > +++ b/xen/arch/arm/arm64/smpboot.c > @@ -36,6 +36,7 @@ static int __init smp_spin_table_cpu_up(int cpu) > > iounmap(release); > > + dsb(); > sev(); > > return cpu_up_send_sgi(cpu); >
On Wed, 2014-01-15 at 16:36 +0000, Julien Grall wrote: > On 01/15/2014 02:58 PM, Ian Campbell wrote: > > On Tue, 2014-01-14 at 18:48 +0000, Julien Grall wrote: > >> On 01/14/2014 04:55 PM, Ian Campbell wrote: > >>> flush_xen_data_tlb_range_va() is clearly bogus since it flushes the tlb, not > >>> the data cache. Perhaps what was meant was flush_xen_dcache(), but the address > >>> was mapped with ioremap_nocache and hence isn't cached in the first place. > >>> Accesses should be via writeq though, so do that. > >>> > >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > >> Acked-by: Julien Grall <julien.grall@linaro.org> > > > > Thanks. I think release wise this can wait for 4.5, the flush is > > unnecessary but not harmful. > > > > While the use of writeq is needed for the additional barriers which it > > adds it's not strictly needed here because there is no prior write to > > sequence against (and ioremap_nocache has a barrier in it). > > > > Unless removing this pointless flush makes some analysis of the use of > > the functions less confusing perhaps? > > For code comprehension, it's better. I think this patch is like "arm: > flush TLB on all CPUs when setting and ...". It's not harmful when Xen > is used but it helps readability. True, I'm not sure I'm comfortable hanging a freeze exception on that though. > > > However thinking about the writeq barriers to lead me to notice the lack > > of a recommended dsb() before the subsequent use of sev(), which is > > needed to ensure that the write has occurred before we wake the other > > processor. We get away with this because the iounmap in the middle does > > a write_pte which has a dsb in it. Phew! Still. for 4.5: > > > > 8<----- > > > > From aab391b98760cc8417e06512848cf83dd5d71b5c Mon Sep 17 00:00:00 2001 > > From: Ian Campbell <ian.campbell@citrix.com> > > Date: Wed, 15 Jan 2014 14:49:04 +0000 > > Subject: [PATCH] xen: arm: add barrier before sev in smp_spin_table_cpu_up > > > > This ensures that the writeq to the release address has occurred. > > > > In reality there is a dsb() in the iounmap() (in the e entual write_pte()) but > > e entual? Did you function write_pte()? s/ /v/ => eventual. Oops.
diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c index 9146476..9c7bf29 100644 --- a/xen/arch/arm/arm64/smpboot.c +++ b/xen/arch/arm/arm64/smpboot.c @@ -36,6 +36,7 @@ static int __init smp_spin_table_cpu_up(int cpu) iounmap(release); + dsb(); sev(); return cpu_up_send_sgi(cpu);