diff mbox series

x86/mm/encrypt: Use %a asm operand modifier to obtain %rip-relative address

Message ID 20231120153419.3045-1-ubizjak@gmail.com
State New
Headers
Series x86/mm/encrypt: Use %a asm operand modifier to obtain %rip-relative address |

Commit Message

Uros Bizjak Nov. 20, 2023, 3:33 p.m. UTC 
  The "a" asm operand modifier substitutes a memory reference, with the
actual operand treated as address.  For x86_64, when a symbol is
provided, the "a" modifier emits "sym(%rip)" instead of "sym".

Clang allows only "i" and "r" operand constraints with an "a" modifier,
so the patch normalizes the modifier/constraint pair to "a"/"i"
which is consistent between both compilers.

No functional change intended.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 arch/x86/mm/mem_encrypt_identity.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

Comments

Borislav Petkov Nov. 20, 2023, 5:15 p.m. UTC | #1 
On Mon, Nov 20, 2023 at 04:33:50PM +0100, Uros Bizjak wrote:
> The "a" asm operand modifier substitutes a memory reference, with the
> actual operand treated as address.  For x86_64, when a symbol is
> provided, the "a" modifier emits "sym(%rip)" instead of "sym".
> 
> Clang allows only "i" and "r" operand constraints with an "a" modifier,
> so the patch normalizes the modifier/constraint pair to "a"/"i"

s/the patch normalizes/normalize/

> which is consistent between both compilers.
> 
> No functional change intended.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>  arch/x86/mm/mem_encrypt_identity.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index d73aeb16417f..6a351fdd1b39 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -346,9 +346,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
>  	 * We're running identity mapped, so we must obtain the address to the
>  	 * SME encryption workarea using rip-relative addressing.
>  	 */
> -	asm ("lea sme_workarea(%%rip), %0"
> -	     : "=r" (workarea_start)
> -	     : "p" (sme_workarea));
> +	asm ("lea %a1, %0" : "=r" (workarea_start) : "i" (sme_workarea));

Yeah, I saw that particular subthread today.

Are you sure this "a" modifier DTRT with all gcc version we support?

I.e., from 5.1 onwards...

Just making sure.

Thx.
Uros Bizjak Nov. 20, 2023, 7:58 p.m. UTC | #2 
On Mon, Nov 20, 2023 at 6:15 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Nov 20, 2023 at 04:33:50PM +0100, Uros Bizjak wrote:
> > The "a" asm operand modifier substitutes a memory reference, with the
> > actual operand treated as address.  For x86_64, when a symbol is
> > provided, the "a" modifier emits "sym(%rip)" instead of "sym".
> >
> > Clang allows only "i" and "r" operand constraints with an "a" modifier,
> > so the patch normalizes the modifier/constraint pair to "a"/"i"
>
> s/the patch normalizes/normalize/
>
> > which is consistent between both compilers.
> >
> > No functional change intended.
> >
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
> >  arch/x86/mm/mem_encrypt_identity.c | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > index d73aeb16417f..6a351fdd1b39 100644
> > --- a/arch/x86/mm/mem_encrypt_identity.c
> > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > @@ -346,9 +346,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
> >        * We're running identity mapped, so we must obtain the address to the
> >        * SME encryption workarea using rip-relative addressing.
> >        */
> > -     asm ("lea sme_workarea(%%rip), %0"
> > -          : "=r" (workarea_start)
> > -          : "p" (sme_workarea));
> > +     asm ("lea %a1, %0" : "=r" (workarea_start) : "i" (sme_workarea));
>
> Yeah, I saw that particular subthread today.
>
> Are you sure this "a" modifier DTRT with all gcc version we support?
>
> I.e., from 5.1 onwards...

Yes [1].

[1] https://godbolt.org/z/aWe7b16rT

Thanks,
Uros.
Kirill A. Shutemov Nov. 21, 2023, 9 a.m. UTC | #3 
On Mon, Nov 20, 2023 at 08:58:29PM +0100, Uros Bizjak wrote:
> On Mon, Nov 20, 2023 at 6:15 PM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Mon, Nov 20, 2023 at 04:33:50PM +0100, Uros Bizjak wrote:
> > > The "a" asm operand modifier substitutes a memory reference, with the
> > > actual operand treated as address.  For x86_64, when a symbol is
> > > provided, the "a" modifier emits "sym(%rip)" instead of "sym".
> > >
> > > Clang allows only "i" and "r" operand constraints with an "a" modifier,
> > > so the patch normalizes the modifier/constraint pair to "a"/"i"
> >
> > s/the patch normalizes/normalize/
> >
> > > which is consistent between both compilers.
> > >
> > > No functional change intended.
> > >
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > Cc: Andy Lutomirski <luto@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Cc: Borislav Petkov <bp@alien8.de>
> > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > > ---
> > >  arch/x86/mm/mem_encrypt_identity.c | 16 ++++------------
> > >  1 file changed, 4 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > > index d73aeb16417f..6a351fdd1b39 100644
> > > --- a/arch/x86/mm/mem_encrypt_identity.c
> > > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > > @@ -346,9 +346,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
> > >        * We're running identity mapped, so we must obtain the address to the
> > >        * SME encryption workarea using rip-relative addressing.
> > >        */
> > > -     asm ("lea sme_workarea(%%rip), %0"
> > > -          : "=r" (workarea_start)
> > > -          : "p" (sme_workarea));
> > > +     asm ("lea %a1, %0" : "=r" (workarea_start) : "i" (sme_workarea));
> >
> > Yeah, I saw that particular subthread today.
> >
> > Are you sure this "a" modifier DTRT with all gcc version we support?
> >
> > I.e., from 5.1 onwards...
> 
> Yes [1].
> 
> [1] https://godbolt.org/z/aWe7b16rT

