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

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

[Ian Campbell]

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>
---
 xen/arch/arm/arm64/smpboot.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 1287c72..9146476 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -32,8 +32,7 @@ static int __init smp_spin_table_cpu_up(int cpu)
         return -EFAULT;
     }
 
-    release[0] = __pa(init_secondary);
-    flush_xen_data_tlb_range_va((vaddr_t)release, sizeof(*release));
+    writeq(__pa(init_secondary), release);
 
     iounmap(release);
 
-- 
1.7.10.4

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

[Julien Grall]

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>
> ---
>  xen/arch/arm/arm64/smpboot.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 1287c72..9146476 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -32,8 +32,7 @@ static int __init smp_spin_table_cpu_up(int cpu)
>          return -EFAULT;
>      }
>  
> -    release[0] = __pa(init_secondary);
> -    flush_xen_data_tlb_range_va((vaddr_t)release, sizeof(*release));
> +    writeq(__pa(init_secondary), release);
>  
>      iounmap(release);
>  
> 


-- 
Julien Grall

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

[Ian Campbell]

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?

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

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

[Julien Grall]

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

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

[Ian Campbell]

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.

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

[Ian Campbell]

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>

applied, thanks.