[PATCH 0/3] x86/alternative: Make alternative patching more robust

[PATCH 0/3] x86/alternative: Make alternative patching more robust

[Juergen Gross]

This small series makes alternative patching a little bit more robust
for ALTERNATIVE_<n>() use cases by using a local buffer for merging
multiple patch actions of the same location.

With this series applied intermediate patch results due to multiple
features matching in a ALTERNATIVE_<n>() invocation won't be visible
to e.g. interrupt handlers being invoked between the single patching
operations.

Additionally any indirect call replacements with their direct call
equivalents no longer need to be placed carefully to be the first
executed patch action of an ALTERNATIVE_<n>() invocation, as the
needed reference of the indirect call of the original instruction
will still see the original code.

These patches have been sent before as part of my MSR paravirt rework
series. I'm sending them now as an independent series, as V2 of the
MSR series is no longer relying on them.

Juergen Gross (3):
  x86/alternative: Drop not needed test after call of alt_replace_call()
  x86/alternative: Refactor apply_alternatives()
  x86/alternative: Patch a single alternative location only once

 arch/x86/kernel/alternative.c | 86 ++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 37 deletions(-)

-- 
2.51.0

[PATCH 1/3] x86/alternative: Drop not needed test after call of alt_replace_call()

[Juergen Gross]

alt_replace_call() will never return a negative value, so testing the
return value to be less than zero can be dropped.

This makes it possible to switch the return type of alt_replace_call()
and the type of insn_buff_sz to unsigned int.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/alternative.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 7bde68247b5f..05d3dbe7104e 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -559,7 +559,7 @@ EXPORT_SYMBOL(BUG_func);
  * Rewrite the "call BUG_func" replacement to point to the target of the
  * indirect pv_ops call "call *disp(%ip)".
  */
