Re: [PATCH] ARM: vfp: use asm volatile for FP control register accesses

From: Nathan Chancellor <nathan@kernel.org>
To: Ard Biesheuvel <ardb+git@google.com>
Cc: linux-arm-kernel@lists.infradead.org, linux@armlinux.org.uk,
	arnd@arndb.de, linus.walleij@linaro.org,
	Ard Biesheuvel <ardb@kernel.org>,
	stable@kernel.org
Subject: Re: [PATCH] ARM: vfp: use asm volatile for FP control register accesses
Date: Tue, 26 Mar 2024 16:55:29 -0700	[thread overview]
Message-ID: <20240326235529.GA2025585@dev-arch.thelio-3990X> (raw)
In-Reply-To: <20240318093004.117153-2-ardb+git@google.com>

On Mon, Mar 18, 2024 at 10:30:05AM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Clang may reorder FP control register reads and writes, due to the fact
> that the inline asm() blocks in the read/write wrappers are not volatile
> qualified, and the compiler has no idea that these reads and writes may
> have side effects.
> 
> In particular, reads of FPSCR may generate an UNDEF exception if a
> floating point exception is pending, and the FP emulation code in
> VFP_bounce() explicitly clears FP exceptions temporarily in order to be
> able to perform the emulation on behalf of user space. This requires
> that the writes to FPEXC are never reordered with respect to accesses to
> other FP control registers, such as FPSCR.
> 
> So use asm volatile for both the read and the write helpers.
> 
> Cc: <stable@kernel.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

This seems reasonable to me based on my understanding of GCC's
documentation. However, their documentation states "the compiler can
move even volatile asm instructions relative to other code, including
across jump instructions" and I feel like there was some discussion
around this sentence in the past but I can't remember what the
conclusion was, although I want to say Clang did not have the same
behavior. Regardless:

Acked-by: Nathan Chancellor <nathan@kernel.org>

I am just curious, how was this discovered or noticed? Was there a
report I missed?

> ---
>  arch/arm/vfp/vfpinstr.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/vfp/vfpinstr.h b/arch/arm/vfp/vfpinstr.h
> index 3c7938fd40aa..c4ac778e6fc9 100644
> --- a/arch/arm/vfp/vfpinstr.h
> +++ b/arch/arm/vfp/vfpinstr.h
> @@ -66,14 +66,14 @@
>  
>  #define fmrx(_vfp_) ({			\
>  	u32 __v;			\
> -	asm(".fpu	vfpv2\n"	\
> +	asm volatile(".fpu vfpv2\n"	\
>  	    "vmrs	%0, " #_vfp_	\
>  	    : "=r" (__v) : : "cc");	\
>  	__v;				\
>   })
>  
>  #define fmxr(_vfp_,_var_)		\
> -	asm(".fpu	vfpv2\n"	\
> +	asm volatile(".fpu vfpv2\n"	\
>  	    "vmsr	" #_vfp_ ", %0"	\
>  	   : : "r" (_var_) : "cc")
>  
> -- 
> 2.39.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel