2024-04-10 02:41:52

by zhuqiuer

Subject: [PATCH v2] ARM: Add a memory clobber to the fmrx instruction

The instruction fmrx is used throughout the kernel,
where it is sometimes expected to be skipped
by incrementing the program counter, such as in vfpmodule.c:vfp_init().
Therefore, the instruction should not be reordered when it is not intended.
Adding a barrier() instruction before and after this call cannot prevent
reordering by the compiler, as the fmrx instruction is constrained
by '=r', meaning it works on the general register but not on memory.
To ensure the order of the instruction after compiling,
adding a memory clobber is necessary.

Below is the code snippet disassembled from the method:
vfpmodule.c:vfp_init(), compiled by LLVM.

Before the patching:
xxxxx: xxxxx bl c010c688 <register_undef_hook>
xxxxx: xxxxx mov r0, r4
xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
..
xxxxx: xxxxx bl c0791c8c <printk>
xxxxx: xxxxx movw r5, #23132 ; 0x5a5c
xxxxx: xxxxx vmrs r4, fpsid <- this is the fmrx instruction

After the patching:
xxxxx: xxxxx bl c010c688 <register_undef_hook>
xxxxx: xxxxx mov r0, r4
xxxxx: xxxxx vmrs r5, fpsid <- this is the fmrx instruction
xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>

Signed-off-by: zhuqiuer <[email protected]>
---
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..ae2c9b9b7701 100644
--- a/arch/arm/vfp/vfpinstr.h
+++ b/arch/arm/vfp/vfpinstr.h
@@ -68,14 +68,14 @@
u32 __v; \
asm(".fpu vfpv2\n" \
"vmrs %0, " #_vfp_ \
- : "=r" (__v) : : "cc"); \
+ : "=r" (__v) : : "memory", "cc"); \
__v; \
})

#define fmxr(_vfp_,_var_) \
asm(".fpu vfpv2\n" \
"vmsr " #_vfp_ ", %0" \
- : : "r" (_var_) : "cc")
+ : : "r" (_var_) : "memory", "cc")

#else

--
2.12.3

2024-04-10 12:31:28

by Ard Biesheuvel

Subject: Re: [PATCH v2] ARM: Add a memory clobber to the fmrx instruction

On Wed, 10 Apr 2024 at 04:41, zhuqiuer <[email protected]> wrote:
>
> The instruction fmrx is used throughout the kernel,
> where it is sometimes expected to be skipped
> by incrementing the program counter, such as in vfpmodule.c:vfp_init().
> Therefore, the instruction should not be reordered when it is not intended.
> Adding a barrier() instruction before and after this call cannot prevent
> reordering by the compiler, as the fmrx instruction is constrained
> by '=r', meaning it works on the general register but not on memory.
> To ensure the order of the instruction after compiling,
> adding a memory clobber is necessary.
>
> Below is the code snippet disassembled from the method:
> vfpmodule.c:vfp_init(), compiled by LLVM.
>
> Before the patching:
> xxxxx: xxxxx bl c010c688 <register_undef_hook>
> xxxxx: xxxxx mov r0, r4
> xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
> ...
> xxxxx: xxxxx bl c0791c8c <printk>
> xxxxx: xxxxx movw r5, #23132 ; 0x5a5c
> xxxxx: xxxxx vmrs r4, fpsid <- this is the fmrx instruction
>
> After the patching:
> xxxxx: xxxxx bl c010c688 <register_undef_hook>
> xxxxx: xxxxx mov r0, r4
> xxxxx: xxxxx vmrs r5, fpsid <- this is the fmrx instruction
> xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
>
> Signed-off-by: zhuqiuer <[email protected]>

This also fixes the issue I observed so

Reviewed-by: Ard Biesheuvel <[email protected]>


> ---
> 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..ae2c9b9b7701 100644
> --- a/arch/arm/vfp/vfpinstr.h
> +++ b/arch/arm/vfp/vfpinstr.h
> @@ -68,14 +68,14 @@
> u32 __v; \
> asm(".fpu vfpv2\n" \
> "vmrs %0, " #_vfp_ \
> - : "=r" (__v) : : "cc"); \
> + : "=r" (__v) : : "memory", "cc"); \
> __v; \
> })
>
> #define fmxr(_vfp_,_var_) \
> asm(".fpu vfpv2\n" \
> "vmsr " #_vfp_ ", %0" \
> - : : "r" (_var_) : "cc")
> + : : "r" (_var_) : "memory", "cc")
>
> #else
>
> --
> 2.12.3
>

2024-04-10 15:36:17

by Nathan Chancellor

Subject: Re: [PATCH v2] ARM: Add a memory clobber to the fmrx instruction