-static int alt_replace_call(u8 *instr, u8 *insn_buff, struct alt_instr *a)
+static unsigned int alt_replace_call(u8 *instr, u8 *insn_buff, struct alt_instr *a)
 {
 	void *target, *bug = &BUG_func;
 	s32 disp;
@@ -643,7 +643,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	 * order.
 	 */
 	for (a = start; a < end; a++) {
-		int insn_buff_sz = 0;
+		unsigned int insn_buff_sz = 0;
 
 		/*
 		 * In case of nested ALTERNATIVE()s the outer alternative might
@@ -683,11 +683,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		memcpy(insn_buff, replacement, a->replacementlen);
 		insn_buff_sz = a->replacementlen;
 
-		if (a->flags & ALT_FLAG_DIRECT_CALL) {
+		if (a->flags & ALT_FLAG_DIRECT_CALL)
 			insn_buff_sz = alt_replace_call(instr, insn_buff, a);
-			if (insn_buff_sz < 0)
-				continue;
-		}
 
 		for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
 			insn_buff[insn_buff_sz] = 0x90;
-- 
2.51.0

[tip: x86/core] x86/alternative: Drop not needed test after call of alt_replace_call()

[tip-bot2 for Juergen Gross]

The following commit has been merged into the x86/core branch of tip:

Commit-ID:     ad74016b919cbad78d203fa1c459ae18e73ce586
Gitweb:        https://git.kernel.org/tip/ad74016b919cbad78d203fa1c459ae18e73ce586
Author:        Juergen Gross <jgross@suse.com>
AuthorDate:    Mon, 29 Sep 2025 13:29:45 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 14 Oct 2025 10:38:11 +02:00

x86/alternative: Drop not needed test after call of alt_replace_call()

alt_replace_call() will never return a negative value, so testing the
return value to be less than zero can be dropped.

This makes it possible to switch the return type of alt_replace_call()
and the type of insn_buff_sz to unsigned int.

Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/alternative.c |  9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 8ee5ff5..4f3ea50 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -559,7 +559,7 @@ EXPORT_SYMBOL(BUG_func);
  * Rewrite the "call BUG_func" replacement to point to the target of the
  * indirect pv_ops call "call *disp(%ip)".
  */
-static int alt_replace_call(u8 *instr, u8 *insn_buff, struct alt_instr *a)
+static unsigned int alt_replace_call(u8 *instr, u8 *insn_buff, struct alt_instr *a)
 {
 	void *target, *bug = &BUG_func;
 	s32 disp;
@@ -643,7 +643,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	 * order.
 	 */
 	for (a = start; a < end; a++) {
-		int insn_buff_sz = 0;
+		unsigned int insn_buff_sz = 0;
 
 		/*
 		 * In case of nested ALTERNATIVE()s the outer alternative might
@@ -683,11 +683,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		memcpy(insn_buff, replacement, a->replacementlen);
 		insn_buff_sz = a->replacementlen;
 
-		if (a->flags & ALT_FLAG_DIRECT_CALL) {
+		if (a->flags & ALT_FLAG_DIRECT_CALL)
 			insn_buff_sz = alt_replace_call(instr, insn_buff, a);
-			if (insn_buff_sz < 0)
-				continue;
-		}
 
 		for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
 			insn_buff[insn_buff_sz] = 0x90;

[PATCH 2/3] x86/alternative: Refactor apply_alternatives()

[Juergen Gross]

Refactor apply_alternatives() by splitting out patching of a single
alt_instr instance into a sub-function.

Keep the final text_poke_early() call in apply_alternatives() in
order to prepare merging multiple alternative patching instances of
the same location.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/alternative.c | 60 +++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 05d3dbe7104e..735cc017f2d3 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -604,6 +604,34 @@ static inline u8 * instr_va(struct alt_instr *i)
 	return (u8 *)&i->instr_offset + i->instr_offset;
 }
 
+static void __init_or_module apply_one_alternative(u8 *instr, u8 *insn_buff,
+						   struct alt_instr *a)
+{
+	u8 *replacement = (u8 *)&a->repl_offset + a->repl_offset;
+	unsigned int insn_buff_sz;
+
+	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
+		a->cpuid >> 5,
+		a->cpuid & 0x1f,
+		instr, instr, a->instrlen,
+		replacement, a->replacementlen, a->flags);
+
+	memcpy(insn_buff, replacement, a->replacementlen);
+	insn_buff_sz = a->replacementlen;
+
+	if (a->flags & ALT_FLAG_DIRECT_CALL)
+		insn_buff_sz = alt_replace_call(instr, insn_buff, a);
+
+	for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
+		insn_buff[insn_buff_sz] = 0x90;
+
+	text_poke_apply_relocation(insn_buff, instr, a->instrlen, replacement, a->replacementlen);
+
+	DUMP_BYTES(ALT, instr, a->instrlen, "%px:   old_insn: ", instr);
+	DUMP_BYTES(ALT, replacement, a->replacementlen, "%px:   rpl_insn: ", replacement);
+	DUMP_BYTES(ALT, insn_buff, insn_buff_sz, "%px: final_insn: ", instr);
+}
+
 /*
  * Replace instructions with better alternatives for this CPU type. This runs
  * before SMP is initialized to avoid SMP problems with self modifying code.
@@ -618,7 +646,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 						  struct alt_instr *end)
 {
 	u8 insn_buff[MAX_PATCH_LEN];
-	u8 *instr, *replacement;
+	u8 *instr;
 	struct alt_instr *a, *b;
 
 	DPRINTK(ALT, "alt table %px, -> %px", start, end);
@@ -643,8 +671,6 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	 * order.
 	 */
 	for (a = start; a < end; a++) {
-		unsigned int insn_buff_sz = 0;
-
 		/*
 		 * In case of nested ALTERNATIVE()s the outer alternative might
 		 * add more padding. To ensure consistent patching find the max
@@ -657,7 +683,6 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		}
 
 		instr = instr_va(a);
-		replacement = (u8 *)&a->repl_offset + a->repl_offset;
 		BUG_ON(a->instrlen > sizeof(insn_buff));
 		BUG_ON(a->cpuid >= (NCAPINTS + NBUGINTS) * 32);
 
@@ -670,32 +695,11 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
 			memcpy(insn_buff, instr, a->instrlen);
 			optimize_nops(instr, insn_buff, a->instrlen);
-			text_poke_early(instr, insn_buff, a->instrlen);
-			continue;
+		} else {
+			apply_one_alternative(instr, insn_buff, a);
 		}
 
-		DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
-			a->cpuid >> 5,
-			a->cpuid & 0x1f,
-			instr, instr, a->instrlen,
-			replacement, a->replacementlen, a->flags);
-
-		memcpy(insn_buff, replacement, a->replacementlen);
-		insn_buff_sz = a->replacementlen;
-
-		if (a->flags & ALT_FLAG_DIRECT_CALL)
-			insn_buff_sz = alt_replace_call(instr, insn_buff, a);
-
-		for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
-			insn_buff[insn_buff_sz] = 0x90;
-
-		text_poke_apply_relocation(insn_buff, instr, a->instrlen, replacement, a->replacementlen);
-
-		DUMP_BYTES(ALT, instr, a->instrlen, "%px:   old_insn: ", instr);
-		DUMP_BYTES(ALT, replacement, a->replacementlen, "%px:   rpl_insn: ", replacement);
-		DUMP_BYTES(ALT, insn_buff, insn_buff_sz, "%px: final_insn: ", instr);
-
-		text_poke_early(instr, insn_buff, insn_buff_sz);
+		text_poke_early(instr, insn_buff, a->instrlen);
 	}
 
 	kasan_enable_current();
-- 
2.51.0

[tip: x86/core] x86/alternative: Refactor apply_alternatives()

[tip-bot2 for Juergen Gross]

The following commit has been merged into the x86/core branch of tip:

Commit-ID:     9f6a53b4ecc6800b0db5463071ce95a0afab6746
Gitweb:        https://git.kernel.org/tip/9f6a53b4ecc6800b0db5463071ce95a0afab6746
Author:        Juergen Gross <jgross@suse.com>
AuthorDate:    Mon, 29 Sep 2025 13:29:46 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 14 Oct 2025 10:38:11 +02:00

x86/alternative: Refactor apply_alternatives()

Refactor apply_alternatives() by splitting out patching of a single
alt_instr instance into a sub-function.

Keep the final text_poke_early() call in apply_alternatives() in
order to prepare merging multiple alternative patching instances of
the same location.

Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/alternative.c | 60 ++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 4f3ea50..d86a012 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -604,6 +604,34 @@ static inline u8 * instr_va(struct alt_instr *i)
 	return (u8 *)&i->instr_offset + i->instr_offset;
 }
 
+static void __init_or_module apply_one_alternative(u8 *instr, u8 *insn_buff,
+						   struct alt_instr *a)
+{
+	u8 *replacement = (u8 *)&a->repl_offset + a->repl_offset;
+	unsigned int insn_buff_sz;
+
+	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
+		a->cpuid >> 5,
+		a->cpuid & 0x1f,
+		instr, instr, a->instrlen,
+		replacement, a->replacementlen, a->flags);
+
+	memcpy(insn_buff, replacement, a->replacementlen);
+	insn_buff_sz = a->replacementlen;
+
+	if (a->flags & ALT_FLAG_DIRECT_CALL)
+		insn_buff_sz = alt_replace_call(instr, insn_buff, a);
+
+	for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
+		insn_buff[insn_buff_sz] = 0x90;
+
+	text_poke_apply_relocation(insn_buff, instr, a->instrlen, replacement, a->replacementlen);
+
+	DUMP_BYTES(ALT, instr, a->instrlen, "%px:   old_insn: ", instr);
+	DUMP_BYTES(ALT, replacement, a->replacementlen, "%px:   rpl_insn: ", replacement);
+	DUMP_BYTES(ALT, insn_buff, insn_buff_sz, "%px: final_insn: ", instr);
+}
+
 /*
  * Replace instructions with better alternatives for this CPU type. This runs
  * before SMP is initialized to avoid SMP problems with self modifying code.
@@ -618,7 +646,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 						  struct alt_instr *end)
 {
 	u8 insn_buff[MAX_PATCH_LEN];
-	u8 *instr, *replacement;
+	u8 *instr;
 	struct alt_instr *a, *b;
 
 	DPRINTK(ALT, "alt table %px, -> %px", start, end);
@@ -643,8 +671,6 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	 * order.
 	 */
 	for (a = start; a < end; a++) {
-		unsigned int insn_buff_sz = 0;
-
 		/*
 		 * In case of nested ALTERNATIVE()s the outer alternative might
 		 * add more padding. To ensure consistent patching find the max
@@ -657,7 +683,6 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		}
 
 		instr = instr_va(a);
-		replacement = (u8 *)&a->repl_offset + a->repl_offset;
 		BUG_ON(a->instrlen > sizeof(insn_buff));
 		BUG_ON(a->cpuid >= (NCAPINTS + NBUGINTS) * 32);
 
@@ -670,32 +695,11 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
 			memcpy(insn_buff, instr, a->instrlen);
 			optimize_nops(instr, insn_buff, a->instrlen);
-			text_poke_early(instr, insn_buff, a->instrlen);
-			continue;
+		} else {
+			apply_one_alternative(instr, insn_buff, a);
 		}
 
-		DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
-			a->cpuid >> 5,
-			a->cpuid & 0x1f,
-			instr, instr, a->instrlen,
-			replacement, a->replacementlen, a->flags);
-
-		memcpy(insn_buff, replacement, a->replacementlen);
-		insn_buff_sz = a->replacementlen;
-
-		if (a->flags & ALT_FLAG_DIRECT_CALL)
-			insn_buff_sz = alt_replace_call(instr, insn_buff, a);
-
-		for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
-			insn_buff[insn_buff_sz] = 0x90;
-
-		text_poke_apply_relocation(insn_buff, instr, a->instrlen, replacement, a->replacementlen);
-
-		DUMP_BYTES(ALT, instr, a->instrlen, "%px:   old_insn: ", instr);
-		DUMP_BYTES(ALT, replacement, a->replacementlen, "%px:   rpl_insn: ", replacement);
-		DUMP_BYTES(ALT, insn_buff, insn_buff_sz, "%px: final_insn: ", instr);
-
-		text_poke_early(instr, insn_buff, insn_buff_sz);
+		text_poke_early(instr, insn_buff, a->instrlen);
 	}
 
 	kasan_enable_current();

[PATCH 3/3] x86/alternative: Patch a single alternative location only once

[Juergen Gross]

Instead of patching a single location potentially multiple times in
case of nested ALTERNATIVE()s, do the patching only after having
evaluated all alt_instr instances for that location.

This has multiple advantages:

- In case of replacing an indirect with a direct call using the
  ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
  instance before any other instances at the same location (the
  original instruction is needed for finding the target of the direct
  call).

- In case of nested ALTERNATIVE()s there is no intermediate replacement
  visible. This avoids any problems in case e.g. an interrupt is
  happening between the single instances and the patched location is
  used during handling the interrupt.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/alternative.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 735cc017f2d3..ccf07131cd47 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	u8 insn_buff[MAX_PATCH_LEN];
 	u8 *instr;
 	struct alt_instr *a, *b;
+	unsigned int instances = 0;
+	bool patched = false;
 
 	DPRINTK(ALT, "alt table %px, -> %px", start, end);
 
@@ -677,9 +679,13 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		 * padding for all alt_instr entries for this site (nested
 		 * alternatives result in consecutive entries).
 		 */
-		for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
-			u8 len = max(a->instrlen, b->instrlen);
-			a->instrlen = b->instrlen = len;
+		if (!instances) {
+			for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
+				u8 len = max(a->instrlen, b->instrlen);
+				a->instrlen = b->instrlen = len;
+			}
+			instances = b - a;
+			patched = false;
 		}
 
 		instr = instr_va(a);
@@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		 * - feature not present but ALT_FLAG_NOT is set to mean,
 		 *   patch if feature is *NOT* present.
 		 */
-		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
-			memcpy(insn_buff, instr, a->instrlen);
-			optimize_nops(instr, insn_buff, a->instrlen);
-		} else {
+		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
 			apply_one_alternative(instr, insn_buff, a);
+			patched = true;
 		}
 
-		text_poke_early(instr, insn_buff, a->instrlen);
+		instances--;
+		if (!instances) {
+			if (!patched) {
+				memcpy(insn_buff, instr, a->instrlen);
+				optimize_nops(instr, insn_buff, a->instrlen);
+			}
+			text_poke_early(instr, insn_buff, a->instrlen);
+		}
 	}
 
 	kasan_enable_current();
-- 
2.51.0

Re: [PATCH 3/3] x86/alternative: Patch a single alternative location only once

[Peter Zijlstra]

On Mon, Sep 29, 2025 at 01:29:47PM +0200, Juergen Gross wrote:
> Instead of patching a single location potentially multiple times in
> case of nested ALTERNATIVE()s, do the patching only after having
> evaluated all alt_instr instances for that location.
> 
> This has multiple advantages:
> 
> - In case of replacing an indirect with a direct call using the
>   ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>   instance before any other instances at the same location (the
>   original instruction is needed for finding the target of the direct
>   call).
> 
> - In case of nested ALTERNATIVE()s there is no intermediate replacement
>   visible. This avoids any problems in case e.g. an interrupt is
>   happening between the single instances and the patched location is
>   used during handling the interrupt.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - new patch
> ---
>  arch/x86/kernel/alternative.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 735cc017f2d3..ccf07131cd47 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  	u8 insn_buff[MAX_PATCH_LEN];
>  	u8 *instr;
>  	struct alt_instr *a, *b;
> +	unsigned int instances = 0;
> +	bool patched = false;
>  
>  	DPRINTK(ALT, "alt table %px, -> %px", start, end);
>  
> @@ -677,9 +679,13 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  		 * padding for all alt_instr entries for this site (nested
>  		 * alternatives result in consecutive entries).
>  		 */
> -		for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
> -			u8 len = max(a->instrlen, b->instrlen);
> -			a->instrlen = b->instrlen = len;
> +		if (!instances) {
> +			for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
> +				u8 len = max(a->instrlen, b->instrlen);
> +				a->instrlen = b->instrlen = len;
> +			}
> +			instances = b - a;
> +			patched = false;
>  		}
>  
>  		instr = instr_va(a);
> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  		 * - feature not present but ALT_FLAG_NOT is set to mean,
>  		 *   patch if feature is *NOT* present.
>  		 */
> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
> -			memcpy(insn_buff, instr, a->instrlen);
> -			optimize_nops(instr, insn_buff, a->instrlen);
> -		} else {
> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>  			apply_one_alternative(instr, insn_buff, a);
> +			patched = true;
>  		}
>  
> -		text_poke_early(instr, insn_buff, a->instrlen);
> +		instances--;
> +		if (!instances) {
> +			if (!patched) {
> +				memcpy(insn_buff, instr, a->instrlen);
> +				optimize_nops(instr, insn_buff, a->instrlen);
> +			}
> +			text_poke_early(instr, insn_buff, a->instrlen);
> +		}
>  	}
>  
>  	kasan_enable_current();

