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

[PATCH v3 0/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.

Changes in V2:
- complete rework (Boris Petkov)

Changes in V3:
- split former V2 patch into 2 by introducing a helper function (Boris Petkov)
- repost the small cleanup patch 1 which was taken before, but has somehow
  vanished from the tip x86/alternative branch (it is still in the tip
  master branch, but I couldn't find it in any other tip branch).

Juergen Gross (3):
  x86/alternative: Drop not needed test after call of alt_replace_call()
  x86/alternative: Use a helper function for patching alternatives
  x86/alternative: Patch a single alternative location only once

 arch/x86/kernel/alternative.c | 135 +++++++++++++++++++---------------
 1 file changed, 76 insertions(+), 59 deletions(-)

-- 
2.51.0

[PATCH v3 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>
---
 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 8ee5ff547357..4f3ea50e41e8 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

[PATCH v3 2/3] x86/alternative: Use a helper function for patching alternatives

[Juergen Gross]

Tidy up apply_alternatives() by moving the main patching action of a
single alternative instance into a helper function.

As the original instruction is being patched with optimized padding
NOPs even in case no replacement instruction has been selected, do
the main patching action in the main loop. This requires to use
__apply_relocation() instead of text_poke_apply_relocation() in
order to have only a single call of optimize_nops(), resulting in a
more transparent coding (the question why optimize_nops() isn't
being called in all cases has come up more than once in the past).

Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 arch/x86/kernel/alternative.c | 121 +++++++++++++++++++---------------
 1 file changed, 67 insertions(+), 54 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 4f3ea50e41e8..248e5f212a2b 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -604,6 +604,68 @@ static inline u8 * instr_va(struct alt_instr *i)
 	return (u8 *)&i->instr_offset + i->instr_offset;
 }
 
+struct patch_site {
+	u8 *instr;
+	u8 buff[MAX_PATCH_LEN];
+	u8 len;
+};
+
+static void __init_or_module analyze_patch_site(struct patch_site *ps,
+						struct alt_instr *p, struct alt_instr *end)
+{
+	struct alt_instr *r;
+	u8 buff_sz;
+	u8 *repl;
+
+	/*
+	 * In case of nested ALTERNATIVE()s the outer alternative might add
+	 * more padding. To ensure consistent patching find the max padding for
+	 * all alt_instr entries for this site (nested alternatives result in
+	 * consecutive entries).
+	 */
+	ps->instr = instr_va(p);
+	ps->len = p->instrlen;
+	for (r = p+1; r < end && instr_va(r) == ps->instr; r++) {
+		ps->len = max(ps->len, r->instrlen);
+		p->instrlen = r->instrlen = ps->len;
+	}
+
+	BUG_ON(ps->len > sizeof(ps->buff));
+	BUG_ON(p->cpuid >= (NCAPINTS + NBUGINTS) * 32);
+
+	/*
+	 * Patch if either:
+	 * - feature is present
+	 * - feature not present but ALT_FLAG_NOT is set to mean,
+	 *   patch if feature is *NOT* present.
+	 */
+	if (!boot_cpu_has(p->cpuid) == !(p->flags & ALT_FLAG_NOT)) {
+		memcpy(ps->buff, ps->instr, ps->len);
+		return;
+	}
+
+	repl = (u8 *)&p->repl_offset + p->repl_offset;
+	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
+		p->cpuid >> 5, p->cpuid & 0x1f,
+		ps->instr, ps->instr, ps->len,
+		repl, p->replacementlen, p->flags);
+
+	memcpy(ps->buff, repl, p->replacementlen);
+	buff_sz = p->replacementlen;
+
+	if (p->flags & ALT_FLAG_DIRECT_CALL)
+		buff_sz = alt_replace_call(ps->instr, ps->buff, p);
+
+	for (; buff_sz < ps->len; buff_sz++)
+		ps->buff[buff_sz] = 0x90;
+
+	__apply_relocation(ps->buff, ps->instr, ps->len, repl, p->replacementlen);
+
+	DUMP_BYTES(ALT, ps->instr, ps->len, "%px:   old_insn: ", ps->instr);
+	DUMP_BYTES(ALT, repl, p->replacementlen, "%px:   rpl_insn: ", repl);
+	DUMP_BYTES(ALT, ps->buff, ps->len, "%px: final_insn: ", ps->instr);
+}
+
 /*
  * Replace instructions with better alternatives for this CPU type. This runs
  * before SMP is initialized to avoid SMP problems with self modifying code.
@@ -617,9 +679,7 @@ static inline u8 * instr_va(struct alt_instr *i)
 void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 						  struct alt_instr *end)
 {
-	u8 insn_buff[MAX_PATCH_LEN];
-	u8 *instr, *replacement;
-	struct alt_instr *a, *b;
+	struct alt_instr *a;
 
 	DPRINTK(ALT, "alt table %px, -> %px", start, end);
 
@@ -643,59 +703,12 @@ 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
-		 * 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;
-		}
-
-		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);
-
-		/*
-		 * Patch if either:
-		 * - feature is present
-		 * - 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);
-			text_poke_early(instr, insn_buff, a->instrlen);
-			continue;
-		}
-
-		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);
+		struct patch_site ps;
 
-		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);
+		analyze_patch_site(&ps, a, end);
 
-		text_poke_early(instr, insn_buff, insn_buff_sz);
+		optimize_nops(ps.instr, ps.buff, ps.len);
+		text_poke_early(ps.instr, ps.buff, ps.len);
 	}
 
 	kasan_enable_current();
-- 
2.51.0

[PATCH v3 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).
  This issue has been hit when trying to do paravirt patching similar
  to the following:
    ALTERNATIVE_2(PARAVIRT_CALL,    // indirect call
                  instr, feature,   // native instruction
                  ALT_CALL_INSTR, X86_FEATURE_XENPV)  // Xen function
  In case "feature" was true, "instr" replaced the indirect call. Under
  Xen PV the patching to have a direct call failed, as the original
  indirect call was no longer there to find the call target.

- 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:
- complete rework (Boris Petkov)
V3:
- rebase to added patch 2
---
 arch/x86/kernel/alternative.c | 59 ++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 248e5f212a2b..1d8dbf5521e9 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -610,10 +610,10 @@ struct patch_site {
 	u8 len;
 };
 
-static void __init_or_module analyze_patch_site(struct patch_site *ps,
-						struct alt_instr *p, struct alt_instr *end)
+static struct alt_instr * __init_or_module analyze_patch_site(
+	struct patch_site *ps, struct alt_instr *p, struct alt_instr *end)
 {
-	struct alt_instr *r;
+	struct alt_instr *r = NULL;
 	u8 buff_sz;
 	u8 *repl;
 
@@ -622,48 +622,54 @@ static void __init_or_module analyze_patch_site(struct patch_site *ps,
 	 * more padding. To ensure consistent patching find the max padding for
 	 * all alt_instr entries for this site (nested alternatives result in
 	 * consecutive entries).
+	 * Find the last alt_instr eligible for patching at the site.
 	 */
 	ps->instr = instr_va(p);
-	ps->len = p->instrlen;
-	for (r = p+1; r < end && instr_va(r) == ps->instr; r++) {
-		ps->len = max(ps->len, r->instrlen);
-		p->instrlen = r->instrlen = ps->len;
+	ps->len = 0;
+	for (; p < end && instr_va(p) == ps->instr; p++) {
+		ps->len = max(ps->len, p->instrlen);
+
+		BUG_ON(p->cpuid >= (NCAPINTS + NBUGINTS) * 32);
+		/*
+		 * Patch if either:
+		 * - feature is present
+		 * - feature not present but ALT_FLAG_NOT is set to mean,
+		 *   patch if feature is *NOT* present.
+		 */
+		if (!boot_cpu_has(p->cpuid) != !(p->flags & ALT_FLAG_NOT))
+			r = p;
 	}
 
 	BUG_ON(ps->len > sizeof(ps->buff));
-	BUG_ON(p->cpuid >= (NCAPINTS + NBUGINTS) * 32);
 
-	/*
-	 * Patch if either:
-	 * - feature is present
-	 * - feature not present but ALT_FLAG_NOT is set to mean,
-	 *   patch if feature is *NOT* present.
-	 */
-	if (!boot_cpu_has(p->cpuid) == !(p->flags & ALT_FLAG_NOT)) {
+	if (!r) {
+		/* Nothing to patch, use original instruction. */
 		memcpy(ps->buff, ps->instr, ps->len);
-		return;
+		return p;
 	}
 
-	repl = (u8 *)&p->repl_offset + p->repl_offset;
+	repl = (u8 *)&r->repl_offset + r->repl_offset;
 	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
-		p->cpuid >> 5, p->cpuid & 0x1f,
+		r->cpuid >> 5, r->cpuid & 0x1f,
 		ps->instr, ps->instr, ps->len,
-		repl, p->replacementlen, p->flags);
+		repl, r->replacementlen, r->flags);
 
-	memcpy(ps->buff, repl, p->replacementlen);
-	buff_sz = p->replacementlen;
+	memcpy(ps->buff, repl, r->replacementlen);
+	buff_sz = r->replacementlen;
 
-	if (p->flags & ALT_FLAG_DIRECT_CALL)
-		buff_sz = alt_replace_call(ps->instr, ps->buff, p);
+	if (r->flags & ALT_FLAG_DIRECT_CALL)
+		buff_sz = alt_replace_call(ps->instr, ps->buff, r);
 
 	for (; buff_sz < ps->len; buff_sz++)
 		ps->buff[buff_sz] = 0x90;
 
-	__apply_relocation(ps->buff, ps->instr, ps->len, repl, p->replacementlen);
+	__apply_relocation(ps->buff, ps->instr, ps->len, repl, r->replacementlen);
 
 	DUMP_BYTES(ALT, ps->instr, ps->len, "%px:   old_insn: ", ps->instr);
 	DUMP_BYTES(ALT, repl, p->replacementlen, "%px:   rpl_insn: ", repl);
 	DUMP_BYTES(ALT, ps->buff, ps->len, "%px: final_insn: ", ps->instr);
+
+	return p;
 }
 
 /*
@@ -702,10 +708,11 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	 * So be careful if you want to change the scan order to any other
 	 * order.
 	 */