On Wed, Apr 10, 2024 at 02:31:11PM +0200, Ard Biesheuvel wrote:
> On Wed, 10 Apr 2024 at 04:41, zhuqiuer <[email protected]> wrote:
> >
> > The instruction fmrx is used throughout the kernel,
> > where it is sometimes expected to be skipped
> > by incrementing the program counter, such as in vfpmodule.c:vfp_init().
> > Therefore, the instruction should not be reordered when it is not intended.
> > Adding a barrier() instruction before and after this call cannot prevent
> > reordering by the compiler, as the fmrx instruction is constrained
> > by '=r', meaning it works on the general register but not on memory.
> > To ensure the order of the instruction after compiling,
> > adding a memory clobber is necessary.
> >
> > Below is the code snippet disassembled from the method:
> > vfpmodule.c:vfp_init(), compiled by LLVM.
> >
> > Before the patching:
> > xxxxx: xxxxx bl c010c688 <register_undef_hook>
> > xxxxx: xxxxx mov r0, r4
> > xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
> > ...
> > xxxxx: xxxxx bl c0791c8c <printk>
> > xxxxx: xxxxx movw r5, #23132 ; 0x5a5c
> > xxxxx: xxxxx vmrs r4, fpsid <- this is the fmrx instruction
> >
> > After the patching:
> > xxxxx: xxxxx bl c010c688 <register_undef_hook>
> > xxxxx: xxxxx mov r0, r4
> > xxxxx: xxxxx vmrs r5, fpsid <- this is the fmrx instruction
> > xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
> >
> > Signed-off-by: zhuqiuer <[email protected]>
>
> This also fixes the issue I observed so
>
> Reviewed-by: Ard Biesheuvel <[email protected]>

Reviewed-by: Nathan Chancellor <[email protected]>

This can probably go in Russell's patch tracker? Your patch had

Cc: [email protected]

in it, should this one as well?

> > ---
> > 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..ae2c9b9b7701 100644
> > --- a/arch/arm/vfp/vfpinstr.h
> > +++ b/arch/arm/vfp/vfpinstr.h
> > @@ -68,14 +68,14 @@
> > u32 __v; \
> > asm(".fpu vfpv2\n" \
> > "vmrs %0, " #_vfp_ \
> > - : "=r" (__v) : : "cc"); \
> > + : "=r" (__v) : : "memory", "cc"); \
> > __v; \
> > })
> >
> > #define fmxr(_vfp_,_var_) \
> > asm(".fpu vfpv2\n" \
> > "vmsr " #_vfp_ ", %0" \
> > - : : "r" (_var_) : "cc")
> > + : : "r" (_var_) : "memory", "cc")
> >
> > #else
> >
> > --
> > 2.12.3
> >

2024-04-11 07:16:08

by Ard Biesheuvel

Subject: Re: [PATCH v2] ARM: Add a memory clobber to the fmrx instruction

On Wed, 10 Apr 2024 at 17:35, Nathan Chancellor <[email protected]> wrote:
>
> On Wed, Apr 10, 2024 at 02:31:11PM +0200, Ard Biesheuvel wrote:
> > On Wed, 10 Apr 2024 at 04:41, zhuqiuer <[email protected]> wrote:
> > >
> > > The instruction fmrx is used throughout the kernel,
> > > where it is sometimes expected to be skipped
> > > by incrementing the program counter, such as in vfpmodule.c:vfp_init().
> > > Therefore, the instruction should not be reordered when it is not intended.
> > > Adding a barrier() instruction before and after this call cannot prevent
> > > reordering by the compiler, as the fmrx instruction is constrained
> > > by '=r', meaning it works on the general register but not on memory.
> > > To ensure the order of the instruction after compiling,
> > > adding a memory clobber is necessary.
> > >
> > > Below is the code snippet disassembled from the method:
> > > vfpmodule.c:vfp_init(), compiled by LLVM.
> > >
> > > Before the patching:
> > > xxxxx: xxxxx bl c010c688 <register_undef_hook>
> > > xxxxx: xxxxx mov r0, r4
> > > xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
> > > ...
> > > xxxxx: xxxxx bl c0791c8c <printk>
> > > xxxxx: xxxxx movw r5, #23132 ; 0x5a5c
> > > xxxxx: xxxxx vmrs r4, fpsid <- this is the fmrx instruction
> > >
> > > After the patching:
> > > xxxxx: xxxxx bl c010c688 <register_undef_hook>
> > > xxxxx: xxxxx mov r0, r4
> > > xxxxx: xxxxx vmrs r5, fpsid <- this is the fmrx instruction
> > > xxxxx: xxxxx bl c010c6e4 <unregister_undef_hook>
> > >
> > > Signed-off-by: zhuqiuer <[email protected]>
> >
> > This also fixes the issue I observed so
> >
> > Reviewed-by: Ard Biesheuvel <[email protected]>
>
> Reviewed-by: Nathan Chancellor <[email protected]>
>
> This can probably go in Russell's patch tracker?

Yes.

> Your patch had
>
> Cc: [email protected]
>
> in it, should this one as well?
>

Yes.


> > > ---
> > > 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..ae2c9b9b7701 100644
> > > --- a/arch/arm/vfp/vfpinstr.h
> > > +++ b/arch/arm/vfp/vfpinstr.h
> > > @@ -68,14 +68,14 @@
> > > u32 __v; \
> > > asm(".fpu vfpv2\n" \
> > > "vmrs %0, " #_vfp_ \
> > > - : "=r" (__v) : : "cc"); \
> > > + : "=r" (__v) : : "memory", "cc"); \
> > > __v; \
> > > })
> > >
> > > #define fmxr(_vfp_,_var_) \
> > > asm(".fpu vfpv2\n" \
> > > "vmsr " #_vfp_ ", %0" \
> > > - : : "r" (_var_) : "cc")
> > > + : : "r" (_var_) : "memory", "cc")
> > >
> > > #else
> > >
> > > --
> > > 2.12.3
> > >