I think you lost the optimize_nops() call for the patched case.

That is, note how apply_one_alternative() does 0x90 padding, but then
you only do optimize_nops() when !patched.

Re: [PATCH 3/3] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3585 bytes --]

On 30.09.25 09:26, Peter Zijlstra wrote:
> On Mon, Sep 29, 2025 at 01:29:47PM +0200, Juergen Gross wrote:
>> Instead of patching a single location potentially multiple times in
>> case of nested ALTERNATIVE()s, do the patching only after having
>> evaluated all alt_instr instances for that location.
>>
>> This has multiple advantages:
>>
>> - In case of replacing an indirect with a direct call using the
>>    ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>>    instance before any other instances at the same location (the
>>    original instruction is needed for finding the target of the direct
>>    call).
>>
>> - In case of nested ALTERNATIVE()s there is no intermediate replacement
>>    visible. This avoids any problems in case e.g. an interrupt is
>>    happening between the single instances and the patched location is
>>    used during handling the interrupt.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - new patch
>> ---
>>   arch/x86/kernel/alternative.c | 27 +++++++++++++++++++--------
>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>> index 735cc017f2d3..ccf07131cd47 100644
>> --- a/arch/x86/kernel/alternative.c
>> +++ b/arch/x86/kernel/alternative.c
>> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   	u8 insn_buff[MAX_PATCH_LEN];
>>   	u8 *instr;
>>   	struct alt_instr *a, *b;
>> +	unsigned int instances = 0;
>> +	bool patched = false;
>>   
>>   	DPRINTK(ALT, "alt table %px, -> %px", start, end);
>>   
>> @@ -677,9 +679,13 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   		 * padding for all alt_instr entries for this site (nested
>>   		 * alternatives result in consecutive entries).
>>   		 */
>> -		for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
>> -			u8 len = max(a->instrlen, b->instrlen);
>> -			a->instrlen = b->instrlen = len;
>> +		if (!instances) {
>> +			for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
>> +				u8 len = max(a->instrlen, b->instrlen);
>> +				a->instrlen = b->instrlen = len;
>> +			}
>> +			instances = b - a;
>> +			patched = false;
>>   		}
>>   
>>   		instr = instr_va(a);
>> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   		 * - feature not present but ALT_FLAG_NOT is set to mean,
>>   		 *   patch if feature is *NOT* present.
>>   		 */
>> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
>> -			memcpy(insn_buff, instr, a->instrlen);
>> -			optimize_nops(instr, insn_buff, a->instrlen);
>> -		} else {
>> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>>   			apply_one_alternative(instr, insn_buff, a);
>> +			patched = true;
>>   		}
>>   
>> -		text_poke_early(instr, insn_buff, a->instrlen);
>> +		instances--;
>> +		if (!instances) {
>> +			if (!patched) {
>> +				memcpy(insn_buff, instr, a->instrlen);
>> +				optimize_nops(instr, insn_buff, a->instrlen);
>> +			}
>> +			text_poke_early(instr, insn_buff, a->instrlen);
>> +		}
>>   	}
>>   
>>   	kasan_enable_current();
> 
> I think you lost the optimize_nops() call for the patched case.
> 
> That is, note how apply_one_alternative() does 0x90 padding, but then
> you only do optimize_nops() when !patched.

