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

[Julien Grall <julien.grall@linaro.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 <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);
> 


-- 
Julien Grall