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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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
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@xxxxxxxxxx>
---
 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);
-- 
1.7.10.4




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