| Message ID | 20180702110415.10465-12-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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.
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
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 --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