objtool, x86/alternative: Convert objtool annotations and x86 alternatives to assembler macros
Subsystems (7)
Josh Poimboeuf
maintainercover objtool, x86/alternative: Convert objtool annotations and x86 alternatives to assembler macros
Convert objtool annotations and x86 alternatives to assembler macros. This creates a shared C/asm alternative implementation and makes the code generation significantly more readable.
Patches 1-5: convert objtool
Patches 6-8: convert alternatives
Based on tip/master.
Before:
-----
# ./arch/x86/include/asm/smap.h:47: alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
# ALT: oldinstr
771:
772:
# ALT: padding
.skip -(((775f-774f)-(772b-771b)) > 0) * ((775f-774f)-(772b-771b)),0x90 773:
.pushsection .altinstructions, "aM", @progbits, 14
.long 771b - .
.long 774f - .
.4byte ( 9*32+20)
.byte 773b-771b
.byte 775f-774f
.popsection
.pushsection .altinstr_replacement, "ax"
.pushsection .discard.annotate_data, "M", @progbits, 8; .long 1b - ., 1; .popsection # ALT: replacement
774:
stac
775:
.popsection
-----
After:
-----
# ./arch/x86/include/asm/smap.h:47: alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
ALTERNATIVE "", "stac", "( 9*32+20)"
-----
Future work:
- BUG()/WARN()
- static_cpu_has()
- static calls/branches
- exceptions
- smp locks
- unwind hints
Josh Poimboeuf (8):
objtool: Rename ANNOTYPE_IGNORE_ALTS -> ANNOTYPE_IGNORE_ALTERNATIVE
objtool: Make ANNOTYPE_DATA_SPECIAL unique across all annotations
objtool: Rename C ANNOTATE_REACHABLE to ANNOTATE_REACHABLE_LABEL
objtool: Rename asm ANNOTATE_NOCFI_SYM to ANNOTATE_NOCFI
objtool: Convert annotations to assembler macros x86/asm: Use unique code labels in __FILL_RETURN_BUFFER x86/asm: Remove newlines in alternatives x86/alternative: Convert alternatives to assembler macros
arch/x86/include/asm/alternative.h | 196 +++++++++++++------------- arch/x86/include/asm/asm.h | 8 +- arch/x86/include/asm/bug.h | 2 +- arch/x86/include/asm/irq_stack.h | 2 +- arch/x86/include/asm/jump_label.h | 1 + arch/x86/include/asm/nospec-branch.h | 8 +- arch/x86/include/asm/paravirt_types.h | 2 +- arch/x86/include/asm/processor.h | 2 +- arch/x86/include/asm/smap.h | 8 +- arch/x86/include/asm/vmware.h | 2 +- arch/x86/platform/efi/efi_stub_64.S | 2 +- include/linux/annotate.h | 133 ++++++++--------- include/linux/objtool_types.h | 5 +- tools/include/linux/objtool_types.h | 5 +- tools/objtool/check.c | 4 +- 15 files changed, 196 insertions(+), 184 deletions(-)
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerPATCH 1/8 objtool: Rename ANNOTYPE_IGNORE_ALTS -> ANNOTYPE_IGNORE_ALTERNATIVE
Rename ANNOTYPE_IGNORE_ALTS to ANNOTYPE_IGNORE_ALTERNATIVE for consistency with its corresponding ANNOTATE_IGNORE_ALTERNATIVE macro.
--- include/linux/annotate.h | 4 ++-- include/linux/objtool_types.h | 2 +- tools/include/linux/objtool_types.h | 2 +- tools/objtool/check.c | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-)
include/linux/annotate.h +2 -2
@@ -75,7 +75,7 @@* objtool annotation to ignore the alternatives and only consider the original * instruction(s). */ -#define ANNOTATE_IGNORE_ALTERNATIVE ASM_ANNOTATE(ANNOTYPE_IGNORE_ALTS) +#define ANNOTATE_IGNORE_ALTERNATIVE ASM_ANNOTATE(ANNOTYPE_IGNORE_ALTERNATIVE)/* * This macro indicates that the following intra-function call is valid. * Any non-annotated intra-function call will cause objtool to issue a warning.
@@ -116,7 +116,7 @@#define ANNOTATE_RETPOLINE_SAFE ANNOTATE type=ANNOTYPE_RETPOLINE_SAFE/* ANNOTATE_INSTR_BEGIN ANNOTATE type=ANNOTYPE_INSTR_BEGIN */ /* ANNOTATE_INSTR_END ANNOTATE type=ANNOTYPE_INSTR_END */ -#define ANNOTATE_IGNORE_ALTERNATIVE ANNOTATE type=ANNOTYPE_IGNORE_ALTS +#define ANNOTATE_IGNORE_ALTERNATIVE ANNOTATE type=ANNOTYPE_IGNORE_ALTERNATIVE#define ANNOTATE_INTRA_FUNCTION_CALL ANNOTATE type=ANNOTYPE_INTRA_FUNCTION_CALL#define ANNOTATE_UNRET_BEGIN ANNOTATE type=ANNOTYPE_UNRET_BEGIN#define ANNOTATE_REACHABLE ANNOTATE type=ANNOTYPE_REACHABLE
include/linux/objtool_types.h +1 -1
@@ -62,7 +62,7 @@ struct unwind_hint {#define ANNOTYPE_INSTR_BEGIN 3#define ANNOTYPE_INSTR_END 4#define ANNOTYPE_UNRET_BEGIN 5 -#define ANNOTYPE_IGNORE_ALTS 6 +#define ANNOTYPE_IGNORE_ALTERNATIVE 6#define ANNOTYPE_INTRA_FUNCTION_CALL 7#define ANNOTYPE_REACHABLE 8#define ANNOTYPE_NOCFI 9
tools/include/linux/objtool_types.h +1 -1
@@ -62,7 +62,7 @@ struct unwind_hint {#define ANNOTYPE_INSTR_BEGIN 3#define ANNOTYPE_INSTR_END 4#define ANNOTYPE_UNRET_BEGIN 5 -#define ANNOTYPE_IGNORE_ALTS 6 +#define ANNOTYPE_IGNORE_ALTERNATIVE 6#define ANNOTYPE_INTRA_FUNCTION_CALL 7#define ANNOTYPE_REACHABLE 8#define ANNOTYPE_NOCFI 9
tools/objtool/check.c +2 -2
@@ -2358,7 +2358,7 @@ static int __annotate_early(struct objtool_file *file, int type, struct instructswitch (type) {/* Must be before add_special_section_alts() */ -case ANNOTYPE_IGNORE_ALTS : +case ANNOTYPE_IGNORE_ALTERNATIVE : insn->ignore_alts = true;break ;
@@ -2439,7 +2439,7 @@ static int __annotate_late(struct objtool_file *file, int type, struct instructi insn->unret =1 ;break ; -case ANNOTYPE_IGNORE_ALTS : +case ANNOTYPE_IGNORE_ALTERNATIVE :/* early */ break ;
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerPATCH 2/8 objtool: Make ANNOTYPE_DATA_SPECIAL unique across all annotations
In reply to Josh PoimboeufWhile text annotations and data annotations are placed in separate sections, their underlying format (and creation macros) are the same. Use the same ANNOTYPE value space for both to reduce the possibility of clashes.
--- include/linux/objtool_types.h | 3 +-- tools/include/linux/objtool_types.h | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
include/linux/objtool_types.h +1 -2
@@ -66,7 +66,6 @@ struct unwind_hint {#define ANNOTYPE_INTRA_FUNCTION_CALL 7#define ANNOTYPE_REACHABLE 8#define ANNOTYPE_NOCFI 9 - -#define ANNOTYPE_DATA_SPECIAL 1 +#define ANNOTYPE_DATA_SPECIAL 10#endif /* _LINUX_OBJTOOL_TYPES_H */
tools/include/linux/objtool_types.h +1 -2
@@ -66,7 +66,6 @@ struct unwind_hint {#define ANNOTYPE_INTRA_FUNCTION_CALL 7#define ANNOTYPE_REACHABLE 8#define ANNOTYPE_NOCFI 9 - -#define ANNOTYPE_DATA_SPECIAL 1 +#define ANNOTYPE_DATA_SPECIAL 10#endif /* _LINUX_OBJTOOL_TYPES_H */
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerPATCH 3/8 objtool: Rename C ANNOTATE_REACHABLE to ANNOTATE_REACHABLE_LABEL
In reply to Josh PoimboeufUnlike the asm version, the C version of ANNOTATE_REACHABLE takes an argument. Rename it to ANNOTATE_REACHABLE_LABEL accordingly.
--- arch/x86/include/asm/bug.h | 2 +- arch/x86/include/asm/irq_stack.h | 2 +- include/linux/annotate.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/include/asm/bug.h +1 -1
@@ -125,7 +125,7 @@ do { \* message out than not know about it. */ -#define ARCH_WARN_REACHABLE ANNOTATE_REACHABLE(1b) +#define ARCH_WARN_REACHABLE ANNOTATE_REACHABLE_LABEL(1b)#define __WARN_FLAGS (cond_str, flags) \ do { \
arch/x86/include/asm/irq_stack.h +1 -1
@@ -101,7 +101,7 @@#define ASM_CALL_ARG0 \ "1: call %c[__func] \n" \ - ANNOTATE_REACHABLE(1b) " \n" + ANNOTATE_REACHABLE_LABEL(1b) " \n"#define ASM_CALL_ARG1 \ "movq %[arg1], %%rdi \n" \
include/linux/annotate.h +1 -1
@@ -94,7 +94,7 @@* terminating, like a noreturn CALL or UD2 when we know they are not -- eg * WARN using UD2. */ -#define ANNOTATE_REACHABLE (label) ASM_ANNOTATE_LABEL(label, ANNOTYPE_REACHABLE) +#define ANNOTATE_REACHABLE_LABEL (label) ASM_ANNOTATE_LABEL(label, ANNOTYPE_REACHABLE)/* * This should not be used; it annotates away CFI violations. There are a few * valid use cases like kexec handover to the next kernel image, and there is
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerPATCH 4/8 objtool: Rename asm ANNOTATE_NOCFI_SYM to ANNOTATE_NOCFI
In reply to Josh PoimboeufUnlike the C version, the asm version of ANNOTATE_NOCFI_SYM doesn't take an argument. Rename it to ANNOTATE_NOCFI accordingly.
--- arch/x86/platform/efi/efi_stub_64.S | 2 +- include/linux/annotate.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/platform/efi/efi_stub_64.S +1 -1
@@ -14,7 +14,7 @@ SYM_FUNC_START(__efi_call) /* * The EFI code doesn't have any CFI, annotate away the CFI violation. */ - ANNOTATE_NOCFI_SYM + ANNOTATE_NOCFI pushq %rbp movq %rsp, %rbp and $~0xf, %rsp
include/linux/annotate.h +1 -1
@@ -120,7 +120,7 @@#define ANNOTATE_INTRA_FUNCTION_CALL ANNOTATE type=ANNOTYPE_INTRA_FUNCTION_CALL#define ANNOTATE_UNRET_BEGIN ANNOTATE type=ANNOTYPE_UNRET_BEGIN#define ANNOTATE_REACHABLE ANNOTATE type=ANNOTYPE_REACHABLE -#define ANNOTATE_NOCFI_SYM ANNOTATE type=ANNOTYPE_NOCFI +#define ANNOTATE_NOCFI ANNOTATE type=ANNOTYPE_NOCFI#define ANNOTATE_DATA_SPECIAL ANNOTATE_DATA type=ANNOTYPE_DATA_SPECIAL#endif /* __ASSEMBLY__ */
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerPATCH 5/8 objtool: Convert annotations to assembler macros
In reply to Josh PoimboeufImprove code generation readability by converting the objtool annotations into assembler macros which are created when annotate.h is included.
Before:
.pushsection .discard.annotate_insn, "M", @progbits, 8; .long 1b - ., 8; .popsection
After:
ANNOTATE_REACHABLE loc=1b
--- arch/x86/include/asm/asm.h | 8 +- arch/x86/include/asm/jump_label.h | 1 + include/linux/annotate.h | 132 +++++++++++++++--------------- 3 files changed, 72 insertions(+), 69 deletions(-)
arch/x86/include/asm/asm.h +6 -2
@@ -2,15 +2,15 @@#ifndef _ASM_X86_ASM_H#define _ASM_X86_ASM_H -#include <linux/annotate.h> -#ifdef __ASSEMBLER__ +# define __ASM_C (a,b) a# define __ASM_FORM (x, ...) x,## __VA_ARGS__# define __ASM_FORM_RAW (x, ...) x,## __VA_ARGS__# define __ASM_FORM_COMMA (x, ...) x,## __VA_ARGS__,# define __ASM_REGPFX %#else #include <linux/stringify.h> +# define __ASM_C (a,b) b# define __ASM_FORM (x, ...) " " __stringify(x,##__VA_ARGS__) " "# define __ASM_FORM_RAW (x, ...) __stringify(x,##__VA_ARGS__)# define __ASM_FORM_COMMA (x, ...) " " __stringify(x,##__VA_ARGS__) ","
@@ -115,6 +115,10 @@#endif +#define DEFINE_MACRO (name) \ + __ASM_C(DEFINE_ ## name, \ + asm(__stringify(DEFINE_ ## name))) +#ifndef __ASSEMBLER__ static __always_inline __pure void *rip_rel_ptr(void *p) {
arch/x86/include/asm/jump_label.h +1 -0
@@ -11,6 +11,7 @@#include <linux/stringify.h> #include <linux/types.h> +#include <linux/annotate.h> #define JUMP_TABLE_ENTRY (key, label) \ ".pushsection __jump_table, \"aw\" \n\t" \
include/linux/annotate.h +65 -67
186 lines not on this page · raw message
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerRe: [PATCH 5/8] objtool: Convert annotations to assembler macros
Quoted 6 lines · Josh Poimboeuf
Improve code generation readability by converting the objtool annotations into assembler macros which are created when
annotate.his included.Before:
Quoted 7 lines
.pushsection .discard.annotate_insn, "M", @progbits, 8; .long 1b - ., 8; .popsection
After:
ANNOTATE_REACHABLEloc=1bSigned-off-by Josh Poimboeuf <[email protected]>
This needs the following to build on non-x86:
arch/x86/include/asm/asm.h +0 -6
@@ -3,14 +3,12 @@#define _ASM_X86_ASM_H#ifdef __ASSEMBLER__ -# define __ASM_C (a,b) a# define __ASM_FORM (x, ...) x,## __VA_ARGS__# define __ASM_FORM_RAW (x, ...) x,## __VA_ARGS__# define __ASM_FORM_COMMA (x, ...) x,## __VA_ARGS__,# define __ASM_REGPFX %#else #include <linux/stringify.h> -# define __ASM_C (a,b) b# define __ASM_FORM (x, ...) " " __stringify(x,##__VA_ARGS__) " "# define __ASM_FORM_RAW (x, ...) __stringify(x,##__VA_ARGS__)# define __ASM_FORM_COMMA (x, ...) " " __stringify(x,##__VA_ARGS__) ","
@@ -115,10 +113,6 @@#endif -#define DEFINE_MACRO (name) \ - __ASM_C(DEFINE_ ## name, \ - asm(__stringify(DEFINE_ ## name))) -#ifndef __ASSEMBLER__ static __always_inline __pure void *rip_rel_ptr(void *p) {
include/linux/annotate.h +1 -1
@@ -4,7 +4,7 @@#include <linux/objtool_types.h> #include <linux/stringify.h> -#include <asm/asm.h> +#include <linux/asm.h> #define DEFINE___ANNOTATE \ .macro __ANNOTATE sec, type, loc; \
include/linux/asm.h new file +15 -0
@@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_ASM_H +#define _LINUX_ASM_H + +#ifdef __ASSEMBLER__ +# define __ASM_C (a,b) a +#else +# define __ASM_C (a,b) b +#endif + +#define DEFINE_MACRO (name) \ + __ASM_C(DEFINE_ ## name, \ + asm(__stringify(DEFINE_ ## name))) + +#endif /* _LINUX_ASM_H */
Peter Zijlstra
maintainerRe: [PATCH 5/8] objtool: Convert annotations to assembler macros
In reply to Josh PoimboeufQuoted 83 lines · Josh Poimboeuf
Improve code generation readability by converting the objtool annotations into assembler macros which are created when
annotate.his included.Before:
.pushsection .discard.annotate_insn, "M", @progbits, 8; .long 1b - ., 8; .popsection
After:
ANNOTATE_REACHABLEloc=1bSigned-off-by Josh Poimboeuf <[email protected]>arch/x86/include/asm/asm.h +6
-2@@ -2,15 +2,15 @@#ifndef _ASM_X86_ASM_H#define _ASM_X86_ASM_H -#include <linux/annotate.h> -#ifdef __ASSEMBLER__ +# define __ASM_C (a,b) a# define __ASM_FORM (x, ...) x,## __VA_ARGS__# define __ASM_FORM_RAW (x, ...) x,## __VA_ARGS__# define __ASM_FORM_COMMA (x, ...) x,## __VA_ARGS__,# define __ASM_REGPFX %#else #include <linux/stringify.h> +# define __ASM_C (a,b) b# define __ASM_FORM (x, ...) " " __stringify(x,##__VA_ARGS__) " "# define __ASM_FORM_RAW (x, ...) __stringify(x,##__VA_ARGS__)# define __ASM_FORM_COMMA (x, ...) " " __stringify(x,##__VA_ARGS__) ","@@ -115,6 +115,10 @@#endif +#define DEFINE_MACRO (name) \ + __ASM_C(DEFINE_ ## name, \ + asm(__stringify(DEFINE_ ## name))) +#ifndef __ASSEMBLER__ static __always_inline __pure void *rip_rel_ptr(void *p) {include/linux/annotate.h +24
-0@@ -3,53 +3,43 @@#define _LINUX_ANNOTATE_H#include <linux/objtool_types.h> +#include <linux/stringify.h> +#include <asm/asm.h> + +#define DEFINE___ANNOTATE \ + .macro __ANNOTATE sec, type, loc; \ + .pushsection __ASM_C(\sec, \\sec), "M", @progbits, 8; \ + .long __ASM_C(\loc, \\loc) - .; \ + .long __ASM_C(\type, \\type); \ + .popsection; \ + .endm + +#define __DEFINE_ANNOTATE (sec, name, type) \ + .macro name loc=910b; \ + 910: __ANNOTATE sec, type, __ASM_C(\loc, \\loc); \ + .endm + +#define DEFINE_ANNOTATE (type) \ + __DEFINE_ANNOTATE(.discard.annotate_insn, \ + ANNOTATE_ ## type, ANNOTYPE_ ## type) + +#define DEFINE_ANNOTATE_DATA (type) \ + __DEFINE_ANNOTATE(.discard.annotate_data, \ + ANNOTATE_ ## type, ANNOTYPE_ ## type)#ifdef CONFIG_OBJTOOL +DEFINE_MACRO (__ANNOTATE);
include/linux/annotate.h line 42
Quoted 10 lines
include/linux/annotate.h +10
-0+DEFINE_MACRO (ANNOTATE (NOENDBR )); +DEFINE_MACRO (ANNOTATE (RETPOLINE_SAFE )); +DEFINE_MACRO (ANNOTATE (INSTR_BEGIN )); +DEFINE_MACRO (ANNOTATE (INSTR_END )); +DEFINE_MACRO (ANNOTATE (IGNORE_ALTERNATIVE )); +DEFINE_MACRO (ANNOTATE (INTRA_FUNCTION_CALL )); +DEFINE_MACRO (ANNOTATE (UNRET_BEGIN )); +DEFINE_MACRO (ANNOTATE (REACHABLE )); +DEFINE_MACRO (ANNOTATE (NOCFI )); +DEFINE_MACRO (ANNOTATE_DATA (DATA_SPECIAL ));
Well, that took a bit this Monday morning... :-)
Clever.
Josh Poimboeuf
maintainerPATCH 6/8 x86/asm: Use unique code labels in __FILL_RETURN_BUFFER
In reply to Josh PoimboeufIn preparation for unifying the C and asm alternative implementations, change the code labels in __FILL_RETURN_BUFFER to be unique. The 771 and 772 labels are used by the C alternative implementation, which will soon be extended for asm.
--- arch/x86/include/asm/nospec-branch.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/include/asm/nospec-branch.h +4 -4
@@ -126,9 +126,9 @@*/ #define __FILL_RETURN_SLOT \ ANNOTATE_INTRA_FUNCTION_CALL; \ - call 772f; \ + call 872f; \ int3; \ -772: +872:/* * Stuff the entire RSB.
@@ -140,12 +140,12 @@#ifdef CONFIG_X86_64 #define __FILL_RETURN_BUFFER (reg, nr) \ mov $(nr/2), reg; \ -771: \ +871: \ __FILL_RETURN_SLOT \ __FILL_RETURN_SLOT \ add $(BITS_PER_LONG/8) * 2, %_ASM_SP; \ dec reg; \ - jnz 771b; \ + jnz 871b; \/* barrier for jnz misprediction */ \ lfence; \CREDIT_CALL_DEPTH \
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerPATCH 7/8 x86/asm: Remove newlines in alternatives
In reply to Josh PoimboeufIn preparation for converting the alternative implementation to assembler macros, remove all newlines from alternative instructions.
--- arch/x86/include/asm/paravirt_types.h | 2 +- arch/x86/include/asm/processor.h | 2 +- arch/x86/include/asm/smap.h | 8 ++++---- arch/x86/include/asm/vmware.h | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-)
arch/x86/include/asm/paravirt_types.h +1 -1
@@ -249,7 +249,7 @@ extern struct paravirt_patch_template pv_ops;* don't need to bother with CFI prefixes. */ #define PARAVIRT_CALL \ - ANNOTATE_RETPOLINE_SAFE "\n\t" \ + ANNOTATE_RETPOLINE_SAFE "; " \ "call *%[paravirt_opptr];"/*
arch/x86/include/asm/processor.h +1 -1
@@ -706,7 +706,7 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)*/ static __always_inline void amd_clear_divider(void) { - asm volatile(ALTERNATIVE("", "div %2\n\t", X86_BUG_DIV0) + asm volatile(ALTERNATIVE("", "div %2", X86_BUG_DIV0) :: "a" (0), "d" (0), "r" (1)); }
arch/x86/include/asm/smap.h +4 -4
@@ -77,7 +77,7 @@ static __always_inline unsigned long smap_save(void)unsigned long flags; asmvolatile ("# smap_save\n\t" - ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE "\n\t" + ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE"" ,"pushf; pop %0; clac" ,X86_FEATURE_SMAP ) : "=rm" (flags) : : "memory", "cc");
@@ -88,7 +88,7 @@ static __always_inline unsigned long smap_save(void) static __always_inline void smap_restore(unsigned long flags) { asm volatile ("# smap_restore\n\t" - ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE "\n\t" + ALTERNATIVE(ANNOTATE_IGNORE_ALTERNATIVE "", "push %0; popf", X86_FEATURE_SMAP) : : "g" (flags) : "memory", "cc");
@@ -101,9 +101,9 @@ static __always_inline void smap_restore(unsigned long flags) ALTERNATIVE("", "stac", X86_FEATURE_SMAP)#define ASM_CLAC_UNSAFE \ - ALTERNATIVE("", ANNOTATE_IGNORE_ALTERNATIVE "\n\t" "clac", X86_FEATURE_SMAP) + ALTERNATIVE("", ANNOTATE_IGNORE_ALTERNATIVE "; clac", X86_FEATURE_SMAP)#define ASM_STAC_UNSAFE \ - ALTERNATIVE("", ANNOTATE_IGNORE_ALTERNATIVE "\n\t" "stac", X86_FEATURE_SMAP) + ALTERNATIVE("", ANNOTATE_IGNORE_ALTERNATIVE "; stac", X86_FEATURE_SMAP)#endif /* __ASSEMBLER__ */
arch/x86/include/asm/vmware.h +1 -1
@@ -90,7 +90,7 @@ extern unsigned long vmware_tdx_hypercall(unsigned long cmd,* set. The high word of %edx may contain input data from the caller. */ #define VMWARE_HYPERCALL \ - ALTERNATIVE_2("movw %[port], %%dx\n\t" \ + ALTERNATIVE_2("movw %[port], %%dx; " \ "inl (%%dx), %%eax", \ "vmcall", X86_FEATURE_VMCALL, \ "vmmcall", X86_FEATURE_VMW_VMMCALL)
Signature · 1 line
2.52.0
Josh Poimboeuf
maintainerRe: [PATCH 7/8] x86/asm: Remove newlines in alternatives
Quoted 4 lines · Josh Poimboeuf
In preparation for converting the alternative implementation to assembler macros, remove all newlines from alternative instructions.
Signed-off-by Josh Poimboeuf <[email protected]>
x86-32 fixes:
arch/x86/include/asm/nospec-branch.h +24 -24
@@ -441,18 +441,18 @@ static inline void call_depth_return_thunk(void) {}* -mindirect-branch-cs-prefix. */ #define __CS_PREFIX (reg) \ - ".irp rs,r8,r9,r10,r11,r12,r13,r14,r15\n" \ - ".ifc \\rs," reg "\n" \ - ".byte 0x2e\n" \ - ".endif\n" \ - ".endr\n" + ".irp rs,r8,r9,r10,r11,r12,r13,r14,r15; " \ + ".ifc \\rs," reg "; " \ + ".byte 0x2e; " \ + ".endif; " \ + ".endr; "/* * Inline asm uses the %V modifier which is only in newer GCC * which is ensured when CONFIG_MITIGATION_RETPOLINE is defined. */ #define CALL_NOSPEC __CS_PREFIX("%V[thunk_target]") \ - "call __x86_indirect_thunk_%V[thunk_target]\n" + "call __x86_indirect_thunk_%V[thunk_target]"# define THUNK_TARGET (addr) [thunk_target] "r" (addr)
@@ -464,30 +464,30 @@ static inline void call_depth_return_thunk(void) {}*/ # define CALL_NOSPEC \ ALTERNATIVE_2( \ - ANNOTATE_RETPOLINE_SAFE "\n" \ - "call *%[thunk_target]\n", \ - " jmp 904f;\n" \ - " .align 16\n" \ - "901: call 903f;\n" \ - "902: pause;\n" \ - " lfence;\n" \ - " jmp 902b;\n" \ - " .align 16\n" \ - "903: lea 4(%%esp), %%esp;\n" \ - " pushl %[thunk_target];\n" \ - " ret;\n" \ - " .align 16\n" \ - "904: call 901b;\n", \ + ANNOTATE_RETPOLINE_SAFE "; " \ + "call *%[thunk_target]; ", \ + " jmp 904f; " \ + " .align 16; " \ + "901: call 903f; " \ + "902: pause; " \ + " lfence; " \ + " jmp 902b; " \ + " .align 16; " \ + "903: lea 4(%%esp), %%esp; " \ + " pushl %[thunk_target]; " \ + " ret; " \ + " .align 16; " \ + "904: call 901b; ", \ X86_FEATURE_RETPOLINE, \ - "lfence;\n" \ - ANNOTATE_RETPOLINE_SAFE "\n" \ - "call *%[thunk_target]\n", \ + "lfence; " \ + ANNOTATE_RETPOLINE_SAFE "; " \ + "call *%[thunk_target]; ", \ X86_FEATURE_RETPOLINE_LFENCE)# define THUNK_TARGET (addr) [thunk_target] "rm" (addr)#endif #else /* No retpoline for C / inline asm */ -# define CALL_NOSPEC "call *%[thunk_target]\n" +# define CALL_NOSPEC "call *%[thunk_target]; "# define THUNK_TARGET (addr) [thunk_target] "rm" (addr)#endif
arch/x86/kernel/irq_32.c +1 -1
@@ -53,7 +53,7 @@ DEFINE_PER_CPU_CACHE_HOT(struct irq_stack *, softirq_stack_ptr);static void call_on_stack (void *func,void *stack) { - asmvolatile ("xchgl %[sp], %%esp\n" + asmvolatile ("xchgl %[sp], %%esp; " CALL_NOSPEC "movl %[sp], %%esp" : [sp] "+b" (stack)
Josh Poimboeuf
maintainerPATCH 8/8 x86/alternative: Convert alternatives to assembler macros
In reply to Josh PoimboeufImprove code generation readability by converting the alternatives into assembler macros which are created when alternative.h is included.
Before:
# ./arch/x86/include/asm/smap.h:47: alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
# ALT: oldinstr
771:
772:
# ALT: padding
.skip -(((775f-774f)-(772b-771b)) > 0) * ((775f-774f)-(772b-771b)),0x90 773:
.pushsection .altinstructions, "aM", @progbits, 14
.long 771b - .
.long 774f - .
.4byte ( 9*32+20)
.byte 773b-771b
.byte 775f-774f
.popsection
.pushsection .altinstr_replacement, "ax" ANNOTATE_DATA_SPECIAL
# ALT: replacement
774:
stac
775:
.popsection
After:
# ./arch/x86/include/asm/smap.h:47: alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
ALTERNATIVE "", "stac", "( 9*32+20)"
--- arch/x86/include/asm/alternative.h | 196 +++++++++++++++-------------- include/linux/annotate.h | 7 +- 2 files changed, 107 insertions(+), 96 deletions(-)
arch/x86/include/asm/alternative.h +101 -95
@@ -8,6 +8,90 @@#include <asm/asm.h> #include <asm/bug.h> +/* +* CALC_MAX: assembler-compatible max based on the idea from: +* http://graphics.stanford.edu/~seander/bithacks.html#IntegerMinOrMax +* +* The additional "-" is needed because gas uses a "true" value of -1. +*/ +#define CALC_MAX (a, b) (a ^ ((a ^ b) & -(-(a < b)))) + +/* Add padding after the original instructions so the replacements can fit. */ +#define __ALT_SKIP (orig_len, len1, len2, len3) \ + .skip -((CALC_MAX(CALC_MAX(len1, len2), len3) - orig_len) > 0) *\ + (CALC_MAX(CALC_MAX(len1, len2), len3) - orig_len), 0x90 + +#define ALT_SKIP __ALT_SKIP((772b-771b), (775f-774f), (776f-775f), (777f-776f)) + +#define ALT_REPLACE (newinstr1, newinstr2, newinstr3) \ + .pushsection .altinstr_replacement, "ax"; \ + 774: newinstr1; \ + 775: newinstr2; \ + 776: newinstr3; \ + 777: \ + .popsection + +#define ALT_ENTRY (orig_begin, orig_end, new_begin, new_end, ft_flags) \ + __ANNOTATE_DATA_SPECIAL(new_begin); \ + .pushsection .altinstructions, "aM", @progbits, ALT_INSTR_SIZE; \ + .long orig_begin - .; \ + .long new_begin - .; \ + .4byte ft_flags; \ + .byte orig_end - orig_begin; \ + .byte new_end - new_begin; \ + .popsection + +/* +* These ALTERNATIVE .macros get created when this file gets included: +*/ + +#define DEFINE_ALTERNATIVE \ +.macro ALTERNATIVE oldinstr, newinstr, ft_flags; \ + 771: __ASM_C(\oldinstr, \\oldinstr); \ + 772: ALT_SKIP; \ + 773: \ + ALT_REPLACE(__ASM_C(\newinstr, \\newinstr),,); \ + ALT_ENTRY(771b, 773b, 774b, 775b, \ + __ASM_C(\ft_flags, \\ft_flags)); \ +.endm + +#define DEFINE_ALTERNATIVE_2 \ +.macro ALTERNATIVE_2 oldinstr, newinstr1, ft_flags1, \ + newinstr2, ft_flags2; \ + 771: __ASM_C(\oldinstr, \\oldinstr); \ + 772: ALT_SKIP; \ + 773: \ + ALT_REPLACE(__ASM_C(\newinstr1, \\newinstr1), \ + __ASM_C(\newinstr2, \\newinstr2),); \ + ALT_ENTRY(771b, 773b, 774b, 775b, \ + __ASM_C(\ft_flags1, \\ft_flags1)); \ + ALT_ENTRY(771b, 773b, 775b, 776b, \ + __ASM_C(\ft_flags2, \\ft_flags2)); \ +.endm + +#define DEFINE_ALTERNATIVE_3 \ +.macro ALTERNATIVE_3 oldinstr, newinstr1, ft_flags1, \ + newinstr2, ft_flags2, \ + newinstr3, ft_flags3; \ + 771: __ASM_C(\oldinstr, \\oldinstr); \ + 772: ALT_SKIP; \ + 773: \ + ALT_REPLACE(__ASM_C(\newinstr1, \\newinstr1), \ + __ASM_C(\newinstr2, \\newinstr2), \ + __ASM_C(\newinstr3, \\newinstr3)); \ + ALT_ENTRY(771b, 773b, 774b, 775b, \ + __ASM_C(\ft_flags1, \\ft_flags1)); \ + ALT_ENTRY(771b, 773b, 775b, 776b, \ + __ASM_C(\ft_flags2, \\ft_flags2)); \ + ALT_ENTRY(771b, 773b, 776b, 777b, \ + __ASM_C(\ft_flags3, \\ft_flags3)); \ +.endm + +DEFINE_MACRO (ALTERNATIVE ); +DEFINE_MACRO (ALTERNATIVE_2 ); +DEFINE_MACRO (ALTERNATIVE_3 ); + +#define ALT_FLAGS_SHIFT 16#define ALT_FLAG_NOT (1 << 0)
@@ -184,53 +268,30 @@ static inline int alternatives_text_reserved(void *start, void *end)#define ALT_CALL_INSTR "call BUG_func" -#define alt_slen "772b-771b" -#define alt_total_slen "773b-771b" -#define alt_rlen "775f-774f" +#define ALTERNATIVE (oldinstr, newinstr, ft_flags) \ + "ALTERNATIVE \"" oldinstr "\", " \ + "\"" newinstr "\", \"" __stringify(ft_flags) "\"\n" -#define OLDINSTR (oldinstr) \ - "# ALT: oldinstr\n" \ - "771:\n\t" oldinstr "\n772:\n" \ - "# ALT: padding\n" \ - ".skip -(((" alt_rlen ")-(" alt_slen ")) > 0) * " \ - "((" alt_rlen ")-(" alt_slen ")),0x90\n" \ - "773:\n" +#define ALTERNATIVE_2 (oldinstr, \ + newinstr1, ft_flags1, \ + newinstr2, ft_flags2) \ + "ALTERNATIVE_2 \"" oldinstr "\", " \ + "\"" newinstr1 "\", \"" __stringify(ft_flags1) "\", " \ + "\"" newinstr2 "\", \"" __stringify(ft_flags2) "\"\n" -#define ALTINSTR_ENTRY (ft_flags) \ - ".pushsection .altinstructions, \"aM\", @progbits, " \ - __stringify(ALT_INSTR_SIZE) "\n" \ - " .long 771b - .\n"/* label */ \ -" .long 774f - .\n" /* new instruction */ \ - " .4byte " __stringify(ft_flags) "\n" /* feature + flags */ \ -" .byte " alt_total_slen"\n" /* source len */ \ -" .byte " alt_rlen"\n" /* replacement len */ \ - ".popsection\n" - -#define ALTINSTR_REPLACEMENT (newinstr)/* replacement */ \ - ".pushsection .altinstr_replacement, \"ax\"\n" \ - ANNOTATE_DATA_SPECIAL "\n" \ - "# ALT: replacement\n" \ - "774:\n\t" newinstr "\n775:\n" \ - ".popsection\n" - -/* alternative assembly primitive: */ -#define ALTERNATIVE (oldinstr, newinstr, ft_flags) \ - OLDINSTR(oldinstr) \ - ALTINSTR_ENTRY(ft_flags) \ - ALTINSTR_REPLACEMENT(newinstr) - -#define ALTERNATIVE_2 (oldinstr, newinstr1, ft_flags1, newinstr2, ft_flags2) \ - ALTERNATIVE(ALTERNATIVE(oldinstr, newinstr1, ft_flags1), newinstr2, ft_flags2) +#define ALTERNATIVE_3 (oldinstr, \ + newinstr1, ft_flags1, \ + newinstr2, ft_flags2, \ + newinstr3, ft_flags3) \ + "ALTERNATIVE_3 \"" oldinstr "\", " \ + "\"" newinstr1 "\", \"" __stringify(ft_flags1) "\", " \ + "\"" newinstr2 "\", \"" __stringify(ft_flags2) "\", " \ + "\"" newinstr3 "\", \"" __stringify(ft_flags3) "\"\n"/* If @feature is set, patch in @newinstr_yes, otherwise @newinstr_no. */ #define ALTERNATIVE_TERNARY (oldinstr, ft_flags, newinstr_yes, newinstr_no) \ ALTERNATIVE_2(oldinstr, newinstr_no, X86_FEATURE_ALWAYS, newinstr_yes, ft_flags) -#define ALTERNATIVE_3 (oldinstr, newinstr1, ft_flags1, newinstr2, ft_flags2, \ - newinstr3, ft_flags3) \ - ALTERNATIVE(ALTERNATIVE_2(oldinstr, newinstr1, ft_flags1, newinstr2, ft_flags2), \ - newinstr3, ft_flags3) -/* * Alternative instructions for different CPU types or capabilities. *
@@ -332,65 +393,10 @@ void nop_func(void); .endm #endif -/* -* Issue one struct alt_instr descriptor entry (need to put it into -* the section .altinstructions, see below). This entry contains -* enough information for the alternatives patching code to patch an -* instruction. See apply_alternatives(). -*/ -.macro altinstr_entry orig alt ft_flags orig_len alt_len - .long \orig - . - .long \alt - . - .4byte \ft_flags - .byte \orig_len - .byte \alt_len -.endm - .macro ALT_CALL_INSTR call BUG_func .endm -/* -* Define an alternative between two instructions. If @feature is -* present, early code in apply_alternatives() replaces @oldinstr with -* @newinstr. ".skip" directive takes care of proper instruction padding -* in case @newinstr is longer than @oldinstr. - */ -#define __ALTERNATIVE (oldinst, newinst, flag) \ -740: \ - oldinst ; \ -741: \ - .skip -(((744f-743f)-(741b-740b)) > 0) * ((744f-743f)-(741b-740b)),0x90 ;\ -742: \ - .pushsection .altinstructions, "aM", @progbits, ALT_INSTR_SIZE ;\ - altinstr_entry 740b,743f,flag,742b-740b,744f-743f ; \ - .popsection ; \ - .pushsection .altinstr_replacement,"ax" ; \ -743: \ - ANNOTATE_DATA_SPECIAL ; \ - newinst ; \ -744: \ - .popsection ; - -.macro ALTERNATIVE oldinstr, newinstr, ft_flags - __ALTERNATIVE(\oldinstr, \newinstr, \ft_flags) -.endm - -/* - * Same as ALTERNATIVE macro above but for two alternatives. If CPU - * has @feature1, it replaces @oldinstr with @newinstr1. If CPU has - * @feature2, it replaces @oldinstr with @feature2. - */ -.macro ALTERNATIVE_2 oldinstr, newinstr1, ft_flags1, newinstr2, ft_flags2 - __ALTERNATIVE(__ALTERNATIVE(\oldinstr, \newinstr1, \ft_flags1), - \newinstr2, \ft_flags2) -.endm - -.macro ALTERNATIVE_3 oldinstr, newinstr1, ft_flags1, newinstr2, ft_flags2, newinstr3, ft_flags3 - __ALTERNATIVE(ALTERNATIVE_2(\oldinstr, \newinstr1, \ft_flags1, \newinstr2, \ft_flags2), - \newinstr3, \ft_flags3) -.endm -/* If @feature is set, patch in @newinstr_yes, otherwise @newinstr_no. */ #define ALTERNATIVE_TERNARY (oldinstr, ft_flags, newinstr_yes, newinstr_no) \ ALTERNATIVE_2 oldinstr, newinstr_no, X86_FEATURE_ALWAYS, \
include/linux/annotate.h +6 -1
@@ -100,8 +100,13 @@ DEFINE_MACRO(ANNOTATE_DATA(DATA_SPECIAL));* to find and extract individual special section entries as needed. */ #define ANNOTATE_DATA_SPECIAL "ANNOTATE_DATA_SPECIAL" +#define __ANNOTATE_DATA_SPECIAL (label) __ANNOTATE .discard.annotate_data, ANNOTYPE_DATA_SPECIAL, label -#endif /* !__ASSEMBLY__ */ +#else /* __ASSEMBLY__ */ + +#define __ANNOTATE_DATA_SPECIAL (label) ANNOTATE_DATA_SPECIAL loc=label + +#endif /* __ASSEMBLY__ */ #else /* !OBJTOOL */
Signature · 1 line
2.52.0
Peter Zijlstra
maintainerRe: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Quoted 29 lines · Josh Poimboeuf
Improve code generation readability by converting the alternatives into assembler macros which are created when
alternative.his included.Before:
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
# ALT: oldinstr
771:772:
# ALT: padding
.skip -(((775f-774f)-(772b-771b)) > 0) * ((775f-774f)-(772b-771b)),0x90 773:
.pushsection .altinstructions, "aM", @progbits, 14
.long 771b - .
.long 774f - .
.4byte ( 9*32+20)
.byte 773b-771b
.byte 775f-774f
.popsection
.pushsection .altinstr_replacement, "ax"ANNOTATE_DATA_SPECIAL
# ALT: replacement
774:
stac
775:
.popsection
Quoted 6 lines
After:
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1ALTERNATIVE "", "stac", "( 9*32+20)"
So the problem with the gas macro thing is that it doesn't allow for that nesting. I don't think we currently use it other than to define the ALTERNATIVE_2 and ALTERNATIVE_3 macros, but IIRC the reason I started all that was because it was fairly trivial to use things like CALL_NOSPEC in an alternative (where CALL_NOSPEC is already an alternative).
Anyway, yes, the macro variant is easier to read in the .s output.
Josh Poimboeuf
maintainerRe: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Quoted 37 lines · Peter Zijlstra
Quoted 29 lines · Josh Poimboeuf
Improve code generation readability by converting the alternatives into assembler macros which are created when
alternative.his included.Before:
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
# ALT: oldinstr
771:772:
# ALT: padding
.skip -(((775f-774f)-(772b-771b)) > 0) * ((775f-774f)-(772b-771b)),0x90 773:
.pushsection .altinstructions, "aM", @progbits, 14
.long 771b - .
.long 774f - .
.4byte ( 9*32+20)
.byte 773b-771b
.byte 775f-774f
.popsection
.pushsection .altinstr_replacement, "ax"ANNOTATE_DATA_SPECIAL
# ALT: replacement
774:
stac
775:
.popsectionQuoted 6 lines
After:
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1ALTERNATIVE "", "stac", "( 9*32+20)"
Quoted 6 lines
So the problem with the gas macro thing is that it doesn't allow for that nesting. I don't think we currently use it other than to define the
ALTERNATIVE_2andALTERNATIVE_3macros, but IIRC the reason I started all that was because it was fairly trivial to use things likeCALL_NOSPECin an alternative (whereCALL_NOSPECis already an alternative).
Trying to wrap my head around this nested alternative thing as I don't see any current code doing that. Does that only work when the inner alternative points to the same first original instruction as the outer one? Or, can you patch anywhere inside the original or replacement?
Signature · 1 line
Josh
Peter Zijlstra
maintainerRe: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Quoted 45 lines · Josh Poimboeuf
Quoted 37 lines · Peter Zijlstra
Quoted 29 lines · Josh Poimboeuf
Improve code generation readability by converting the alternatives into assembler macros which are created when
alternative.his included.Before:
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
# ALT: oldinstr
771:772:
# ALT: padding
.skip -(((775f-774f)-(772b-771b)) > 0) * ((775f-774f)-(772b-771b)),0x90 773:
.pushsection .altinstructions, "aM", @progbits, 14
.long 771b - .
.long 774f - .
.4byte ( 9*32+20)
.byte 773b-771b
.byte 775f-774f
.popsection
.pushsection .altinstr_replacement, "ax"ANNOTATE_DATA_SPECIAL
# ALT: replacement
774:
stac
775:
.popsectionQuoted 6 lines
After:
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1ALTERNATIVE "", "stac", "( 9*32+20)"Quoted 6 lines
So the problem with the gas macro thing is that it doesn't allow for that nesting. I don't think we currently use it other than to define the
ALTERNATIVE_2andALTERNATIVE_3macros, but IIRC the reason I started all that was because it was fairly trivial to use things likeCALL_NOSPECin an alternative (whereCALL_NOSPECis already an alternative).
Quoted 4 lines
Trying to wrap my head around this nested alternative thing as I don't see any current code doing that. Does that only work when the inner alternative points to the same first original instruction as the outer one? Or, can you patch anywhere inside the original or replacement?
They'd have to be at the exact same location.
Josh Poimboeuf
maintainerRe: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Quoted 15 lines · Peter Zijlstra
Quoted 14 lines · Josh Poimboeuf
Quoted 12 lines · Peter Zijlstra
Quoted 4 lines · Josh Poimboeuf
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1ALTERNATIVE "", "stac", "( 9*32+20)"So the problem with the gas macro thing is that it doesn't allow for that nesting. I don't think we currently use it other than to define the
ALTERNATIVE_2andALTERNATIVE_3macros, but IIRC the reason I started all that was because it was fairly trivial to use things likeCALL_NOSPECin an alternative (whereCALL_NOSPECis already an alternative).
Quoted 6 lines
Quoted 4 lines
Trying to wrap my head around this nested alternative thing as I don't see any current code doing that. Does that only work when the inner alternative points to the same first original instruction as the outer one? Or, can you patch anywhere inside the original or replacement?
They'd have to be at the exact same location.
Ok, so while the syntax itself is nested, the underlying behavior is just stacking alternatives together, like ALTERNATIVE_2 and _3 already do, correct?
While it's clever that the current implementation allows that kind of nested syntax, it seems dangerous. I don't see anything preventing the inner ALTERNATIVE from being placed in the middle of the outer ALTERNATIVE's original instructions, or anywhere in the outer's replacement code.
It would be really easy to introduce CALL_NOSPEC in the middle of a group of instructions in an ALTERNATIVE without realizing that you're likely introducing some subtle or not-so-subtle bug on x86-32, which just happens to hide an ALTERNATIVE_2 inside the CALL_NOSPEC...
The gas macro doesn't give you the leeway to make that mistake, so you'd have to restructure the code slightly to make it fit into a proper ALTERNATIVE_3. Which is less magical and more clear, so that seems like a good thing.
Signature · 1 line
Josh
Peter Zijlstra
maintainerRe: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Quoted 3 lines · Josh Poimboeuf
Ok, so while the syntax itself is nested, the underlying behavior is just stacking alternatives together, like
ALTERNATIVE_2and _3 already do, correct?
Yup.
Quoted 10 lines
While it's clever that the current implementation allows that kind of nested syntax, it seems dangerous. I don't see anything preventing the inner ALTERNATIVE from being placed in the middle of the outer ALTERNATIVE's original instructions, or anywhere in the outer's replacement code.
It would be really easy to introduce
CALL_NOSPECin the middle of a group of instructions in an ALTERNATIVE without realizing that you're likely introducing some subtle or not-so-subtle bug on x86-32, which just happens to hide anALTERNATIVE_2inside theCALL_NOSPEC...
I think I made objtool complain in that case, but I'm not sure.
Quoted 4 lines
The gas macro doesn't give you the leeway to make that mistake, so you'd have to restructure the code slightly to make it fit into a proper
ALTERNATIVE_3. Which is less magical and more clear, so that seems like a good thing.
Perhaps, I'm not really a fan of the ALTERNATIVE_n() macros much. I think writing the nested ALTERNATIVE() form is actually more readable. But perhaps I'm the crazy one -- wouldn't be the first time :-)
Anyway, seeing how its not actually used, and I've since solved the case that gave rise to all this completely differently, perhaps I should just shut up and let you do the conversion.
I mean, we will have to do ALTERNATIVE_4() at some point, and it will be glorious... *sigh*
Josh Poimboeuf
maintainerRe: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Quoted 14 lines · Peter Zijlstra
Quoted 3 lines · Josh Poimboeuf
Ok, so while the syntax itself is nested, the underlying behavior is just stacking alternatives together, like
ALTERNATIVE_2and _3 already do, correct?Yup.
Quoted 6 lines
While it's clever that the current implementation allows that kind of nested syntax, it seems dangerous. I don't see anything preventing the inner ALTERNATIVE from being placed in the middle of the outer ALTERNATIVE's original instructions, or anywhere in the outer's replacement code.
Quoted 6 lines
Quoted 4 lines
It would be really easy to introduce
CALL_NOSPECin the middle of a group of instructions in an ALTERNATIVE without realizing that you're likely introducing some subtle or not-so-subtle bug on x86-32, which just happens to hide anALTERNATIVE_2inside theCALL_NOSPEC...I think I made objtool complain in that case, but I'm not sure.
I do see some checks there. I'm not quite convinced all the edge cases are covered.
Quoted 8 lines
Quoted 4 lines
The gas macro doesn't give you the leeway to make that mistake, so you'd have to restructure the code slightly to make it fit into a proper
ALTERNATIVE_3. Which is less magical and more clear, so that seems like a good thing.Perhaps, I'm not really a fan of the
ALTERNATIVE_n()macros much. I think writing the nestedALTERNATIVE()form is actually more readable. But perhaps I'm the crazy one -- wouldn't be the first time :-)
The nesting might be more readable, but it feels like syntax sugar in a way that's a conceptual mismatch compared to how the alternatives are actually applied.
Quoted 6 lines
Anyway, seeing how its not actually used, and I've since solved the case that gave rise to all this completely differently, perhaps I should just shut up and let you do the conversion.
I mean, we will have to do
ALTERNATIVE_4()at some point, and it will be glorious... *sigh*
Well, at least this makes it a unified implementation so the ugly is only confined to a single place :-)
Signature · 1 line
Josh
Borislav Petkov
maintainerQuoted 3 lines · Josh Poimboeuf
Convert objtool annotations and x86 alternatives to assembler macros. This creates a shared C/asm alternative implementation and makes the code generation significantly more readable.
I don't understand: all this churn for that?
So that .s output - which is looked at by 3 people on this planet - is "readable"?
Dunno, it was readable before too...
Quoted 12 lines
Patches 1-5: convert objtool
Patches 6-8: convert alternativesBased on tip/master.
Before:
-----
#./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1
# ALT: oldinstr
771:
Quoted 2 lines
772:
# ALT: padding
... especially with those helpful comments...
Quoted 18 lines
.skip -(((775f-774f)-(772b-771b)) > 0) * ((775f-774f)-(772b-771b)),0x90 773:
.pushsection .altinstructions, "aM", @progbits, 14
.long 771b - .
.long 774f - .
.4byte ( 9*32+20)
.byte 773b-771b
.byte 775f-774f
.popsection
.pushsection .altinstr_replacement, "ax"
.pushsection .discard.annotate_data, "M", @progbits, 8; .long 1b - ., 1; .popsection # ALT: replacement
774:
stac
775:
.popsection
-----
Quoted 5 lines
After:
-----
#./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1ALTERNATIVE "", "stac", "( 9*32+20)"
Well, that's "readable" but one still needs to go see that ALTERNATIVE macro definition now, right?
So you move the gory details somewhere else.
Sorry, I fail to see why we're doing this...
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteJosh Poimboeuf
maintainerQuoted 11 lines · Borislav Petkov
Quoted 3 lines · Josh Poimboeuf
#
./arch/x86/include/asm/smap.h:47:alternative("", "stac", X86_FEATURE_SMAP);
# 47 "./arch/x86/include/asm/smap.h" 1ALTERNATIVE "", "stac", "( 9*32+20)"Well, that's "readable" but one still needs to go see that ALTERNATIVE macro definition now, right?
So you move the gory details somewhere else.
Sorry, I fail to see why we're doing this...
Partially because Linus complains every time I mess up the code gen ;-) But I like this change too.
Anybody reading the .s file should already know how alternatives work. If not... go read alternatives.h!
Signature · 1 line
Josh