-	for (a = start; a < end; a++) {
+	a = start;
+	while (a < end) {
 		struct patch_site ps;
 
-		analyze_patch_site(&ps, a, end);
+		a = analyze_patch_site(&ps, a, end);
 
 		optimize_nops(ps.instr, ps.buff, ps.len);
 		text_poke_early(ps.instr, ps.buff, ps.len);
-- 
2.51.0

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

[Borislav Petkov]

On Mon, Nov 10, 2025 at 09:23:36AM +0100, Juergen Gross wrote:
> Changes in V3:
> - split former V2 patch into 2 by introducing a helper function (Boris Petkov)
> - repost the small cleanup patch 1 which was taken before, but has somehow
>   vanished from the tip x86/alternative branch (it is still in the tip
>   master branch, but I couldn't find it in any other tip branch).

You mean this one?

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/core&id=ad74016b919cbad78d203fa1c459ae18e73ce586

-- 
Regards/Gruss,
    Boris.

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

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

[Jürgen Groß]

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

On 16.11.25 13:42, Borislav Petkov wrote:
> On Mon, Nov 10, 2025 at 09:23:36AM +0100, Juergen Gross wrote:
>> Changes in V3:
>> - split former V2 patch into 2 by introducing a helper function (Boris Petkov)
>> - repost the small cleanup patch 1 which was taken before, but has somehow
>>    vanished from the tip x86/alternative branch (it is still in the tip
>>    master branch, but I couldn't find it in any other tip branch).
> 
> You mean this one?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/core&id=ad74016b919cbad78d203fa1c459ae18e73ce586
> 

Yes.


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 v3 2/3] x86/alternative: Use a helper function for patching alternatives

[Borislav Petkov]

On Mon, Nov 10, 2025 at 09:23:38AM +0100, Juergen Gross wrote:
> +static void __init_or_module analyze_patch_site(struct patch_site *ps,
> +						struct alt_instr *p, struct alt_instr *end)
> +{
> +	struct alt_instr *r;
> +	u8 buff_sz;
> +	u8 *repl;
> +
> +	/*
> +	 * In case of nested ALTERNATIVE()s the outer alternative might add
> +	 * more padding. To ensure consistent patching find the max padding for
> +	 * all alt_instr entries for this site (nested alternatives result in
> +	 * consecutive entries).
> +	 */
> +	ps->instr = instr_va(p);
> +	ps->len = p->instrlen;
> +	for (r = p+1; r < end && instr_va(r) == ps->instr; r++) {
> +		ps->len = max(ps->len, r->instrlen);
> +		p->instrlen = r->instrlen = ps->len;
> +	}
> +
> +	BUG_ON(ps->len > sizeof(ps->buff));
> +	BUG_ON(p->cpuid >= (NCAPINTS + NBUGINTS) * 32);
> +
> +	/*
> +	 * Patch if either:
> +	 * - feature is present
> +	 * - feature not present but ALT_FLAG_NOT is set to mean,
> +	 *   patch if feature is *NOT* present.
> +	 */
> +	if (!boot_cpu_has(p->cpuid) == !(p->flags & ALT_FLAG_NOT)) {
> +		memcpy(ps->buff, ps->instr, ps->len);
> +		return;
> +	}
> +
> +	repl = (u8 *)&p->repl_offset + p->repl_offset;
> +	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
> +		p->cpuid >> 5, p->cpuid & 0x1f,
> +		ps->instr, ps->instr, ps->len,
> +		repl, p->replacementlen, p->flags);
> +
> +	memcpy(ps->buff, repl, p->replacementlen);
> +	buff_sz = p->replacementlen;
> +
> +	if (p->flags & ALT_FLAG_DIRECT_CALL)
> +		buff_sz = alt_replace_call(ps->instr, ps->buff, p);
> +
> +	for (; buff_sz < ps->len; buff_sz++)
> +		ps->buff[buff_sz] = 0x90;
> +
> +	__apply_relocation(ps->buff, ps->instr, ps->len, repl, p->replacementlen);
> +
> +	DUMP_BYTES(ALT, ps->instr, ps->len, "%px:   old_insn: ", ps->instr);
> +	DUMP_BYTES(ALT, repl, p->replacementlen, "%px:   rpl_insn: ", repl);
> +	DUMP_BYTES(ALT, ps->buff, ps->len, "%px: final_insn: ", ps->instr);
> +}

Well, this doesn't quite look like what I suggested: if we have a function
analyze_patch_site() then it should do only that - analyze the patch site
*only* and not do the patching too.

With the point being that struct patch_site should carry the necessary
information between an analyze step and a patching step so that you have
simple functions doing one thing and one thing only - not mix up things.

And your 3rd patch is making things even worse again - simply in a different
way.

So please take enough time to split the functionality:

1. one function does only patch sites analysis. Once it is done, the
   patch_site struct will contain *all* possible information for the next
   function:

2. patch site. This one takes the information gathered by the analysis phase
   and uses it to patch the site, fixup direct calls, apply relocations, dump
   debug info and so on.

Remember: the point here is to make the code simpler and understandable and
manageable. Something that is sorely needed in that area before people start
piling up more fancy shit.

But fancy shit will go in *only* after the code is clean and can take fancy
shit naturally. Not dump it ontop and someone else will mop up after.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 2/3] x86/alternative: Use a helper function for patching alternatives

[Jürgen Groß]

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

On 17.11.25 14:41, Borislav Petkov wrote:
> On Mon, Nov 10, 2025 at 09:23:38AM +0100, Juergen Gross wrote:
>> +static void __init_or_module analyze_patch_site(struct patch_site *ps,
>> +						struct alt_instr *p, struct alt_instr *end)
>> +{
>> +	struct alt_instr *r;
>> +	u8 buff_sz;
>> +	u8 *repl;
>> +
>> +	/*
>> +	 * In case of nested ALTERNATIVE()s the outer alternative might add
>> +	 * more padding. To ensure consistent patching find the max padding for
>> +	 * all alt_instr entries for this site (nested alternatives result in
>> +	 * consecutive entries).
>> +	 */
>> +	ps->instr = instr_va(p);
>> +	ps->len = p->instrlen;
>> +	for (r = p+1; r < end && instr_va(r) == ps->instr; r++) {
>> +		ps->len = max(ps->len, r->instrlen);
>> +		p->instrlen = r->instrlen = ps->len;
>> +	}
>> +
>> +	BUG_ON(ps->len > sizeof(ps->buff));
>> +	BUG_ON(p->cpuid >= (NCAPINTS + NBUGINTS) * 32);
>> +
>> +	/*
>> +	 * Patch if either:
>> +	 * - feature is present
>> +	 * - feature not present but ALT_FLAG_NOT is set to mean,
>> +	 *   patch if feature is *NOT* present.
>> +	 */
>> +	if (!boot_cpu_has(p->cpuid) == !(p->flags & ALT_FLAG_NOT)) {
>> +		memcpy(ps->buff, ps->instr, ps->len);
>> +		return;
>> +	}
>> +
>> +	repl = (u8 *)&p->repl_offset + p->repl_offset;
>> +	DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
>> +		p->cpuid >> 5, p->cpuid & 0x1f,
>> +		ps->instr, ps->instr, ps->len,
>> +		repl, p->replacementlen, p->flags);
>> +
>> +	memcpy(ps->buff, repl, p->replacementlen);
>> +	buff_sz = p->replacementlen;
>> +
>> +	if (p->flags & ALT_FLAG_DIRECT_CALL)
>> +		buff_sz = alt_replace_call(ps->instr, ps->buff, p);
>> +
>> +	for (; buff_sz < ps->len; buff_sz++)
>> +		ps->buff[buff_sz] = 0x90;
>> +
>> +	__apply_relocation(ps->buff, ps->instr, ps->len, repl, p->replacementlen);
>> +
>> +	DUMP_BYTES(ALT, ps->instr, ps->len, "%px:   old_insn: ", ps->instr);
>> +	DUMP_BYTES(ALT, repl, p->replacementlen, "%px:   rpl_insn: ", repl);
>> +	DUMP_BYTES(ALT, ps->buff, ps->len, "%px: final_insn: ", ps->instr);
>> +}
> 
> Well, this doesn't quite look like what I suggested: if we have a function
> analyze_patch_site() then it should do only that - analyze the patch site
> *only* and not do the patching too.

