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

Josh Poimboeuf posted 8 patches 1 week, 5 days ago
[PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Josh Poimboeuf 1 week, 5 days ago
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)"

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/include/asm/alternative.h | 196 +++++++++++++++--------------
 include/linux/annotate.h           |   7 +-
 2 files changed, 107 insertions(+), 96 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 03364510d5fe..89c69fe8f337 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -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,	\
diff --git a/include/linux/annotate.h b/include/linux/annotate.h
index 8b28f1a81ec4..a80fef36dc92 100644
--- a/include/linux/annotate.h
+++ b/include/linux/annotate.h
@@ -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 */
 
-- 
2.52.0
Re: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Peter Zijlstra 1 week, 4 days ago
On Sat, Dec 06, 2025 at 01:41:15PM -0800, Josh Poimboeuf wrote:
> 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)"
> 

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.
Re: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Josh Poimboeuf 1 week, 3 days ago
On Mon, Dec 08, 2025 at 10:51:09AM +0100, Peter Zijlstra wrote:
> On Sat, Dec 06, 2025 at 01:41:15PM -0800, Josh Poimboeuf wrote:
> > 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)"
> > 
> 
> 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?

-- 
Josh
Re: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Peter Zijlstra 1 week, 3 days ago
On Mon, Dec 08, 2025 at 02:46:23PM -0800, Josh Poimboeuf wrote:
> On Mon, Dec 08, 2025 at 10:51:09AM +0100, Peter Zijlstra wrote:
> > On Sat, Dec 06, 2025 at 01:41:15PM -0800, Josh Poimboeuf wrote:
> > > 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)"
> > > 
> > 
> > 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?

They'd have to be at the exact same location.
Re: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Josh Poimboeuf 1 week, 2 days ago
On Tue, Dec 09, 2025 at 10:24:19AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 08, 2025 at 02:46:23PM -0800, Josh Poimboeuf wrote:
> > On Mon, Dec 08, 2025 at 10:51:09AM +0100, Peter Zijlstra wrote:
> > > On Sat, Dec 06, 2025 at 01:41:15PM -0800, Josh Poimboeuf wrote:
> > > >   # ./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).
> > 
> > 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.

-- 
Josh
Re: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Peter Zijlstra 1 week, 2 days ago
On Tue, Dec 09, 2025 at 05:15:06PM -0800, Josh Poimboeuf wrote:

> 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.

> 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.

> 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*
Re: [PATCH 8/8] x86/alternative: Convert alternatives to assembler macros
Posted by Josh Poimboeuf 1 week, 1 day ago
On Wed, Dec 10, 2025 at 10:16:45AM +0100, Peter Zijlstra wrote:
> On Tue, Dec 09, 2025 at 05:15:06PM -0800, Josh Poimboeuf wrote:
> 
> > 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.
> 
> > 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.

I do see some checks there.  I'm not quite convinced all the edge cases
are covered.

> > 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.

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

-- 
Josh