Skip to contentSkip to conversation

objtool, x86/alternative: Convert objtool annotations and x86 alternatives to assembler macros

PATCH In review Josh Poimboeuf 3 participants 20 messages Dec 6, 2025 – Dec 11, 2025

Subsystems (7)
lore
Share & tools
lore

Josh Poimboeuf

author maintainer
#0 lore raw

cover 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
author maintainer
#1 lore raw

PATCH 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
index 2f1599c9e573..929f5c658a1d 100644
@@ -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
index c6def4049b1a..e740002018c0 100644
@@ -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
index c6def4049b1a..e740002018c0 100644
@@ -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
index 3f7999317f4d..53af6887852a 100644
@@ -2358,7 +2358,7 @@ static int __annotate_early(struct objtool_file *file, int type, struct instruct
 	switch (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
author maintainer
#2 lore raw

PATCH 2/8 objtool: Make ANNOTYPE_DATA_SPECIAL unique across all annotations

In reply to Josh Poimboeuf

While 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
index e740002018c0..0b55dc386b69 100644
@@ -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
index e740002018c0..0b55dc386b69 100644
@@ -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
author maintainer
#3 lore raw

PATCH 3/8 objtool: Rename C ANNOTATE_REACHABLE to ANNOTATE_REACHABLE_LABEL

In reply to Josh Poimboeuf

Unlike 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
index ee23b98353d7..df170c2f1e24 100644
@@ -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
index 8325b79f2ac6..af4b10de3f9c 100644
@@ -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
index 929f5c658a1d..0ece31a9b585 100644
@@ -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
author maintainer
#4 lore raw

PATCH 4/8 objtool: Rename asm ANNOTATE_NOCFI_SYM to ANNOTATE_NOCFI

In reply to Josh Poimboeuf

Unlike 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
index f0a5fba0717e..5aac37d379ab 100644
@@ -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
index 0ece31a9b585..1678410efa1b 100644
@@ -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
author maintainer
#5 lore raw

PATCH 5/8 objtool: Convert annotations to assembler macros

In reply to Josh Poimboeuf

Improve 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
index 0e8c611bc9e2..770321c908e5 100644
@@ -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
index 05b16299588d..d34a16a4d305 100644
@@ -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
author maintainer
#6 lore raw

Re: [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.h is included.

Before:

Quoted 7 lines

.pushsection .discard.annotate_insn, "M", @progbits, 8; .long 1b - ., 8; .popsection

After:

ANNOTATE_REACHABLE loc=1b

This needs the following to build on non-x86:

arch/x86/include/asm/asm.h +0 -6
index 770321c908e5..e63810bfdc68 100644
@@ -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
index a80fef36dc92..a099a1d5e410 100644
@@ -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
new file mode 100644
index 000000000000..94836aa217b4
@@ -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 */
maintainer
#7 lore raw

Re: [PATCH 5/8] objtool: Convert annotations to assembler macros

In reply to Josh Poimboeuf
Quoted 83 lines · Josh Poimboeuf

Improve 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 +6 -2
index 0e8c611bc9e2..770321c908e5 100644
@@ -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
index 1678410efa1b..8b28f1a81ec4 100644
@@ -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.

author maintainer
#8 lore raw

PATCH 6/8 x86/asm: Use unique code labels in __FILL_RETURN_BUFFER

In reply to Josh Poimboeuf

In 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
index a5d41d8cd70a..334c6058a1ec 100644
@@ -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
author maintainer
#9 lore raw

PATCH 7/8 x86/asm: Remove newlines in alternatives

In reply to Josh Poimboeuf

In 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
index 3502939415ad..a05fcc4f5172 100644
@@ -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
index a24c7805acdb..816dbddd7507 100644
@@ -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
index 977bef14a0ab..d9d565024e3b 100644
@@ -77,7 +77,7 @@ static __always_inline unsigned long smap_save(void)
 	unsigned long flags;
 
 	asm volatile ("# 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
index c9cf43d5ef23..100fba7555bf 100644
@@ -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
author maintainer
#10 lore raw

Re: [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.

x86-32 fixes:

arch/x86/include/asm/nospec-branch.h +24 -24
index 334c6058a1ec..2313f3fe19d0 100644
@@ -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
index c7a5d2960d57..9c5bb666bd55 100644
@@ -53,7 +53,7 @@ DEFINE_PER_CPU_CACHE_HOT(struct irq_stack *, softirq_stack_ptr);
 
 static void call_on_stack(void *func, void *stack)
 {
-	asm volatile("xchgl %[sp], %%esp\n"
+	asm volatile("xchgl %[sp], %%esp; "
 		     CALL_NOSPEC
 		     "movl %[sp], %%esp"
 		     : [sp] "+b" (stack)
author maintainer
#11 lore raw

PATCH 8/8 x86/alternative: Convert alternatives to assembler macros

In reply to Josh Poimboeuf

Improve 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
index 03364510d5fe..89c69fe8f337 100644
@@ -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
index 8b28f1a81ec4..a80fef36dc92 100644
@@ -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
maintainer
#12 lore raw

Re: [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.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

Quoted 6 lines

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)"

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.

author maintainer
#13 lore raw

Re: [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.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

Quoted 6 lines

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)"
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_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).

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
maintainer
#14 lore raw

Re: [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.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

Quoted 6 lines

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)"
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_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).

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.

author maintainer
#15 lore raw

Re: [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" 1

	ALTERNATIVE "", "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).

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
maintainer
#16 lore raw

Re: [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_2 and _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_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...

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*

author maintainer
#17 lore raw

Re: [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_2 and _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_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...

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 nested ALTERNATIVE() 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
maintainer
#18 lore raw
In reply to Josh Poimboeuf
Quoted 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 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:

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" 1

	ALTERNATIVE "", "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-netiquette
author maintainer
#19 lore raw
Quoted 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" 1

	ALTERNATIVE "", "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