The call of optimize_nops() is part of text_poke_apply_relocation() when
patching, like without my series.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/3] x86/alternative: Patch a single alternative location only once

[Peter Zijlstra]

On Tue, Sep 30, 2025 at 09:33:25AM +0200, Jürgen Groß wrote:
> On 30.09.25 09:26, Peter Zijlstra wrote:

> > > @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
> > >   		 * - feature not present but ALT_FLAG_NOT is set to mean,
> > >   		 *   patch if feature is *NOT* present.
> > >   		 */
> > > -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
> > > -			memcpy(insn_buff, instr, a->instrlen);
> > > -			optimize_nops(instr, insn_buff, a->instrlen);
> > > -		} else {
> > > +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
> > >   			apply_one_alternative(instr, insn_buff, a);
> > > +			patched = true;
> > >   		}
> > > -		text_poke_early(instr, insn_buff, a->instrlen);
> > > +		instances--;
> > > +		if (!instances) {
> > > +			if (!patched) {
> > > +				memcpy(insn_buff, instr, a->instrlen);
> > > +				optimize_nops(instr, insn_buff, a->instrlen);
> > > +			}
> > > +			text_poke_early(instr, insn_buff, a->instrlen);
> > > +		}
> > >   	}
> > >   	kasan_enable_current();
> > 
> > I think you lost the optimize_nops() call for the patched case.
> > 
> > That is, note how apply_one_alternative() does 0x90 padding, but then
> > you only do optimize_nops() when !patched.
> 
> The call of optimize_nops() is part of text_poke_apply_relocation() when
> patching, like without my series.