But Clang generates RIP-relative load only starting from version 15.
Clange 11 claimed to be supported.
Uros Bizjak Nov. 21, 2023, 9:29 a.m. UTC | #4 
On Tue, Nov 21, 2023 at 10:05 AM Kirill A. Shutemov
<kirill@shutemov.name> wrote:
>
> On Mon, Nov 20, 2023 at 08:58:29PM +0100, Uros Bizjak wrote:
> > On Mon, Nov 20, 2023 at 6:15 PM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Mon, Nov 20, 2023 at 04:33:50PM +0100, Uros Bizjak wrote:
> > > > The "a" asm operand modifier substitutes a memory reference, with the
> > > > actual operand treated as address.  For x86_64, when a symbol is
> > > > provided, the "a" modifier emits "sym(%rip)" instead of "sym".
> > > >
> > > > Clang allows only "i" and "r" operand constraints with an "a" modifier,
> > > > so the patch normalizes the modifier/constraint pair to "a"/"i"
> > >
> > > s/the patch normalizes/normalize/
> > >
> > > > which is consistent between both compilers.
> > > >
> > > > No functional change intended.
> > > >
> > > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > > Cc: Andy Lutomirski <luto@kernel.org>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > Cc: Borislav Petkov <bp@alien8.de>
> > > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > > > ---
> > > >  arch/x86/mm/mem_encrypt_identity.c | 16 ++++------------
> > > >  1 file changed, 4 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > > > index d73aeb16417f..6a351fdd1b39 100644
> > > > --- a/arch/x86/mm/mem_encrypt_identity.c
> > > > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > > > @@ -346,9 +346,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
> > > >        * We're running identity mapped, so we must obtain the address to the
> > > >        * SME encryption workarea using rip-relative addressing.
> > > >        */
> > > > -     asm ("lea sme_workarea(%%rip), %0"
> > > > -          : "=r" (workarea_start)
> > > > -          : "p" (sme_workarea));
> > > > +     asm ("lea %a1, %0" : "=r" (workarea_start) : "i" (sme_workarea));
> > >
> > > Yeah, I saw that particular subthread today.
> > >
> > > Are you sure this "a" modifier DTRT with all gcc version we support?
> > >
> > > I.e., from 5.1 onwards...
> >
> > Yes [1].
> >
> > [1] https://godbolt.org/z/aWe7b16rT
>
> But Clang generates RIP-relative load only starting from version 15.
> Clange 11 claimed to be supported.

Huh, what a bummer...

The patch is retracted then.

Thanks,
Uros.
diff mbox series

Patch

diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index d73aeb16417f..6a351fdd1b39 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -346,9 +346,7 @@  void __init sme_encrypt_kernel(struct boot_params *bp)
 	 * We're running identity mapped, so we must obtain the address to the
 	 * SME encryption workarea using rip-relative addressing.
 	 */
-	asm ("lea sme_workarea(%%rip), %0"
-	     : "=r" (workarea_start)
-	     : "p" (sme_workarea));
+	asm ("lea %a1, %0" : "=r" (workarea_start) : "i" (sme_workarea));
 
 	/*
 	 * Calculate required number of workarea bytes needed:
@@ -582,15 +580,9 @@  void __init sme_enable(struct boot_params *bp)
 	 * identity mapped, so we must obtain the address to the SME command
 	 * line argument data using rip-relative addressing.
 	 */
-	asm ("lea sme_cmdline_arg(%%rip), %0"
-	     : "=r" (cmdline_arg)
-	     : "p" (sme_cmdline_arg));
-	asm ("lea sme_cmdline_on(%%rip), %0"
-	     : "=r" (cmdline_on)
-	     : "p" (sme_cmdline_on));
-	asm ("lea sme_cmdline_off(%%rip), %0"
-	     : "=r" (cmdline_off)
-	     : "p" (sme_cmdline_off));
+	asm ("lea %a1, %0" : "=r" (cmdline_arg) : "i" (sme_cmdline_arg));
+	asm ("lea %a1, %0" : "=r" (cmdline_on) : "i" (sme_cmdline_on));
+	asm ("lea %a1, %0" : "=r" (cmdline_off) : "i" (sme_cmdline_off));
 
 	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT))
 		active_by_default = true;