[Xen-devel,3/4] xen: arm: add barrier before sev in smp_spin_table_cpu_up

Message ID 1395064352-2128-3-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell March 17, 2014, 1:52 p.m.
This ensures that the writeq to the release address has occurred.

In reality there is a dsb() in the iounmap() (in the eventual 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>
---
v2: typos in the commit message
---
 xen/arch/arm/arm64/smpboot.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Julien Grall March 17, 2014, 2:18 p.m. | #1
Hi Ian,

On 03/17/2014 01:52 PM, Ian Campbell wrote:
> This ensures that the writeq to the release address has occurred.
> 
> In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but
> make it explicit.

There other place (see xen/arch/arm/platforms/exynos5.c) where we rely
on the dsb in write_pte. I think we should move the dsb in iounmap().

> 
> 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.

Would it be better to add dsb in the sev() macro?

Regards,
Ian Campbell March 17, 2014, 2:39 p.m. | #2
On Mon, 2014-03-17 at 14:18 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/17/2014 01:52 PM, Ian Campbell wrote:
> > This ensures that the writeq to the release address has occurred.
> > 
> > In reality there is a dsb() in the iounmap() (in the eventual write_pte()) but
> > make it explicit.
> 
> There other place (see xen/arch/arm/platforms/exynos5.c) where we rely
> on the dsb in write_pte. I think we should move the dsb in iounmap().

Now you put it like that it does seem logical that any writes would be
issued prior to/as part of the iounmap.

In that context I think it would in fact be fine for iounmap to rely on
the dsb in write_pte since both a "change the mappings" functions. IOW
maybe we don't need to do anything after all.

> > 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.
> 
> Would it be better to add dsb in the sev() macro?

ARM32 has a dsb_sev macro, maybe we should do the same for ARM64.

Ian
Julien Grall March 17, 2014, 2:43 p.m. | #3
On 03/17/2014 02:39 PM, Ian Campbell wrote:
>>> 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.
>>
>> Would it be better to add dsb in the sev() macro?
> 
> ARM32 has a dsb_sev macro, maybe we should do the same for ARM64.

Do we really need to rename the macro? From what I understand, most of
the time sev => dsb before. I think we can hide it in sev() macro.

Regards,
Ian Campbell March 17, 2014, 2:50 p.m. | #4
On Mon, 2014-03-17 at 14:43 +0000, Julien Grall wrote:
> On 03/17/2014 02:39 PM, Ian Campbell wrote:
> >>> 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.
> >>
> >> Would it be better to add dsb in the sev() macro?
> > 
> > ARM32 has a dsb_sev macro, maybe we should do the same for ARM64.
> 
> Do we really need to rename the macro?

I think so.

> From what I understand, most of
> the time sev => dsb before. I think we can hide it in sev() macro.

These macros are thin shims around the underlying instructions, having
them do other stuff as well would be potentially misleading.

The current macro has one callsight, I think we can live with the
churn...

Ian.
Ian Campbell April 2, 2014, 2:03 p.m. | #5
On Mon, 2014-03-17 at 13:52 +0000, Ian Campbell wrote:
> This ensures that the writeq to the release address has occurred.
> 
> In reality there is a dsb() in the iounmap() (in the eventual 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.

Thinking about this some more the dsb() in the ioumap is logical and it
is reasonable to rely on it. This also satisfies the need for a dsb
before the sev.

So I intend to just drop this patch for the next iteration.

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: typos in the commit message
> ---
>  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..8e2ecf2 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(); /* Ensure that the write to cpu_release_addr has occurred. */
>      sev();
>  
>      return cpu_up_send_sgi(cpu);

Patch

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 9146476..8e2ecf2 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(); /* Ensure that the write to cpu_release_addr has occurred. */
     sev();
 
     return cpu_up_send_sgi(cpu);