From: Nathan Chancellor <nathan@kernel.org> To: Ard Biesheuvel <ardb@kernel.org> Cc: zhuqiuer <zhuqiuer1@huawei.com>, justinstitt@google.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux@armlinux.org.uk, llvm@lists.linux.dev, morbo@google.com, ndesaulniers@google.com Subject: Re: [PATCH v2] ARM: Add a memory clobber to the fmrx instruction Date: Wed, 10 Apr 2024 08:35:26 -0700 [thread overview] Message-ID: <20240410153526.GA3904754@dev-arch.thelio-3990X> (raw) In-Reply-To: <CAMj1kXFdR49n2Je5jYSdbWRZbMGoZenfaqDX3iCQFVZUecrCmw@mail.gmail.com> On Wed, Apr 10, 2024 at 02:31:11PM +0200, Ard Biesheuvel wrote: > On Wed, 10 Apr 2024 at 04:41, zhuqiuer <zhuqiuer1@huawei.com> 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 <zhuqiuer1@huawei.com> > > This also fixes the issue I observed so > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Nathan Chancellor <nathan@kernel.org> This can probably go in Russell's patch tracker? Your patch had Cc: stable@vger.kernel.org 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 > >
WARNING: multiple messages have this Message-ID (diff)
From: Nathan Chancellor <nathan@kernel.org> To: Ard Biesheuvel <ardb@kernel.org> Cc: zhuqiuer <zhuqiuer1@huawei.com>, justinstitt@google.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux@armlinux.org.uk, llvm@lists.linux.dev, morbo@google.com, ndesaulniers@google.com Subject: Re: [PATCH v2] ARM: Add a memory clobber to the fmrx instruction Date: Wed, 10 Apr 2024 08:35:26 -0700 [thread overview] Message-ID: <20240410153526.GA3904754@dev-arch.thelio-3990X> (raw) In-Reply-To: <CAMj1kXFdR49n2Je5jYSdbWRZbMGoZenfaqDX3iCQFVZUecrCmw@mail.gmail.com> On Wed, Apr 10, 2024 at 02:31:11PM +0200, Ard Biesheuvel wrote: > On Wed, 10 Apr 2024 at 04:41, zhuqiuer <zhuqiuer1@huawei.com> 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 <zhuqiuer1@huawei.com> > > This also fixes the issue I observed so > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Nathan Chancellor <nathan@kernel.org> This can probably go in Russell's patch tracker? Your patch had Cc: stable@vger.kernel.org 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 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-04-10 15:35 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-04-09 11:38 [PATCH] ARM: Add a memory clobber to the fmrx instruction zhuqiuer 2024-04-09 11:38 ` zhuqiuer 2024-04-09 16:46 ` Nathan Chancellor 2024-04-09 16:46 ` Nathan Chancellor 2024-04-10 2:41 ` zhuqiuer 2024-04-10 2:41 ` zhuqiuer 2024-04-10 2:41 ` [PATCH v2] " zhuqiuer 2024-04-10 2:41 ` zhuqiuer 2024-04-10 12:31 ` Ard Biesheuvel 2024-04-10 12:31 ` Ard Biesheuvel 2024-04-10 15:35 ` Nathan Chancellor [this message] 2024-04-10 15:35 ` Nathan Chancellor 2024-04-11 7:02 ` Ard Biesheuvel 2024-04-11 7:02 ` Ard Biesheuvel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240410153526.GA3904754@dev-arch.thelio-3990X \ --to=nathan@kernel.org \ --cc=ardb@kernel.org \ --cc=justinstitt@google.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=llvm@lists.linux.dev \ --cc=morbo@google.com \ --cc=ndesaulniers@google.com \ --cc=zhuqiuer1@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.