[PATCH] arm64: fix invalidation of wrong __early_cpu_boot_status cacheline

[PATCH] arm64: fix invalidation of wrong __early_cpu_boot_status cacheline

[Ard Biesheuvel]

In head.S, the str_l macro, which takes a source register, a symbol name
and a temp register, is used to store a status value to the variable
__early_cpu_boot_status. Subsequently, the value of the temp register is
reused to invalidate any cachelines covering this variable.

However, since str_l resolves to

      adrp    \tmp, \sym
      str     \src, [\tmp, :lo12:\sym]

the temp register never actually holds the address of the variable but
only of the 4 KB window that covers it, and reusing it leads to the
wrong cacheline being invalidated. So instead, take the address
explicitly before doing the store, and reuse that value to perform
the cache invalidation.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

This deserves a cc stable, since the macro is always invoked for each CPU
at boot.

Alternatively, we could add a writeback to the str_l macro, but I think
in general, making any assumptions about the value of a temp register is
dodgy, so I prefer this approach instead.

 arch/arm64/kernel/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 8b8a3d1e23fb..fafe21e49aab 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -693,7 +693,8 @@ ENDPROC(__secondary_switched)
 
 	.macro	update_early_cpu_boot_status status, tmp1, tmp2
 	mov	\tmp2, #\status
-	str_l	\tmp2, __early_cpu_boot_status, \tmp1
+	adr_l	\tmp1, __early_cpu_boot_status
+	str	\tmp2, [\tmp1]
 	dmb	sy
 	dc	ivac, \tmp1			// Invalidate potentially stale cache line
 	.endm
-- 
2.5.0

[PATCH] arm64: fix invalidation of wrong __early_cpu_boot_status cacheline

[Mark Rutland]

On Fri, Apr 15, 2016 at 12:11:21PM +0200, Ard Biesheuvel wrote:
> In head.S, the str_l macro, which takes a source register, a symbol name
> and a temp register, is used to store a status value to the variable
> __early_cpu_boot_status. Subsequently, the value of the temp register is
> reused to invalidate any cachelines covering this variable.
> 
> However, since str_l resolves to
> 
>       adrp    \tmp, \sym
>       str     \src, [\tmp, :lo12:\sym]
> 
> the temp register never actually holds the address of the variable but
> only of the 4 KB window that covers it, and reusing it leads to the
> wrong cacheline being invalidated. So instead, take the address
> explicitly before doing the store, and reuse that value to perform
> the cache invalidation.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> This deserves a cc stable, since the macro is always invoked for each CPU
> at boot.

Sounds good, also:

Fixes: bb9052744f4b7ae1 ("arm64: Handle early CPU boot failures")

> Alternatively, we could add a writeback to the str_l macro, but I think
> in general, making any assumptions about the value of a temp register is
> dodgy, so I prefer this approach instead.

Likewise, I prefer this.

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

>  arch/arm64/kernel/head.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 8b8a3d1e23fb..fafe21e49aab 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -693,7 +693,8 @@ ENDPROC(__secondary_switched)
>  
>  	.macro	update_early_cpu_boot_status status, tmp1, tmp2
>  	mov	\tmp2, #\status
> -	str_l	\tmp2, __early_cpu_boot_status, \tmp1
> +	adr_l	\tmp1, __early_cpu_boot_status
> +	str	\tmp2, [\tmp1]
>  	dmb	sy
>  	dc	ivac, \tmp1			// Invalidate potentially stale cache line
>  	.endm
> -- 
> 2.5.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH] arm64: fix invalidation of wrong __early_cpu_boot_status cacheline

[Suzuki K Poulose]

On 15/04/16 11:20, Mark Rutland wrote:
> On Fri, Apr 15, 2016 at 12:11:21PM +0200, Ard Biesheuvel wrote:
>> In head.S, the str_l macro, which takes a source register, a symbol name
>> and a temp register, is used to store a status value to the variable
>> __early_cpu_boot_status. Subsequently, the value of the temp register is
>> reused to invalidate any cachelines covering this variable.
>>
>> However, since str_l resolves to
>>
>>        adrp    \tmp, \sym
>>        str     \src, [\tmp, :lo12:\sym]
>>
>> the temp register never actually holds the address of the variable but
>> only of the 4 KB window that covers it, and reusing it leads to the
>> wrong cacheline being invalidated. So instead, take the address
>> explicitly before doing the store, and reuse that value to perform
>> the cache invalidation.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> This deserves a cc stable, since the macro is always invoked for each CPU
>> at boot.
>
> Sounds good, also:
>
> Fixes: bb9052744f4b7ae1 ("arm64: Handle early CPU boot failures")

This first appeared in 4.6-rc1, so I don't think we need to Cc stable.

The initial patch was using adr_l as in this fix.

Catalin, was there a specific reason for that change ? I remember you mentioning
something about it.

Nevertheless,

Acked-by: Suzuki K Poulose <suzuki.poulose@arm.com>

[PATCH] arm64: fix invalidation of wrong __early_cpu_boot_status cacheline

[Catalin Marinas]

On Fri, Apr 15, 2016 at 11:36:57AM +0100, Suzuki K. Poulose wrote:
> On 15/04/16 11:20, Mark Rutland wrote:
> >On Fri, Apr 15, 2016 at 12:11:21PM +0200, Ard Biesheuvel wrote:
> >>In head.S, the str_l macro, which takes a source register, a symbol name
> >>and a temp register, is used to store a status value to the variable
> >>__early_cpu_boot_status. Subsequently, the value of the temp register is
> >>reused to invalidate any cachelines covering this variable.
> >>
> >>However, since str_l resolves to
> >>
> >>       adrp    \tmp, \sym
> >>       str     \src, [\tmp, :lo12:\sym]
> >>
> >>the temp register never actually holds the address of the variable but
> >>only of the 4 KB window that covers it, and reusing it leads to the
> >>wrong cacheline being invalidated. So instead, take the address
> >>explicitly before doing the store, and reuse that value to perform
> >>the cache invalidation.
> >>
> >>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>---
> >>
> >>This deserves a cc stable, since the macro is always invoked for each CPU
> >>at boot.
> >
> >Sounds good, also:
> >
> >Fixes: bb9052744f4b7ae1 ("arm64: Handle early CPU boot failures")
> 
> This first appeared in 4.6-rc1, so I don't think we need to Cc stable.

Indeed. I'll add the Fixes line though, for reference.

> The initial patch was using adr_l as in this fix.
> 
> Catalin, was there a specific reason for that change ? I remember you mentioning
> something about it.

I think it was just code reduction.

I'll queue the fix for 4.6-rc.

Thanks.

-- 
Catalin