Re: [Xen-devel] [PATCH] xen: arm: correctly write release target in smp_spin_table_cpu_up

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >> Acked-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > 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.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel