From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen: arm: correctly write release target in smp_spin_table_cpu_up Date: Wed, 15 Jan 2014 16:36:30 +0000 Message-ID: <52D6B90E.8050908@linaro.org> References: <1389718504-1514-1-git-send-email-ian.campbell@citrix.com> <52D58692.8000305@linaro.org> <1389797901.3793.71.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1389797901.3793.71.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >> Acked-by: Julien Grall > > 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 > 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 > --- > 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); > -- Julien Grall