Indeed it is. Clearly I need more wakeup juice :-)

Therefore the patches seem fine. I'll try and hold onto them until the
merge window closes and then stick them in x86/core or thereabout.

Re: [PATCH 3/3] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1670 bytes --]

On 30.09.25 09:39, Peter Zijlstra wrote:
> On Tue, Sep 30, 2025 at 09:33:25AM +0200, Jürgen Groß wrote:
>> On 30.09.25 09:26, Peter Zijlstra wrote:
> 
>>>> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>>>    		 * - feature not present but ALT_FLAG_NOT is set to mean,
>>>>    		 *   patch if feature is *NOT* present.
>>>>    		 */
>>>> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
>>>> -			memcpy(insn_buff, instr, a->instrlen);
>>>> -			optimize_nops(instr, insn_buff, a->instrlen);
>>>> -		} else {
>>>> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>>>>    			apply_one_alternative(instr, insn_buff, a);
>>>> +			patched = true;
>>>>    		}
>>>> -		text_poke_early(instr, insn_buff, a->instrlen);
>>>> +		instances--;
>>>> +		if (!instances) {
>>>> +			if (!patched) {
>>>> +				memcpy(insn_buff, instr, a->instrlen);
>>>> +				optimize_nops(instr, insn_buff, a->instrlen);
>>>> +			}
>>>> +			text_poke_early(instr, insn_buff, a->instrlen);
>>>> +		}
>>>>    	}
>>>>    	kasan_enable_current();
>>>
>>> I think you lost the optimize_nops() call for the patched case.
>>>
>>> That is, note how apply_one_alternative() does 0x90 padding, but then
>>> you only do optimize_nops() when !patched.
>>
>> The call of optimize_nops() is part of text_poke_apply_relocation() when
>> patching, like without my series.
> 
> Indeed it is. Clearly I need more wakeup juice :-)
> 
> Therefore the patches seem fine. I'll try and hold onto them until the
> merge window closes and then stick them in x86/core or thereabout.