It doesn't do the patching. That is done in the caller of
analyze_patch_site().

What it does do is looking at one patch site and putting the code to be
patched in into the temporary buffer and then it is printing the debug
info related to the result of the analysis.
> With the point being that struct patch_site should carry the necessary
> information between an analyze step and a patching step so that you have
> simple functions doing one thing and one thing only - not mix up things.
> 
> And your 3rd patch is making things even worse again - simply in a different
> way.
It is following the same direction:

- analyze the patch site (now all of the alternatives for one location)
- putting the code to be patched into the buffer
- print the debug info related to that patch site

> 
> So please take enough time to split the functionality:
> 
> 1. one function does only patch sites analysis. Once it is done, the
>     patch_site struct will contain *all* possible information for the next
>     function:
> 
> 2. patch site. This one takes the information gathered by the analysis phase
>     and uses it to patch the site, fixup direct calls, apply relocations, dump
>     debug info and so on.

The reason I was doing that way was to _really_ have the patching done only
once. This includes the case when the original instruction is kept and just
the nops are being optimized.

I can add one other layer doing the split you are asking for: one for gathering
the information and one for applying relocs and debug printing. But I'd really
like to keep the final patching in apply_alternatives(), as this makes it very
clear where the final patching of the code is done.


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 v3 2/3] x86/alternative: Use a helper function for patching alternatives

[Borislav Petkov]

On Mon, Nov 17, 2025 at 03:45:05PM +0100, Jürgen Groß wrote:
> It doesn't do the patching. That is done in the caller of
> analyze_patch_site().

It doesn't do patch site analysis only either.

> and putting the code to be patched in into the temporary buffer and then it
> is printing the debug info related to the result of the analysis.

This belongs more into the patch_site function, I'd say.

> I can add one other layer doing the split you are asking for: one for gathering
> the information and one for applying relocs and debug printing. But I'd really
> like to keep the final patching in apply_alternatives(), as this makes it very
> clear where the final patching of the code is done.

Fine by me. And I'd like to have clearly separated stages with functions named
accordingly and functions doing *only* one thing and one thing only.

I'm even more fine if you do:

	analyze_patch_site();
	prep_patch_site();
	patch_site();

and have it perfectly clear and separated and borderline trivial.

Thx.

-- 
Regards/Gruss,
    Boris.

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