diff mbox

[PATCHv4,11/19] arm64: don't reload GPRs after apply_ssbd

Message ID 20180702110415.10465-12-mark.rutland@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Rutland July 2, 2018, 11:04 a.m. UTC
Now that all of the syscall logic works on the saved pt_regs, apply_ssbd
can safely corrupt x0-x3 in the entry paths, and we no longer need to
restore them. So let's remove the logic doing so.

With that logic gone, we can fold the branch target into the macro, so
that callers need not deal with this. GAS provides \@, which provides a
unique value per macro invocation, which we can use to create a unique
label.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/entry.S | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

Comments

Will Deacon July 6, 2018, 4:38 p.m. UTC | #1
On Mon, Jul 02, 2018 at 12:04:07PM +0100, Mark Rutland wrote:
> Now that all of the syscall logic works on the saved pt_regs, apply_ssbd
> can safely corrupt x0-x3 in the entry paths, and we no longer need to
> restore them. So let's remove the logic doing so.
> 
> With that logic gone, we can fold the branch target into the macro, so
> that callers need not deal with this. GAS provides \@, which provides a
> unique value per macro invocation, which we can use to create a unique
> label.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/entry.S | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index c41b84d06644..728bc7cc5bbb 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -130,20 +130,21 @@ alternative_else_nop_endif
>  
>  	// This macro corrupts x0-x3. It is the caller's duty
>  	// to save/restore them if required.
> -	.macro	apply_ssbd, state, targ, tmp1, tmp2
> +	.macro	apply_ssbd, state, tmp1, tmp2
>  #ifdef CONFIG_ARM64_SSBD
>  alternative_cb	arm64_enable_wa2_handling
> -	b	\targ
> +	b	skip_apply_ssbd\@
>  alternative_cb_end
>  	ldr_this_cpu	\tmp2, arm64_ssbd_callback_required, \tmp1
> -	cbz	\tmp2, \targ
> +	cbz	\tmp2, skip_apply_ssbd\@
>  	ldr	\tmp2, [tsk, #TSK_TI_FLAGS]
> -	tbnz	\tmp2, #TIF_SSBD, \targ
> +	tbnz	\tmp2, #TIF_SSBD, skip_apply_ssbd\@

Talking to Dave, he makes a good point that this is pretty fragile if a
macro expansion within the macro itself uses \@, since this would result
in an unexpected label update and everything would go wrong.

Can you default initialise an extra label argument to \@, or does that not
work?

Will
Mark Rutland July 9, 2018, 2:21 p.m. UTC | #2
On Fri, Jul 06, 2018 at 05:38:45PM +0100, Will Deacon wrote:
> On Mon, Jul 02, 2018 at 12:04:07PM +0100, Mark Rutland wrote:
> > Now that all of the syscall logic works on the saved pt_regs, apply_ssbd
> > can safely corrupt x0-x3 in the entry paths, and we no longer need to
> > restore them. So let's remove the logic doing so.
> > 
> > With that logic gone, we can fold the branch target into the macro, so
> > that callers need not deal with this. GAS provides \@, which provides a
> > unique value per macro invocation, which we can use to create a unique
> > label.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm64/kernel/entry.S | 20 +++++++-------------
> >  1 file changed, 7 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index c41b84d06644..728bc7cc5bbb 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -130,20 +130,21 @@ alternative_else_nop_endif
> >  
> >  	// This macro corrupts x0-x3. It is the caller's duty
> >  	// to save/restore them if required.
> > -	.macro	apply_ssbd, state, targ, tmp1, tmp2
> > +	.macro	apply_ssbd, state, tmp1, tmp2
> >  #ifdef CONFIG_ARM64_SSBD
> >  alternative_cb	arm64_enable_wa2_handling
> > -	b	\targ
> > +	b	skip_apply_ssbd\@
> >  alternative_cb_end
> >  	ldr_this_cpu	\tmp2, arm64_ssbd_callback_required, \tmp1
> > -	cbz	\tmp2, \targ
> > +	cbz	\tmp2, skip_apply_ssbd\@
> >  	ldr	\tmp2, [tsk, #TSK_TI_FLAGS]
> > -	tbnz	\tmp2, #TIF_SSBD, \targ
> > +	tbnz	\tmp2, #TIF_SSBD, skip_apply_ssbd\@
> 
> Talking to Dave, he makes a good point that this is pretty fragile if a
> macro expansion within the macro itself uses \@, since this would result
> in an unexpected label update and everything would go wrong.

I don't believe that's a problem; \@ is handled as-if it's a named
argument to the macro, and is not incremented within the scope of a
single macro expansion.

e.g. if I assemble:

----
.macro nop_macro
.endm

.macro a n
        a_pre_\n\()_\@:
        .quad   0
        a_post_\n\()_\@:
        .quad   0
.endm

.macro b n
        b_pre_\n\()_\@:
        .quad   0

        a \n

        b_post_\n\()_\@:
        .quad   0
.endm

.macro c n
        c_pre_\n\()_\@:
        .quad   0

        b \n

        c_post_\n\()_\@:
        .quad   0
.endm

.data

        a 0

        b 1

        c 2
----

... then objdump -D gives me:

----
Disassembly of section .data:

0000000000000000 <a_pre_0_0>:
        ...

0000000000000008 <a_post_0_0>:
        ...

0000000000000010 <b_pre_1_1>:
        ...

0000000000000018 <a_pre_1_2>:
        ...

0000000000000020 <a_post_1_2>:
        ...

0000000000000028 <b_post_1_1>:
        ...

0000000000000030 <c_pre_2_3>:
        ...

0000000000000038 <b_pre_2_4>:
        ...

0000000000000040 <a_pre_2_5>:
        ...

0000000000000048 <a_post_2_5>:
        ...

0000000000000050 <b_post_2_4>:
        ...

0000000000000058 <c_post_2_3>:
        ...

----

... where things are obviously nesting just fine.

Thanks,
Mark.
Dave Martin July 10, 2018, 10:37 a.m. UTC | #3
On Mon, Jul 09, 2018 at 03:21:59PM +0100, Mark Rutland wrote:
> On Fri, Jul 06, 2018 at 05:38:45PM +0100, Will Deacon wrote:
> > On Mon, Jul 02, 2018 at 12:04:07PM +0100, Mark Rutland wrote:
> > > Now that all of the syscall logic works on the saved pt_regs, apply_ssbd
> > > can safely corrupt x0-x3 in the entry paths, and we no longer need to
> > > restore them. So let's remove the logic doing so.
> > > 
> > > With that logic gone, we can fold the branch target into the macro, so
> > > that callers need not deal with this. GAS provides \@, which provides a
> > > unique value per macro invocation, which we can use to create a unique
> > > label.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > ---
> > >  arch/arm64/kernel/entry.S | 20 +++++++-------------
> > >  1 file changed, 7 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index c41b84d06644..728bc7cc5bbb 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -130,20 +130,21 @@ alternative_else_nop_endif
> > >  
> > >  	// This macro corrupts x0-x3. It is the caller's duty
> > >  	// to save/restore them if required.
> > > -	.macro	apply_ssbd, state, targ, tmp1, tmp2
> > > +	.macro	apply_ssbd, state, tmp1, tmp2
> > >  #ifdef CONFIG_ARM64_SSBD
> > >  alternative_cb	arm64_enable_wa2_handling
> > > -	b	\targ
> > > +	b	skip_apply_ssbd\@
> > >  alternative_cb_end
> > >  	ldr_this_cpu	\tmp2, arm64_ssbd_callback_required, \tmp1
> > > -	cbz	\tmp2, \targ
> > > +	cbz	\tmp2, skip_apply_ssbd\@
> > >  	ldr	\tmp2, [tsk, #TSK_TI_FLAGS]
> > > -	tbnz	\tmp2, #TIF_SSBD, \targ
> > > +	tbnz	\tmp2, #TIF_SSBD, skip_apply_ssbd\@
> > 
> > Talking to Dave, he makes a good point that this is pretty fragile if a
> > macro expansion within the macro itself uses \@, since this would result
> > in an unexpected label update and everything would go wrong.
> 
> I don't believe that's a problem; \@ is handled as-if it's a named
> argument to the macro, and is not incremented within the scope of a
> single macro expansion.

From
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/macro.c
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/read.c

it looks like the \@ counter (macro_number) is only incremented at the
end expansion of a whole macro body before gas reads the expansion
output (including recursive macro expansions).

So, your conclusion looks right for gas today.  The code implementing
this looks crufty enough to be pretty old.

Can you throw a bug into https://sourceware.org/bugzilla/ to get this
properly documented?  The current wording is ambiguous.  It would be
nice to get gas properly committed to behaving this way.


For the kernel, I suggest using a .L prefix so that the generated
symbols don't bloat the vmlinux symbol table (similar to numbered local
labels) -- unless you really want the symbols retained.

Having a common prefix for all "unique" assembler symbols may help us
to avoid namespace collisions, say

.L__asm__foo_\@
.L__asm__bar_\@

etc.

Cheers
---Dave

> 
> e.g. if I assemble:
> 
> ----
> .macro nop_macro
> .endm
> 
> .macro a n
>         a_pre_\n\()_\@:
>         .quad   0
>         a_post_\n\()_\@:
>         .quad   0
> .endm
> 
> .macro b n
>         b_pre_\n\()_\@:
>         .quad   0
> 
>         a \n
> 
>         b_post_\n\()_\@:
>         .quad   0
> .endm
> 
> .macro c n
>         c_pre_\n\()_\@:
>         .quad   0
> 
>         b \n
> 
>         c_post_\n\()_\@:
>         .quad   0
> .endm
> 
> .data
> 
>         a 0
> 
>         b 1
> 
>         c 2
> ----
> 
> ... then objdump -D gives me:
> 
> ----
> Disassembly of section .data:
> 
> 0000000000000000 <a_pre_0_0>:
>         ...
> 
> 0000000000000008 <a_post_0_0>:
>         ...
> 
> 0000000000000010 <b_pre_1_1>:
>         ...
> 
> 0000000000000018 <a_pre_1_2>:
>         ...
> 
> 0000000000000020 <a_post_1_2>:
>         ...
> 
> 0000000000000028 <b_post_1_1>:
>         ...
> 
> 0000000000000030 <c_pre_2_3>:
>         ...
> 
> 0000000000000038 <b_pre_2_4>:
>         ...
> 
> 0000000000000040 <a_pre_2_5>:
>         ...
> 
> 0000000000000048 <a_post_2_5>:
>         ...
> 
> 0000000000000050 <b_post_2_4>:
>         ...
> 
> 0000000000000058 <c_post_2_3>:
>         ...
> 
> ----
> 
> ... where things are obviously nesting just fine.
> 
> Thanks,
> Mark.
Will Deacon July 10, 2018, 5:33 p.m. UTC | #4
On Tue, Jul 10, 2018 at 11:37:24AM +0100, Dave Martin wrote:
> On Mon, Jul 09, 2018 at 03:21:59PM +0100, Mark Rutland wrote:
> > On Fri, Jul 06, 2018 at 05:38:45PM +0100, Will Deacon wrote:
> > > On Mon, Jul 02, 2018 at 12:04:07PM +0100, Mark Rutland wrote:
> > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > > index c41b84d06644..728bc7cc5bbb 100644
> > > > --- a/arch/arm64/kernel/entry.S
> > > > +++ b/arch/arm64/kernel/entry.S
> > > > @@ -130,20 +130,21 @@ alternative_else_nop_endif
> > > >  
> > > >  	// This macro corrupts x0-x3. It is the caller's duty
> > > >  	// to save/restore them if required.
> > > > -	.macro	apply_ssbd, state, targ, tmp1, tmp2
> > > > +	.macro	apply_ssbd, state, tmp1, tmp2
> > > >  #ifdef CONFIG_ARM64_SSBD
> > > >  alternative_cb	arm64_enable_wa2_handling
> > > > -	b	\targ
> > > > +	b	skip_apply_ssbd\@
> > > >  alternative_cb_end
> > > >  	ldr_this_cpu	\tmp2, arm64_ssbd_callback_required, \tmp1
> > > > -	cbz	\tmp2, \targ
> > > > +	cbz	\tmp2, skip_apply_ssbd\@
> > > >  	ldr	\tmp2, [tsk, #TSK_TI_FLAGS]
> > > > -	tbnz	\tmp2, #TIF_SSBD, \targ
> > > > +	tbnz	\tmp2, #TIF_SSBD, skip_apply_ssbd\@
> > > 
> > > Talking to Dave, he makes a good point that this is pretty fragile if a
> > > macro expansion within the macro itself uses \@, since this would result
> > > in an unexpected label update and everything would go wrong.
> > 
> > I don't believe that's a problem; \@ is handled as-if it's a named
> > argument to the macro, and is not incremented within the scope of a
> > single macro expansion.
> 
> From
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/macro.c
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/read.c
> 
> it looks like the \@ counter (macro_number) is only incremented at the
> end expansion of a whole macro body before gas reads the expansion
> output (including recursive macro expansions).
> 
> So, your conclusion looks right for gas today.  The code implementing
> this looks crufty enough to be pretty old.
> 
> Can you throw a bug into https://sourceware.org/bugzilla/ to get this
> properly documented?  The current wording is ambiguous.  It would be
> nice to get gas properly committed to behaving this way.
> 
> 
> For the kernel, I suggest using a .L prefix so that the generated
> symbols don't bloat the vmlinux symbol table (similar to numbered local
> labels) -- unless you really want the symbols retained.
> 
> Having a common prefix for all "unique" assembler symbols may help us
> to avoid namespace collisions, say
> 
> .L__asm__foo_\@
> .L__asm__bar_\@

Hmm, yes, and that would allow us to replace the open-coded labels in
our assembler.h macros as well, wouldn't it?

Will
Mark Rutland July 11, 2018, 9:46 a.m. UTC | #5
On Tue, Jul 10, 2018 at 06:33:18PM +0100, Will Deacon wrote:
> On Tue, Jul 10, 2018 at 11:37:24AM +0100, Dave Martin wrote:
> > For the kernel, I suggest using a .L prefix so that the generated
> > symbols don't bloat the vmlinux symbol table (similar to numbered local
> > labels) -- unless you really want the symbols retained.
> > 
> > Having a common prefix for all "unique" assembler symbols may help us
> > to avoid namespace collisions, say
> > 
> > .L__asm__foo_\@
> > .L__asm__bar_\@
> 
> Hmm, yes, and that would allow us to replace the open-coded labels in
> our assembler.h macros as well, wouldn't it?

Yup.

For this patch, I'll rename skip_apply_ssbd\@ to .L__asm_ssbd_skip\@.

I'll send another patch (or series) converting our other macros to this
scheme. We can either take that with this series, or as subsequent
cleanup.

Thanks,
Mark.
diff mbox

Patch

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c41b84d06644..728bc7cc5bbb 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -130,20 +130,21 @@  alternative_else_nop_endif
 
 	// This macro corrupts x0-x3. It is the caller's duty
 	// to save/restore them if required.
-	.macro	apply_ssbd, state, targ, tmp1, tmp2
+	.macro	apply_ssbd, state, tmp1, tmp2
 #ifdef CONFIG_ARM64_SSBD
 alternative_cb	arm64_enable_wa2_handling
-	b	\targ
+	b	skip_apply_ssbd\@
 alternative_cb_end
 	ldr_this_cpu	\tmp2, arm64_ssbd_callback_required, \tmp1
-	cbz	\tmp2, \targ
+	cbz	\tmp2, skip_apply_ssbd\@
 	ldr	\tmp2, [tsk, #TSK_TI_FLAGS]
-	tbnz	\tmp2, #TIF_SSBD, \targ
+	tbnz	\tmp2, #TIF_SSBD, skip_apply_ssbd\@
 	mov	w0, #ARM_SMCCC_ARCH_WORKAROUND_2
 	mov	w1, #\state
 alternative_cb	arm64_update_smccc_conduit
 	nop					// Patched to SMC/HVC #0
 alternative_cb_end
+skip_apply_ssbd\@:
 #endif
 	.endm
 
@@ -173,13 +174,7 @@  alternative_cb_end
 	ldr	x19, [tsk, #TSK_TI_FLAGS]	// since we can unmask debug
 	disable_step_tsk x19, x20		// exceptions when scheduling.
 
-	apply_ssbd 1, 1f, x22, x23
-
-#ifdef CONFIG_ARM64_SSBD
-	ldp	x0, x1, [sp, #16 * 0]
-	ldp	x2, x3, [sp, #16 * 1]
-#endif
-1:
+	apply_ssbd 1, x22, x23
 
 	mov	x29, xzr			// fp pointed to user-space
 	.else
@@ -321,8 +316,7 @@  alternative_if ARM64_WORKAROUND_845719
 alternative_else_nop_endif
 #endif
 3:
-	apply_ssbd 0, 5f, x0, x1
-5:
+	apply_ssbd 0, x0, x1
 	.endif
 
 	msr	elr_el1, x21			// set up the return data