Thanks.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[tip: x86/core] x86/alternative: Patch a single alternative location only once

[tip-bot2 for Juergen Gross]

The following commit has been merged into the x86/core branch of tip:

Commit-ID:     a17e1039874831c49d3c15d699a65a8f8130dbb5
Gitweb:        https://git.kernel.org/tip/a17e1039874831c49d3c15d699a65a8f8130dbb5
Author:        Juergen Gross <jgross@suse.com>
AuthorDate:    Mon, 29 Sep 2025 13:29:47 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 14 Oct 2025 10:38:11 +02:00

x86/alternative: Patch a single alternative location only once

Instead of patching a single location potentially multiple times in
case of nested ALTERNATIVE()s, do the patching only after having
evaluated all alt_instr instances for that location.

This has multiple advantages:

- In case of replacing an indirect with a direct call using the
  ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
  instance before any other instances at the same location (the
  original instruction is needed for finding the target of the direct
  call).

- In case of nested ALTERNATIVE()s there is no intermediate replacement
  visible. This avoids any problems in case e.g. an interrupt is
  happening between the single instances and the patched location is
  used during handling the interrupt.

Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/alternative.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index d86a012..90c0d16 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	u8 insn_buff[MAX_PATCH_LEN];
 	u8 *instr;
 	struct alt_instr *a, *b;
+	unsigned int instances = 0;
+	bool patched = false;
 
 	DPRINTK(ALT, "alt table %px, -> %px", start, end);
 
@@ -677,9 +679,13 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		 * padding for all alt_instr entries for this site (nested
 		 * alternatives result in consecutive entries).
 		 */
-		for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
-			u8 len = max(a->instrlen, b->instrlen);
-			a->instrlen = b->instrlen = len;
+		if (!instances) {
+			for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
+				u8 len = max(a->instrlen, b->instrlen);
+				a->instrlen = b->instrlen = len;
+			}
+			instances = b - a;
+			patched = false;
 		}
 
 		instr = instr_va(a);
@@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		 * - feature not present but ALT_FLAG_NOT is set to mean,
 		 *   patch if feature is *NOT* present.
 		 */
-		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
-			memcpy(insn_buff, instr, a->instrlen);
-			optimize_nops(instr, insn_buff, a->instrlen);
-		} else {
+		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
 			apply_one_alternative(instr, insn_buff, a);
+			patched = true;
 		}
 
-		text_poke_early(instr, insn_buff, a->instrlen);
+		instances--;
+		if (!instances) {
+			if (!patched) {
+				memcpy(insn_buff, instr, a->instrlen);
+				optimize_nops(instr, insn_buff, a->instrlen);
+			}
+			text_poke_early(instr, insn_buff, a->instrlen);
+		}
 	}
 
 	kasan_enable_current();

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 08:42:34AM -0000, tip-bot2 for Juergen Gross wrote:
> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  	u8 insn_buff[MAX_PATCH_LEN];
>  	u8 *instr;
>  	struct alt_instr *a, *b;
> +	unsigned int instances = 0;
> +	bool patched = false;

Except that we have the reverse fir tree rule in tip for function-local vars.

The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;

> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  		 * - feature not present but ALT_FLAG_NOT is set to mean,
>  		 *   patch if feature is *NOT* present.
>  		 */
> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
> -			memcpy(insn_buff, instr, a->instrlen);
> -			optimize_nops(instr, insn_buff, a->instrlen);
> -		} else {
> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>  			apply_one_alternative(instr, insn_buff, a);
> +			patched = true;
>  		}
>  
> -		text_poke_early(instr, insn_buff, a->instrlen);
> +		instances--;
> +		if (!instances) {
> +			if (!patched) {

I don't see how this is making this code better - this is slowly turning into
an unreadable mess with those magic "instances" and "patched".

And frankly, the justification for this patch is also meh: an interrupt might
use the location?!? If this is a real issue then we better disable IRQs around
it. But not make the code yucky.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 02:59:09PM +0200, Borislav Petkov wrote:
> And frankly, the justification for this patch is also meh: an interrupt might
> use the location?!? If this is a real issue then we better disable IRQs around
> it. But not make the code yucky.

And this patch is not doing what it is claiming is doing:

Here's an incarnation of XSTATE_XSAVE which is a 3-way alternative:

At location ffffffff81283cd3, it replaces the default XSAVE with XSAVEOPT.

[    2.456696] SMP alternatives: feat: 10*32+0, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6d9, len: 6) flags: 0x0
[    2.459317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40	xsave64 0x40(%r12)
[    2.460806] SMP alternatives: ffffffff89c1e6d9:   rpl_insn: 49 0f ae 74 24 40	xsaveopt64 0x40(%r12)
[    2.463316] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f ae 74 24 40

and then that exact same location:

[    2.464757] SMP alternatives: feat: 10*32+1, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6df, len: 6) flags: 0x0
[    2.467317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40
[    2.468746] SMP alternatives: ffffffff89c1e6df:   rpl_insn: 49 0f c7 64 24 40	xsaves64 0x40(%r12)
[    2.470167] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f c7 64 24 40

gets XSAVES.

So, long story short, this needs more thought:

1. check whether patching is needed

2. a helper function evaluates all instances and figures out the final insn
   bytes which need to be patched along with the proper padding

3. the proper bytes are copied into the target location and all good

But not like this - the idea is somewhat ok but it needs to be executed in
a cleaner manner.

I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2229 bytes --]

On 14.10.25 15:25, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 02:59:09PM +0200, Borislav Petkov wrote:
>> And frankly, the justification for this patch is also meh: an interrupt might
>> use the location?!? If this is a real issue then we better disable IRQs around
>> it. But not make the code yucky.
> 
> And this patch is not doing what it is claiming is doing:
> 
> Here's an incarnation of XSTATE_XSAVE which is a 3-way alternative:
> 
> At location ffffffff81283cd3, it replaces the default XSAVE with XSAVEOPT.
> 
> [    2.456696] SMP alternatives: feat: 10*32+0, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6d9, len: 6) flags: 0x0
> [    2.459317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40	xsave64 0x40(%r12)
> [    2.460806] SMP alternatives: ffffffff89c1e6d9:   rpl_insn: 49 0f ae 74 24 40	xsaveopt64 0x40(%r12)
> [    2.463316] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f ae 74 24 40
> 
> and then that exact same location:
> 
> [    2.464757] SMP alternatives: feat: 10*32+1, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6df, len: 6) flags: 0x0
> [    2.467317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40
> [    2.468746] SMP alternatives: ffffffff89c1e6df:   rpl_insn: 49 0f c7 64 24 40	xsaves64 0x40(%r12)
> [    2.470167] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f c7 64 24 40
> 
> gets XSAVES.

Oh, indeed, I should have adapted the printing. The patching is fine,
it is just the debug output which claims to have written 2 different
instructions to old_insn, while it didn't.

> 
> So, long story short, this needs more thought:
> 
> 1. check whether patching is needed
> 
> 2. a helper function evaluates all instances and figures out the final insn
>     bytes which need to be patched along with the proper padding
> 
> 3. the proper bytes are copied into the target location and all good
> 
> But not like this - the idea is somewhat ok but it needs to be executed in
> a cleaner manner.
> 
> I'd say.

I can have a try with this approach.

Could take some time, vacation is coming up...


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 04:13:11PM +0200, Jürgen Groß wrote:
> I can have a try with this approach.
> 
> Could take some time, vacation is coming up...

Thanks.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Peter Zijlstra]

On Tue, Oct 14, 2025 at 04:13:11PM +0200, Jürgen Groß wrote:

> Could take some time, vacation is coming up...

Enjoy! This patch has meanwhile gone away.

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 486 bytes --]

On 14.10.25 16:18, Peter Zijlstra wrote:
> On Tue, Oct 14, 2025 at 04:13:11PM +0200, Jürgen Groß wrote:
> 
>> Could take some time, vacation is coming up...
> 
> Enjoy! This patch has meanwhile gone away.
> 

I'm not sure the "x86/alternative: Refactor apply_alternatives()"
patch will stay the way it is now. Maybe better to remove it, too.

"x86/alternative: Drop not needed test after call of alt_replace_call()"
won't be changed, so it can be kept IMHO.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 04:26:07PM +0200, Juergen Gross wrote:
> I'm not sure the "x86/alternative: Refactor apply_alternatives()"
> patch will stay the way it is now. Maybe better to remove it, too.

Ok, lemme zap it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1706 bytes --]

On 14.10.25 14:59, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 08:42:34AM -0000, tip-bot2 for Juergen Gross wrote:
>> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   	u8 insn_buff[MAX_PATCH_LEN];
>>   	u8 *instr;
>>   	struct alt_instr *a, *b;
>> +	unsigned int instances = 0;
>> +	bool patched = false;
> 
> Except that we have the reverse fir tree rule in tip for function-local vars.

Okay.

>> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   		 * - feature not present but ALT_FLAG_NOT is set to mean,
>>   		 *   patch if feature is *NOT* present.
>>   		 */
>> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
>> -			memcpy(insn_buff, instr, a->instrlen);
>> -			optimize_nops(instr, insn_buff, a->instrlen);
>> -		} else {
>> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>>   			apply_one_alternative(instr, insn_buff, a);
>> +			patched = true;
>>   		}
>>   
>> -		text_poke_early(instr, insn_buff, a->instrlen);
>> +		instances--;
>> +		if (!instances) {
>> +			if (!patched) {
> 
> I don't see how this is making this code better - this is slowly turning into
> an unreadable mess with those magic "instances" and "patched".
> 
> And frankly, the justification for this patch is also meh: an interrupt might
> use the location?!? If this is a real issue then we better disable IRQs around
> it. But not make the code yucky.

For one it is not the only justification.

And just for the record: NMI handling is using ALTERNATIVE_2, so you
can't just "disable interrupts" and be sure it will work.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 04:08:47PM +0200, Jürgen Groß wrote:
> And just for the record: NMI handling is using ALTERNATIVE_2, so you
> can't just "disable interrupts" and be sure it will work.

Are you fixing anything you're actually hitting while patching alternatives or
are you talking hypothetically here?

Because I can't think of an easy way to trigger NMIs that early during boot,
when not even SMP is up.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1218 bytes --]

On 14.10.25 16:12, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 04:08:47PM +0200, Jürgen Groß wrote:
>> And just for the record: NMI handling is using ALTERNATIVE_2, so you
>> can't just "disable interrupts" and be sure it will work.
> 
> Are you fixing anything you're actually hitting while patching alternatives or
> are you talking hypothetically here?

I really ran into issues while writing my paravirt MSR series, as I had
an ALTERNATIVE3() invocation modifying the original instruction (the
indirect paravirt call) with an WRMSR and then trying to turn the no
longer existing indirect call into a direct one.

So I didn't have the intermediate issue in interrupt handling, but the
issue with rules how to place indirect call replacements in ALTERNATIVEn()
invocations.

The interrupt handling still might be a thing, while NMI is for sure rather
unlikely.

> 
> Because I can't think of an easy way to trigger NMIs that early during boot,
> when not even SMP is up.
> 

NMIs are probably really a more theoretical issue. And even if NMIs are
affected, they will still be affected in case they happen just when half of
the new instruction bytes have been written.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 04:22:09PM +0200, Jürgen Groß wrote:
> I really ran into issues while writing my paravirt MSR series, as I had
> an ALTERNATIVE3() invocation modifying the original instruction (the
> indirect paravirt call) with an WRMSR and then trying to turn the no
> longer existing indirect call into a direct one.

So put *that* in the commit message. Along with a detailed example of what
you were seeing. This is bazillion miles more useful than some hypothetically,
potentially ... case.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1000 bytes --]

On 14.10.25 16:26, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 04:22:09PM +0200, Jürgen Groß wrote:
>> I really ran into issues while writing my paravirt MSR series, as I had
>> an ALTERNATIVE3() invocation modifying the original instruction (the
>> indirect paravirt call) with an WRMSR and then trying to turn the no
>> longer existing indirect call into a direct one.
> 
> So put *that* in the commit message. Along with a detailed example of what
> you were seeing. This is bazillion miles more useful than some hypothetically,
> potentially ... case.

I have this here in the commit message:

   - In case of replacing an indirect with a direct call using the
     ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
     instance before any other instances at the same location (the
     original instruction is needed for finding the target of the direct
     call).

which is explaining why the problem is occurring. Isn't that enough?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Borislav Petkov]

On Tue, Oct 14, 2025 at 04:31:31PM +0200, Jürgen Groß wrote:
> I have this here in the commit message:
> 
>   - In case of replacing an indirect with a direct call using the
>     ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>     instance before any other instances at the same location (the
>     original instruction is needed for finding the target of the direct
>     call).
> 
> which is explaining why the problem is occurring. Isn't that enough?

I can guess what this is about but a concrete example here would make it
a lot clearer, I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/core] x86/alternative: Patch a single alternative location only once

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 719 bytes --]

On 14.10.25 16:39, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 04:31:31PM +0200, Jürgen Groß wrote:
>> I have this here in the commit message:
>>
>>    - In case of replacing an indirect with a direct call using the
>>      ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>>      instance before any other instances at the same location (the
>>      original instruction is needed for finding the target of the direct
>>      call).
>>
>> which is explaining why the problem is occurring. Isn't that enough?
> 
> I can guess what this is about but a concrete example here would make it
> a lot clearer, I'd say.

Okay, I'll expand that with the concrete example.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]