[tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[tip-bot2 for Josh Poimboeuf]

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

Commit-ID:     e5ff90b179d45df71373cf79f99d20c9abe229cb
Gitweb:        https://git.kernel.org/tip/e5ff90b179d45df71373cf79f99d20c9abe229cb
Author:        Josh Poimboeuf <jpoimboe@kernel.org>
AuthorDate:    Sun, 02 Mar 2025 17:21:03 -08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 03 Mar 2025 11:39:54 +01:00

x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
asm statement with a call instruction to force the compiler to set up
the frame pointer before doing the call.

Without frame pointers, no such constraint is needed.  Make it
conditional on frame pointers.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/asm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 0d268e6..f1db9e8 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -232,7 +232,11 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
 #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
+#else
+#define ASM_CALL_CONSTRAINT
+#endif
 
 #endif /* __ASSEMBLY__ */
 

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Mon, 3 Mar 2025 at 01:02, tip-bot2 for Josh Poimboeuf
<tip-bot2@linutronix.de> wrote:
>
> x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers
>
> With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
> asm statement with a call instruction to force the compiler to set up
> the frame pointer before doing the call.
>
> Without frame pointers, no such constraint is needed.  Make it
> conditional on frame pointers.

Can we please explain *why* this is done?

It may not be required, but it makes the source code uglier and adds a
conditional. What's the advantage of adding this extra logic?

I'm sure there is some reason for this change, but that reason should
be explained.

Because "we don't need it" cuts both ways. Maybe we don't need the
ASM_CALL_CONSTRAINT, but it also didn't use to hurt us.

The problems seems entirely caused by the change to use a strictly
inferior version of ASM_CALL_CONSTRAINT.

Is there really no better option? Because the new ASM_CALL_CONSTRAINT
seems actively horrendous.

                Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Mon, Mar 03, 2025 at 08:28:10AM -1000, Linus Torvalds wrote:
> On Mon, 3 Mar 2025 at 01:02, tip-bot2 for Josh Poimboeuf
> <tip-bot2@linutronix.de> wrote:
> >
> > x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers
> >
> > With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
> > asm statement with a call instruction to force the compiler to set up
> > the frame pointer before doing the call.
> >
> > Without frame pointers, no such constraint is needed.  Make it
> > conditional on frame pointers.
> 
> Can we please explain *why* this is done?
> 
> It may not be required, but it makes the source code uglier and adds a
> conditional. What's the advantage of adding this extra logic?
> 
> I'm sure there is some reason for this change, but that reason should
> be explained.
> 
> Because "we don't need it" cuts both ways. Maybe we don't need the
> ASM_CALL_CONSTRAINT, but it also didn't use to hurt us.
> 
> The problems seems entirely caused by the change to use a strictly
> inferior version of ASM_CALL_CONSTRAINT.
> 
> Is there really no better option? Because the new ASM_CALL_CONSTRAINT
> seems actively horrendous.

The original ASM_CALL_CONSTRAINT was already pretty horrendous, can you
clarify why you think the new one is even worse?

On thing that's nicer, now that it's an input constraint, it can be
appended to other constraints without affecting the constraint ordering.

As far as making it conditional, the code generation seems better.
Before, it was forcing some functions to set up the frame pointer even
with ORC.

Also it nicely confines the hack to a little CONFIG_FRAME_POINTER
corner.  Frame pointers are very much deprecated on x86-64 anyway, maybe
we can just eventually get rid of that option.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 3 Mar 2025 at 01:02, tip-bot2 for Josh Poimboeuf
> <tip-bot2@linutronix.de> wrote:
> >
> > x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers
> >
> > With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
> > asm statement with a call instruction to force the compiler to set up
> > the frame pointer before doing the call.
> >
> > Without frame pointers, no such constraint is needed.  Make it
> > conditional on frame pointers.
> 
> Can we please explain *why* this is done?
> 
> It may not be required, but it makes the source code uglier and adds a
> conditional. What's the advantage of adding this extra logic?
> 
> I'm sure there is some reason for this change, but that reason should
> be explained.
> 
> Because "we don't need it" cuts both ways. Maybe we don't need the
> ASM_CALL_CONSTRAINT, but it also didn't use to hurt us.
> 
> The problems seems entirely caused by the change to use a strictly
> inferior version of ASM_CALL_CONSTRAINT.
> 
> Is there really no better option? Because the new ASM_CALL_CONSTRAINT
> seems actively horrendous.

So Josh forgot to Cc: lkml in this 5-patch series:

63852     Mar 02 Josh Poimboeuf     | [PATCH 0/5] x86: Fix ASM_CALL_CONSTRAINT for Clang 19 and disable it for ORC
63853     Mar 02 Josh Poimboeuf     | ├─>[PATCH 1/5] KVM: VMX: Use named operands in inline asm
63855     Mar 02 Josh Poimboeuf     | ├─>[PATCH 2/5] x86/hyperv: Use named operands in inline asm
63856     Mar 02 Josh Poimboeuf     | ├─>[PATCH 3/5] x86/alternative: Simplify alternative_call() interface
63857     Mar 02 Josh Poimboeuf     | ├─>[PATCH 4/5] x86: Fix ASM_CALL_CONSTRAINT for Clang 19 + KCOV + KMSAN
63858     Mar 02 Josh Poimboeuf     | ├─>[PATCH 5/5] x86: Make ASM_CALL_CONSTRAINT conditional on frame pointers

... which omission I tried to fix in the commits by adding Cc: lkml - 
but which made the discussion unthreaded on Lore ... a mistake I won't 
repeat.

This reply of yours is for patch 5/5 which is arguably out of context 
in isolation, while the main justification for changing 
ASM_CALL_CONSTRAINT is in 4/5 (a Clang 19 code generation quirk to fix 
a warning) - see attached it below.

The full 5-patch series in -next is:

  cccc85ea4032 KVM: VMX: Use named operands in inline asm
  2668d7b4aff8 x86/hyperv: Use named operands in inline asm
  1edd623ccaaf x86/alternatives: Simplify alternative_call() interface
  96092b7552d9 x86/asm: Fix ASM_CALL_CONSTRAINT for Clang 19 + KCOV + KMSAN
  e5ff90b179d4 x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

I bounced you the key emails, but I'm not sure that helps much.

I'll avoid this situation in the future, Cc:lkml works well for 
individual patches and I used it in the past, but it loses context in 
the series-submission case.

Thanks,

	Ingo

===============================>
From 96092b7552d96f841c94265920c922da72ab46e3 Mon Sep 17 00:00:00 2001
From: Josh Poimboeuf <jpoimboe@kernel.org>
Date: Sun, 2 Mar 2025 17:21:02 -0800
Subject: [PATCH] x86/asm: Fix ASM_CALL_CONSTRAINT for Clang 19 + KCOV + KMSAN

With CONFIG_KCOV and CONFIG_KMSAN enabled, a case was found with Clang
19 where it takes the ASM_CALL_CONSTRAINT output constraint quite
literally by saving and restoring %rsp around the inline asm.  Not only
is that completely unecessary, it confuses objtool and results in the
following warning on Clang 19:

  arch/x86/kvm/cpuid.o: warning: objtool: do_cpuid_func+0x2428: undefined stack state

After some experimentation it was discovered that an input constraint of
__builtin_frame_address(0) generates better code for such cases and
still achieves the desired result of forcing the frame pointer to get
set up for both compilers.  Change ASM_CALL_CONSTRAINT to do that.

Fixes: f5caf621ee35 ("x86/asm: Fix inline asm call constraints for Clang")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: linux-kernel@vger.kernel.org
Closes: https://lore.kernel.org/oe-kbuild-all/202502150634.qjxwSeJR-lkp@intel.com/
---
 arch/x86/include/asm/alternative.h    |  8 ++---
 arch/x86/include/asm/asm.h            |  8 +++--
 arch/x86/include/asm/atomic64_32.h    |  3 +-
 arch/x86/include/asm/cmpxchg_32.h     | 13 ++++-----
 arch/x86/include/asm/irq_stack.h      |  3 +-
 arch/x86/include/asm/mshyperv.h       | 55 ++++++++++++++++++-----------------
 arch/x86/include/asm/paravirt_types.h |  6 ++--
 arch/x86/include/asm/percpu.h         | 34 ++++++++++------------
 arch/x86/include/asm/preempt.h        | 22 +++++++-------
 arch/x86/include/asm/sync_core.h      |  6 ++--
 arch/x86/include/asm/uaccess.h        | 12 ++++----
 arch/x86/include/asm/uaccess_64.h     | 10 ++++---
 arch/x86/include/asm/xen/hypercall.h  |  4 +--
 arch/x86/kernel/alternative.c         |  8 ++---
 arch/x86/kvm/emulate.c                | 11 ++++---
 arch/x86/kvm/vmx/vmx_ops.h            |  3 +-
 16 files changed, 111 insertions(+), 95 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 52626a7251e6..5fcfe96d7c72 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -239,9 +239,10 @@ static inline int alternatives_text_reserved(void *start, void *end)
  */
 #define alternative_call(oldfunc, newfunc, ft_flags, output, input, clobbers...)	\
 	asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags)	\
-		: ALT_OUTPUT_SP(output)							\
+		: output								\
 		: [old] "i" (oldfunc), [new] "i" (newfunc)				\
 		  COMMA(input)								\
+		  COMMA(ASM_CALL_CONSTRAINT)						\
 		: clobbers)
 
 /*
@@ -254,14 +255,13 @@ static inline int alternatives_text_reserved(void *start, void *end)
 			   output, input, clobbers...)					\
 	asm_inline volatile(ALTERNATIVE_2("call %c[old]", "call %c[new1]", ft_flags1,	\
 		"call %c[new2]", ft_flags2)						\
-		: ALT_OUTPUT_SP(output)							\
+		: output								\
 		: [old] "i" (oldfunc), [new1] "i" (newfunc1),				\
 		  [new2] "i" (newfunc2)							\
 		  COMMA(input)								\
+		  COMMA(ASM_CALL_CONSTRAINT)						\
 		: clobbers)
 
-#define ALT_OUTPUT_SP(...) ASM_CALL_CONSTRAINT, ## __VA_ARGS__
-
 /* Macro for creating assembler functions avoiding any C magic. */
 #define DEFINE_ASM_FUNC(func, instr, sec)		\
 	asm (".pushsection " #sec ", \"ax\"\n"		\
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 975ae7a9397e..0d268e6875db 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -213,6 +213,8 @@ static __always_inline __pure void *rip_rel_ptr(void *p)
 
 /* For C file, we already have NOKPROBE_SYMBOL macro */
 
+register unsigned long current_stack_pointer asm(_ASM_SP);
+
 /* Insert a comma if args are non-empty */
 #define COMMA(x...)		__COMMA(x)
 #define __COMMA(...)		, ##__VA_ARGS__
@@ -225,13 +227,13 @@ static __always_inline __pure void *rip_rel_ptr(void *p)
 #define ASM_INPUT(x...)		x
 
 /*
- * This output constraint should be used for any inline asm which has a "call"
+ * This input constraint should be used for any inline asm which has a "call"
  * instruction.  Otherwise the asm may be inserted before the frame pointer
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
-register unsigned long current_stack_pointer asm(_ASM_SP);
-#define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
+#define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
+
 #endif /* __ASSEMBLY__ */
 
 #define _ASM_EXTABLE(from, to)					\
diff --git a/arch/x86/include/asm/atomic64_32.h b/arch/x86/include/asm/atomic64_32.h
index ab838205c1c6..8efb4f2eacb5 100644
--- a/arch/x86/include/asm/atomic64_32.h
+++ b/arch/x86/include/asm/atomic64_32.h
@@ -51,9 +51,10 @@ static __always_inline s64 arch_atomic64_read_nonatomic(const atomic64_t *v)
 #ifdef CONFIG_X86_CX8
 #define __alternative_atomic64(f, g, out, in, clobbers...)		\
 	asm volatile("call %c[func]"					\
-		     : ALT_OUTPUT_SP(out) \
+		     : out						\
 		     : [func] "i" (atomic64_##g##_cx8)			\
 		       COMMA(in)					\
+		       COMMA(ASM_CALL_CONSTRAINT)			\
 		     : clobbers)
 
 #define ATOMIC64_DECL(sym) ATOMIC64_DECL_ONE(sym##_cx8)
diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
index ee89fbc4dd4b..3ae0352604f0 100644
--- a/arch/x86/include/asm/cmpxchg_32.h
+++ b/arch/x86/include/asm/cmpxchg_32.h
@@ -95,9 +95,9 @@ static __always_inline bool __try_cmpxchg64_local(volatile u64 *ptr, u64 *oldp,
 		ALTERNATIVE(_lock_loc					\
 			    "call cmpxchg8b_emu",			\
 			    _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8)	\
-		: ALT_OUTPUT_SP("+a" (o.low), "+d" (o.high))		\
-		: "b" (n.low), "c" (n.high),				\
-		  [ptr] "S" (_ptr)					\
+		: "+a" (o.low), "+d" (o.high)				\
+		: "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)		\
+		  COMMA(ASM_CALL_CONSTRAINT)				\
 		: "memory");						\
 									\
 	o.full;								\
@@ -126,10 +126,9 @@ static __always_inline u64 arch_cmpxchg64_local(volatile u64 *ptr, u64 old, u64
 			    "call cmpxchg8b_emu",			\
 			    _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
 		CC_SET(e)						\
-		: ALT_OUTPUT_SP(CC_OUT(e) (ret),			\
-				"+a" (o.low), "+d" (o.high))		\
-		: "b" (n.low), "c" (n.high),				\
-		  [ptr] "S" (_ptr)					\
+		: CC_OUT(e) (ret), "+a" (o.low), "+d" (o.high)		\
+		: "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)		\
+		  COMMA(ASM_CALL_CONSTRAINT)				\
 		: "memory");						\
 									\
 	if (unlikely(!ret))						\
diff --git a/arch/x86/include/asm/irq_stack.h b/arch/x86/include/asm/irq_stack.h
index 562a547c29a5..8e56a07aef70 100644
--- a/arch/x86/include/asm/irq_stack.h
+++ b/arch/x86/include/asm/irq_stack.h
@@ -92,8 +92,9 @@
 									\
 	"popq	%%rsp					\n"		\
 									\
-	: "+r" (tos), ASM_CALL_CONSTRAINT				\
+	: "+r" (tos)							\
 	: [__func] "i" (func), [tos] "r" (tos) argconstr		\
+	  COMMA(ASM_CALL_CONSTRAINT)					\
 	: "cc", "rax", "rcx", "rdx", "rsi", "rdi", "r8", "r9", "r10",	\
 	  "memory"							\
 	);								\
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 5e6193dbc97e..791a9b2537f0 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -79,9 +79,10 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
 	if (hv_isolation_type_snp() && !hyperv_paravisor_present) {
 		__asm__ __volatile__("mov %[output_address], %%r8\n"
 				     "vmmcall"
-				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-				       "+c" (control), "+d" (input_address)
+				     : "=a" (hv_status), "+c" (control),
+				       "+d" (input_address)
 				     : [output_address] "r" (output_address)
+				       COMMA(ASM_CALL_CONSTRAINT)
 				     : "cc", "memory", "r8", "r9", "r10", "r11");
 		return hv_status;
 	}
@@ -91,10 +92,11 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
 
 	__asm__ __volatile__("mov %[output_address], %%r8\n"
 			     CALL_NOSPEC
-			     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-			       "+c" (control), "+d" (input_address)
+			     : "=a" (hv_status), "+c" (control),
+			       "+d" (input_address)
 			     : [output_address] "r" (output_address),
 			       THUNK_TARGET(hv_hypercall_pg)
+			       COMMA(ASM_CALL_CONSTRAINT)
 			     : "cc", "memory", "r8", "r9", "r10", "r11");
 #else
 	u32 input_address_hi = upper_32_bits(input_address);
@@ -106,12 +108,11 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
 		return U64_MAX;
 
 	__asm__ __volatile__(CALL_NOSPEC
-			     : "=A" (hv_status),
-			       "+c" (input_address_lo), ASM_CALL_CONSTRAINT
-			     : "A" (control),
-			       "b" (input_address_hi),
-			       "D"(output_address_hi), "S"(output_address_lo),
+			     : "=A" (hv_status), "+c" (input_address_lo)
+			     : "A" (control), "b" (input_address_hi),
+			       "D" (output_address_hi), "S"(output_address_lo),
 			       THUNK_TARGET(hv_hypercall_pg)
+			       COMMA(ASM_CALL_CONSTRAINT)
 			     : "cc", "memory");
 #endif /* !x86_64 */
 	return hv_status;
@@ -135,14 +136,16 @@ static inline u64 _hv_do_fast_hypercall8(u64 control, u64 input1)
 	if (hv_isolation_type_snp() && !hyperv_paravisor_present) {
 		__asm__ __volatile__(
 				"vmmcall"
-				: "=a" (hv_status), ASM_CALL_CONSTRAINT,
-				"+c" (control), "+d" (input1)
-				:: "cc", "r8", "r9", "r10", "r11");
+				: "=a" (hv_status), "+c" (control),
+				  "+d" (input1)
+				: ASM_CALL_CONSTRAINT
+				: "cc", "r8", "r9", "r10", "r11");
 	} else {
 		__asm__ __volatile__(CALL_NOSPEC
-				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-				       "+c" (control), "+d" (input1)
+				     : "=a" (hv_status), "+c" (control),
+				       "+d" (input1)
 				     : THUNK_TARGET(hv_hypercall_pg)
+				       COMMA(ASM_CALL_CONSTRAINT)
 				     : "cc", "r8", "r9", "r10", "r11");
 	}
 #else
@@ -151,12 +154,10 @@ static inline u64 _hv_do_fast_hypercall8(u64 control, u64 input1)
 		u32 input1_lo = lower_32_bits(input1);
 
 		__asm__ __volatile__ (CALL_NOSPEC
-				      : "=A"(hv_status),
-					"+c"(input1_lo),
-					ASM_CALL_CONSTRAINT
-				      :	"A" (control),
-					"b" (input1_hi),
+				      : "=A"(hv_status), "+c"(input1_lo)
+				      :	"A" (control), "b" (input1_hi),
 					THUNK_TARGET(hv_hypercall_pg)
+					COMMA(ASM_CALL_CONSTRAINT)
 				      : "cc", "edi", "esi");
 	}
 #endif
@@ -189,17 +190,19 @@ static inline u64 _hv_do_fast_hypercall16(u64 control, u64 input1, u64 input2)
 	if (hv_isolation_type_snp() && !hyperv_paravisor_present) {
 		__asm__ __volatile__("mov %[input2], %%r8\n"
 				     "vmmcall"
-				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-				       "+c" (control), "+d" (input1)
+				     : "=a" (hv_status), "+c" (control),
+				       "+d" (input1)
 				     : [input2] "r" (input2)
+				       COMMA(ASM_CALL_CONSTRAINT)
 				     : "cc", "r8", "r9", "r10", "r11");
 	} else {
 		__asm__ __volatile__("mov %[input2], %%r8\n"
 				     CALL_NOSPEC
-				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-				       "+c" (control), "+d" (input1)
+				     : "=a" (hv_status), "+c" (control),
+				       "+d" (input1)
 				     : [input2] "r" (input2),
 				       THUNK_TARGET(hv_hypercall_pg)
+				       COMMA(ASM_CALL_CONSTRAINT)
 				     : "cc", "r8", "r9", "r10", "r11");
 	}
 #else
@@ -210,11 +213,11 @@ static inline u64 _hv_do_fast_hypercall16(u64 control, u64 input1, u64 input2)
 		u32 input2_lo = lower_32_bits(input2);
 
 		__asm__ __volatile__ (CALL_NOSPEC
-				      : "=A"(hv_status),
-					"+c"(input1_lo), ASM_CALL_CONSTRAINT
+				      : "=A"(hv_status), "+c" (input1_lo)
 				      :	"A" (control), "b" (input1_hi),
-					"D"(input2_hi), "S"(input2_lo),
+					"D" (input2_hi), "S" (input2_lo),
 					THUNK_TARGET(hv_hypercall_pg)
+					COMMA(ASM_CALL_CONSTRAINT)
 				      : "cc");
 	}
 #endif
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index e26633c00455..68bdce684769 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -392,9 +392,10 @@ int paravirt_disable_iospace(void);
 		PVOP_TEST_NULL(op);					\
 		asm volatile(ALTERNATIVE(PARAVIRT_CALL, ALT_CALL_INSTR,	\
 				ALT_CALL_ALWAYS)			\
-			     : call_clbr, ASM_CALL_CONSTRAINT		\
+			     : call_clbr				\
 			     : paravirt_ptr(op),			\
 			       ##__VA_ARGS__				\
+			       COMMA(ASM_CALL_CONSTRAINT)		\
 			     : "memory", "cc" extra_clbr);		\
 		ret;							\
 	})
@@ -407,9 +408,10 @@ int paravirt_disable_iospace(void);
 		asm volatile(ALTERNATIVE_2(PARAVIRT_CALL,		\
 				 ALT_CALL_INSTR, ALT_CALL_ALWAYS,	\
 				 alt, cond)				\
-			     : call_clbr, ASM_CALL_CONSTRAINT		\
+			     : call_clbr				\
 			     : paravirt_ptr(op),			\
 			       ##__VA_ARGS__				\
+			       COMMA(ASM_CALL_CONSTRAINT)		\
 			     : "memory", "cc" extra_clbr);		\
 		ret;							\
 	})
diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 8a8cf86dded3..60390a019ca9 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -323,10 +323,10 @@ do {									\
 	asm_inline qual (						\
 		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
 			    "cmpxchg8b " __percpu_arg([var]), X86_FEATURE_CX8) \
-		: ALT_OUTPUT_SP([var] "+m" (__my_cpu_var(_var)),	\
-				"+a" (old__.low), "+d" (old__.high))	\
-		: "b" (new__.low), "c" (new__.high),			\
-		  "S" (&(_var))						\
+		: [var] "+m" (__my_cpu_var(_var)), "+a" (old__.low),	\
+		   "+d" (old__.high)					\
+		: "b" (new__.low), "c" (new__.high), "S" (&(_var))	\
+		  COMMA(ASM_CALL_CONSTRAINT)				\
 		: "memory");						\
 									\
 	old__.var;							\
@@ -353,11 +353,10 @@ do {									\
 		ALTERNATIVE("call this_cpu_cmpxchg8b_emu",		\
 			    "cmpxchg8b " __percpu_arg([var]), X86_FEATURE_CX8) \
 		CC_SET(z)						\
-		: ALT_OUTPUT_SP(CC_OUT(z) (success),			\
-				[var] "+m" (__my_cpu_var(_var)),	\
-				"+a" (old__.low), "+d" (old__.high))	\
-		: "b" (new__.low), "c" (new__.high),			\
-		  "S" (&(_var))						\
+		: CC_OUT(z) (success), [var] "+m" (__my_cpu_var(_var)),	\
+		  "+a" (old__.low), "+d" (old__.high)			\
+		: "b" (new__.low), "c" (new__.high), "S" (&(_var))	\
+		  COMMA(ASM_CALL_CONSTRAINT)				\
 		: "memory");						\
 	if (unlikely(!success))						\
 		*_oval = old__.var;					\
@@ -392,10 +391,10 @@ do {									\
 	asm_inline qual (						\
 		ALTERNATIVE("call this_cpu_cmpxchg16b_emu",		\
 			    "cmpxchg16b " __percpu_arg([var]), X86_FEATURE_CX16) \
-		: ALT_OUTPUT_SP([var] "+m" (__my_cpu_var(_var)),	\
-				"+a" (old__.low), "+d" (old__.high))	\
-		: "b" (new__.low), "c" (new__.high),			\
-		  "S" (&(_var))						\
+		: [var] "+m" (__my_cpu_var(_var)), "+a" (old__.low),	\
+		   "+d" (old__.high)					\
+		: "b" (new__.low), "c" (new__.high), "S" (&(_var))	\
+		  COMMA(ASM_CALL_CONSTRAINT)				\
 		: "memory");						\
 									\
 	old__.var;							\
@@ -422,11 +421,10 @@ do {									\
 		ALTERNATIVE("call this_cpu_cmpxchg16b_emu",		\
 			    "cmpxchg16b " __percpu_arg([var]), X86_FEATURE_CX16) \
 		CC_SET(z)						\
-		: ALT_OUTPUT_SP(CC_OUT(z) (success),			\
-				[var] "+m" (__my_cpu_var(_var)),	\
-				"+a" (old__.low), "+d" (old__.high))	\
-		: "b" (new__.low), "c" (new__.high),			\
-		  "S" (&(_var))						\
+		: CC_OUT(z) (success), [var] "+m" (__my_cpu_var(_var)),	\
+		  "+a" (old__.low), "+d" (old__.high)			\
+		: "b" (new__.low), "c" (new__.high), "S" (&(_var))	\
+		  COMMA(ASM_CALL_CONSTRAINT)				\
 		: "memory");						\
 	if (unlikely(!success))						\
 		*_oval = old__.var;					\
diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index 919909d8cb77..7e834820e030 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -121,27 +121,29 @@ extern asmlinkage void preempt_schedule_notrace_thunk(void);
 
 DECLARE_STATIC_CALL(preempt_schedule, preempt_schedule_dynamic_enabled);
 
-#define __preempt_schedule() \
-do { \
-	__STATIC_CALL_MOD_ADDRESSABLE(preempt_schedule); \
-	asm volatile ("call " STATIC_CALL_TRAMP_STR(preempt_schedule) : ASM_CALL_CONSTRAINT); \
+#define __preempt_schedule()						\
+do {									\
+	__STATIC_CALL_MOD_ADDRESSABLE(preempt_schedule);		\
+	asm volatile ("call " STATIC_CALL_TRAMP_STR(preempt_schedule)	\
+		      : : ASM_CALL_CONSTRAINT);				\
 } while (0)
 
 DECLARE_STATIC_CALL(preempt_schedule_notrace, preempt_schedule_notrace_dynamic_enabled);
 
-#define __preempt_schedule_notrace() \
-do { \
-	__STATIC_CALL_MOD_ADDRESSABLE(preempt_schedule_notrace); \
-	asm volatile ("call " STATIC_CALL_TRAMP_STR(preempt_schedule_notrace) : ASM_CALL_CONSTRAINT); \
+#define __preempt_schedule_notrace()					\
+do {									\
+	__STATIC_CALL_MOD_ADDRESSABLE(preempt_schedule_notrace);	\
+	asm volatile ("call " STATIC_CALL_TRAMP_STR(preempt_schedule_notrace) \
+		      : : ASM_CALL_CONSTRAINT);				\
 } while (0)
 
 #else /* PREEMPT_DYNAMIC */
 
 #define __preempt_schedule() \
-	asm volatile ("call preempt_schedule_thunk" : ASM_CALL_CONSTRAINT);
+	asm volatile ("call preempt_schedule_thunk" : : ASM_CALL_CONSTRAINT);
 
 #define __preempt_schedule_notrace() \
-	asm volatile ("call preempt_schedule_notrace_thunk" : ASM_CALL_CONSTRAINT);
+	asm volatile ("call preempt_schedule_notrace_thunk" : : ASM_CALL_CONSTRAINT);
 
 #endif /* PREEMPT_DYNAMIC */
 
diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
index 96bda43538ee..c88e354b9ceb 100644
--- a/arch/x86/include/asm/sync_core.h
+++ b/arch/x86/include/asm/sync_core.h
@@ -16,7 +16,7 @@ static __always_inline void iret_to_self(void)
 		"pushl $1f\n\t"
 		"iret\n\t"
 		"1:"
-		: ASM_CALL_CONSTRAINT : : "memory");
+		: : ASM_CALL_CONSTRAINT : "memory");
 }
 #else
 static __always_inline void iret_to_self(void)
@@ -34,7 +34,9 @@ static __always_inline void iret_to_self(void)
 		"pushq $1f\n\t"
 		"iretq\n\t"
 		"1:"
-		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
+		: "=&r" (tmp)
+		: ASM_CALL_CONSTRAINT
+		: "cc", "memory");
 }
 #endif /* CONFIG_X86_32 */
 
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a441..4a5f0c19864e 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -79,9 +79,9 @@ extern int __get_user_bad(void);
 	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
 	__chk_user_ptr(ptr);						\
 	asm volatile("call __" #fn "_%c[size]"				\
-		     : "=a" (__ret_gu), "=r" (__val_gu),		\
-			ASM_CALL_CONSTRAINT				\
-		     : "0" (ptr), [size] "i" (sizeof(*(ptr))));		\
+		     : "=a" (__ret_gu), "=r" (__val_gu)			\
+		     : "0" (ptr), [size] "i" (sizeof(*(ptr)))		\
+		       COMMA(ASM_CALL_CONSTRAINT));			\
 	instrument_get_user(__val_gu);					\
 	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
 	__builtin_expect(__ret_gu, 0);					\
@@ -178,12 +178,12 @@ extern void __put_user_nocheck_8(void);
 	__ptr_pu = __ptr;						\
 	__val_pu = __x;							\
 	asm volatile("call __" #fn "_%c[size]"				\
-		     : "=c" (__ret_pu),					\
-			ASM_CALL_CONSTRAINT				\
+		     : "=c" (__ret_pu)					\
 		     : "0" (__ptr_pu),					\
 		       "r" (__val_pu),					\
 		       [size] "i" (sizeof(*(ptr)))			\
-		     :"ebx");						\
+		       COMMA(ASM_CALL_CONSTRAINT)			\
+		     : "ebx");						\
 	instrument_put_user(__x, __ptr, sizeof(*(ptr)));		\
 	__builtin_expect(__ret_pu, 0);					\
 })
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index c52f0133425b..87a1b9e9cf22 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -129,8 +129,9 @@ copy_user_generic(void *to, const void *from, unsigned long len)
 			    "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
 		"2:\n"
 		_ASM_EXTABLE_UA(1b, 2b)
-		:"+c" (len), "+D" (to), "+S" (from), ASM_CALL_CONSTRAINT
-		: : "memory", "rax");
+		: "+c" (len), "+D" (to), "+S" (from)
+		: ASM_CALL_CONSTRAINT
+		: "memory", "rax");
 	clac();
 	return len;
 }
@@ -191,8 +192,9 @@ static __always_inline __must_check unsigned long __clear_user(void __user *addr
 			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRS))
 		"2:\n"
 	       _ASM_EXTABLE_UA(1b, 2b)
-	       : "+c" (size), "+D" (addr), ASM_CALL_CONSTRAINT
-	       : "a" (0));
+	       : "+c" (size), "+D" (addr)
+	       : "a" (0)
+	         COMMA(ASM_CALL_CONSTRAINT));
 
 	clac();
 
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 97771b9d33af..683772a58939 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -101,7 +101,7 @@ DECLARE_STATIC_CALL(xen_hypercall, xen_hypercall_func);
 	__ADDRESSABLE_xen_hypercall			\
 	"call __SCT__xen_hypercall"
 
-#define __HYPERCALL_ENTRY(x)	"a" (x)
+#define __HYPERCALL_ENTRY(x)	"a" (x) COMMA(ASM_CALL_CONSTRAINT)
 
 #ifdef CONFIG_X86_32
 #define __HYPERCALL_RETREG	"eax"
@@ -127,7 +127,7 @@ DECLARE_STATIC_CALL(xen_hypercall, xen_hypercall_func);
 	register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
 	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5;
 
-#define __HYPERCALL_0PARAM	"=r" (__res), ASM_CALL_CONSTRAINT
+#define __HYPERCALL_0PARAM	"=r" (__res)
 #define __HYPERCALL_1PARAM	__HYPERCALL_0PARAM, "+r" (__arg1)
 #define __HYPERCALL_2PARAM	__HYPERCALL_1PARAM, "+r" (__arg2)
 #define __HYPERCALL_3PARAM	__HYPERCALL_2PARAM, "+r" (__arg3)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 8b66a555d2f0..6a7eb063a5d5 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1624,8 +1624,8 @@ static noinline void __init int3_selftest(void)
 	asm volatile ("int3_selftest_ip:\n\t"
 		      ANNOTATE_NOENDBR
 		      "    int3; nop; nop; nop; nop\n\t"
-		      : ASM_CALL_CONSTRAINT
-		      : __ASM_SEL_RAW(a, D) (&val)
+		      : : __ASM_SEL_RAW(a, D) (&val)
+			  COMMA(ASM_CALL_CONSTRAINT)
 		      : "memory");
 
 	BUG_ON(val != 1);
@@ -1657,8 +1657,8 @@ static noinline void __init alt_reloc_selftest(void)
 	 */
 	asm_inline volatile (
 		ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS)
-		: ASM_CALL_CONSTRAINT
-		: [mem] "m" (__alt_reloc_selftest_addr)
+		: : [mem] "m" (__alt_reloc_selftest_addr)
+		    COMMA(ASM_CALL_CONSTRAINT)
 		: _ASM_ARG1
 	);
 }
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 60986f67c35a..40e12a177197 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1070,7 +1070,9 @@ static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
 	asm("push %[flags]; popf; " CALL_NOSPEC
-	    : "=a"(rc), ASM_CALL_CONSTRAINT : [thunk_target]"r"(fop), [flags]"r"(flags));
+	    : "=a" (rc)
+	    : [thunk_target] "r" (fop), [flags] "r" (flags)
+	      COMMA(ASM_CALL_CONSTRAINT));
 	return rc;
 }
 
@@ -5079,9 +5081,10 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop)
 		fop += __ffs(ctxt->dst.bytes) * FASTOP_SIZE;
 
 	asm("push %[flags]; popf; " CALL_NOSPEC " ; pushf; pop %[flags]\n"
-	    : "+a"(ctxt->dst.val), "+d"(ctxt->src.val), [flags]"+D"(flags),
-	      [thunk_target]"+S"(fop), ASM_CALL_CONSTRAINT
-	    : "c"(ctxt->src2.val));
+	    : "+a" (ctxt->dst.val), "+d" (ctxt->src.val), [flags] "+D" (flags),
+	      [thunk_target] "+S" (fop)
+	    : "c" (ctxt->src2.val)
+	      COMMA(ASM_CALL_CONSTRAINT));
 
 	ctxt->eflags = (ctxt->eflags & ~EFLAGS_MASK) | (flags & EFLAGS_MASK);
 	if (!fop) /* exception is returned in fop variable */
diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h
index 96677576c836..a614addd589f 100644
--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -144,8 +144,9 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
 		     /* VMREAD faulted.  As above, except push '1' for @fault. */
 		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_ONE_REG, %[output])
 
-		     : ASM_CALL_CONSTRAINT, [output] "=&r" (value)
+		     : [output] "=&r" (value)
 		     : [field] "r" (field)
+		       COMMA(ASM_CALL_CONSTRAINT)
 		     : "cc");
 	return value;
 

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Mon, 3 Mar 2025 at 22:33, Ingo Molnar <mingo@kernel.org> wrote:
>
> So Josh forgot to Cc: lkml in this 5-patch series:

Honestly, this all seems to be complete garbage.

Yes, so Josh found a problem with the thing that has worked for years.

Then Josh did it another way AND THAT HAD EVEN MORE PROBLEMS.

So now it's trying to fix up all of *those* problems instead, making
the code uglier and more fragile.

And I'm not at all convinced that the new model works at all. It's a
random "this happens to work around it on the compiler versions I have
tested", rather than some obvious fix.

Put another way: the old code has years of testing and is
significantly simpler. The new code is new and untested and more
complicated and has already caused known new problems, never mind any
unknown ones.

It really doesn't sound like a good trade-off to me.

                   Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Tue, 4 Mar 2025 at 07:51, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Put another way: the old code has years of testing and is
> significantly simpler. The new code is new and untested and more
> complicated and has already caused known new problems, never mind any
> unknown ones.
>
> It really doesn't sound like a good trade-off to me.

Side note: it's not clear that we should need to do that
ASM_CALL_CONSTRAINT thing _at_all_ any more.

Iirc, the only reason we did it was for old versions of gcc, and we're
already in the process of switching minimum gcc versions up to past
where the whole thing is relevant at all. There's another tip bot
commit that makes the minimum gcc version be 8.1 due to the (much
MUCH) cleaner percpu section series.

And afaik, that makes all of this completely pointless.

So tell me again - why are we making the kernel code worse?

                Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Tue, Mar 04, 2025 at 08:01:58AM -1000, Linus Torvalds wrote:
> On Tue, 4 Mar 2025 at 07:51, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Put another way: the old code has years of testing and is
> > significantly simpler. The new code is new and untested and more
> > complicated and has already caused known new problems, never mind any
> > unknown ones.
> >
> > It really doesn't sound like a good trade-off to me.

I'm utterly confused, what are these new problems you're referring to?

And how specifically is this more fragile?

AFAICT, there was one known bug before the patches.  Now there are zero
known bugs.

Of course, it's entirely possible the build bots will shake out new
objtool warnings over the next weeks.  But as of now, I haven't seen
anything.

> Side note: it's not clear that we should need to do that
> ASM_CALL_CONSTRAINT thing _at_all_ any more.
> 
> Iirc, the only reason we did it was for old versions of gcc, and we're
> already in the process of switching minimum gcc versions up to past
> where the whole thing is relevant at all. There's another tip bot
> commit that makes the minimum gcc version be 8.1 due to the (much
> MUCH) cleaner percpu section series.
> 
> And afaik, that makes all of this completely pointless.

I'm excited to hear we can get rid of a lot of old GCC cruft, but this
has nothing to do with the compiler version.

It's needed for CONFIG_UNWINDER_FRAME_POINTER, for all compiler
versions.  Otherwise the callee may skip the frame pointer setup before
doing the call.

> So tell me again - why are we making the kernel code worse?

Again I don't see how this is worse, please spell it out for me...

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Tue, 4 Mar 2025 at 08:21, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> I'm utterly confused, what are these new problems you're referring to?

Random ugly code, untested, special versions for different config options.

> And how specifically is this more fragile?

Random ugly code, untested, special versions for different config options.

__builtin_frame_address() is much more complex than just the old "use
a register variable".

> AFAICT, there was one known bug before the patches.  Now there are zero
> known bugs.

Big surprise - since it hasn't been tested on very many compiler versions.

> I'm excited to hear we can get rid of a lot of old GCC cruft, but this
> has nothing to do with the compiler version.
>
> It's needed for CONFIG_UNWINDER_FRAME_POINTER, for all compiler
> versions.  Otherwise the callee may skip the frame pointer setup before
> doing the call.

So you claim. But the original ASM_CALL_CONSTRAINT code was for a gcc
bug that was reportedly fixed in gcc-7.1

So is it *actually* required?

Because in my testing, gcc doesn't move inline asms to before the
frame pointer setup any more.

But I actually didn't base my arguments on my very limited testing,
but on our own documented history of this thing.

In your own words from 8 years go in commit f5caf621ee35 ("x86/asm:
Fix inline asm call constraints for Clang"), just having the register
variable makes the problem go away:

    With GCC 7.2, however, GCC's behavior has changed.  It now changes its
    behavior based on the conversion of the register variable to a global.
    That somehow convinces it to *always* set up the frame pointer before
    inserting *any* inline asm.  (Therefore, listing the variable as an
    output constraint is a no-op and is no longer necessary.)

and the whole ASM_CALL_CONSTRAINT thing is just unnecessary.

So I repeat: why are we making the code worse?

               Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Tue, 4 Mar 2025 at 08:48, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Random ugly code, untested, special versions for different config options.
>
> __builtin_frame_address() is much more complex than just the old "use
> a register variable".

On the gcc bugzilla that hpa opened, I also note that Pinski said that
the __builtin_frame_address() is likely to just work by accident.

Exactly like the %rsp case.

I'd be much more inclined to look for whether marking the asm
'volatile' would be a more reliable model. Or adding a memory clobber
or similar.

Those kinds of solutions would also hopefully not need different
sequences for different config options. Because
__builtin_frame_address() really *is* fundamentally fragile, and the
fact that frame pointers change behavior is a pretty big symptom of
that fragility.

             Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Tue, Mar 04, 2025 at 08:57:13AM -1000, Linus Torvalds wrote:
> On Tue, 4 Mar 2025 at 08:48, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Random ugly code, untested, special versions for different config options.
> >
> > __builtin_frame_address() is much more complex than just the old "use
> > a register variable".
> 
> On the gcc bugzilla that hpa opened, I also note that Pinski said that
> the __builtin_frame_address() is likely to just work by accident.
> 
> Exactly like the %rsp case.

Right, so they're equally horrible in that sense.

> I'd be much more inclined to look for whether marking the asm
> 'volatile' would be a more reliable model. Or adding a memory clobber
> or similar.

Believe me, I've tried those and they don't work.

> Those kinds of solutions would also hopefully not need different
> sequences for different config options. Because
> __builtin_frame_address() really *is* fundamentally fragile, and the
> fact that frame pointers change behavior is a pretty big symptom of
> that fragility.

While that may be theoretically true, the reality is that it produces
better code for Clang.

If the main argument is that it needs more testing, then sure, let's go
test more compiler versions.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Tue, Mar 04, 2025 at 11:56:27AM -0800, Josh Poimboeuf wrote:
> On Tue, Mar 04, 2025 at 08:57:13AM -1000, Linus Torvalds wrote:
> > On Tue, 4 Mar 2025 at 08:48, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > Random ugly code, untested, special versions for different config options.
> > >
> > > __builtin_frame_address() is much more complex than just the old "use
> > > a register variable".
> > 
> > On the gcc bugzilla that hpa opened, I also note that Pinski said that
> > the __builtin_frame_address() is likely to just work by accident.
> > 
> > Exactly like the %rsp case.
> 
> Right, so they're equally horrible in that sense.
> 
> > I'd be much more inclined to look for whether marking the asm
> > 'volatile' would be a more reliable model. Or adding a memory clobber
> > or similar.
> 
> Believe me, I've tried those and they don't work.
> 
> > Those kinds of solutions would also hopefully not need different
> > sequences for different config options. Because
> > __builtin_frame_address() really *is* fundamentally fragile, and the
> > fact that frame pointers change behavior is a pretty big symptom of
> > that fragility.
> 
> While that may be theoretically true, the reality is that it produces
> better code for Clang.

BTW.  I confirmed that even the current version of ASM_CALL_CONSTRAINT
affects code generation for non-frame-pointer builds.

No matter what the ASM_CALL_CONSTRAINT implementation looks like, we
really don't want it affecting code generation for the ORC case.

So making ASM_CALL_CONSTRAINT empty for CONFIG_UNWINDER_ORC is a good
thing for code generation, regardless of the ASM_CALL_CONSTRAINT
implementation.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Tue, 4 Mar 2025 at 09:56, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> While that may be theoretically true, the reality is that it produces
> better code for Clang.

Does clang even need it? Last we did any changes for clang, it wasn't
because clang needed the marker at all, it was because clang was
unhappy with the stack pointer register define being local.

                Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Tue, 4 Mar 2025 at 10:13, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 4 Mar 2025 at 09:56, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > While that may be theoretically true, the reality is that it produces
> > better code for Clang.
>
> Does clang even need it? Last we did any changes for clang, it wasn't
> because clang needed the marker at all, it was because clang was
> unhappy with the stack pointer register define being local.

Put another way: if we make this conditional, it would make a whole
lot more sense to make it conditional on the *compiler*, not on some
random kernel config option.

At least making some "use this to mark inline asms" be
compiler-specific makes sense. We already do exactly that for other
compiler issues (we used to have the "asm goto output" gcc bugs that
way, and we still do asm_inline that way)

And as far as I know, we've only ever needed this for gcc, and gcc has
never had any problem with just using %rsp as the input - whether as a
local variable or as a global one.

But regardless, changing from one very tested model to another, when a
gcc person already has said that the new model isn't reliable, and
doing it for gcc because of a *clang* issue, really seems all kinds of
insane.

                 Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Linus Torvalds]

On Tue, 4 Mar 2025 at 10:13, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Does clang even need it?

Bah. Yes it does. I may not have clang sources to try to look at, but
I can do a test-build.

Anyway, I do think it would be better to make this compiler-specific,
and keep gcc using the old tested case that works well regardless of
whether frame pointers are enabled or not, since it doesn't _care_.

And I think there's a better way to deal with the whole "generate
better code when not needed" too, if clang really has to have that
disgusting __builtin_frame_pointer() thing that then has problems when
frame pointers aren't enabled.

IOW, you could do something pointless like

   extern int unused_variable;
  #define ASM_CALL_CONSTRAINT "+m" (unused_variable)

which generates a dependency that doesn't matter, and then doesn't
then require preprocessor hacks for when it is empty.

So I *think* the patch could be something like

 - move the define to <asm/compiler-xyzzy,.h>

 - for gcc, use the old tested code

 - for clang, use the "either __builtin_frame_pointer(0) or dummy
dependency" thing

 - have big comments about it, because our historical changelogs
clearly are not accurate wrt this all.

                 Linus

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Tue, Mar 04, 2025 at 10:41:08AM -1000, Linus Torvalds wrote:
> On Tue, 4 Mar 2025 at 10:13, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Does clang even need it?
> 
> Bah. Yes it does. I may not have clang sources to try to look at, but
> I can do a test-build.
> 
> Anyway, I do think it would be better to make this compiler-specific,
> and keep gcc using the old tested case that works well regardless of
> whether frame pointers are enabled or not, since it doesn't _care_.

Problem is, the old ASM_CALL_CONSTRAINT is an output constraint, but the
new one with __builtin_frame_address() is an *input* constraint.  How
would you combine those into a single macro?

> And I think there's a better way to deal with the whole "generate
> better code when not needed" too, if clang really has to have that
> disgusting __builtin_frame_pointer() thing that then has problems when
> frame pointers aren't enabled.

But as I've mentioned, even the old ASM_CALL_CONSTRAINT macro affects
code generation for the non-FP case.  For both compilers.  Not sure why.

We don't want to punish the default ORC config -- (hopefully) used in
the vast majority of configs at this point -- for frame pointer sins.

> IOW, you could do something pointless like
> 
>    extern int unused_variable;
>   #define ASM_CALL_CONSTRAINT "+m" (unused_variable)
> 
> which generates a dependency that doesn't matter, and then doesn't
> then require preprocessor hacks for when it is empty.

... assuming that DTRT and doesn't trigger any side effects, for all
compiler versions, now and forever?


BTW, I'm not opposed to just forgetting all this mess and stripping out
frame pointers altogether.  They have no benefit compared to ORC.  And
objtool is pretty much mandatory at this point.

Then we'd no longer have to worry about which awful hack does the least
amount of harm for each compiler and all its supported versions.

And for the eventual red zone enablement, we can enter the light of
supported behavior with that "redzone" clobber, and use objtool to
make sure that gets used.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Tue, Mar 04, 2025 at 08:48:29AM -1000, Linus Torvalds wrote:
> In your own words from 8 years go in commit f5caf621ee35 ("x86/asm:
> Fix inline asm call constraints for Clang"), just having the register
> variable makes the problem go away:
> 
>     With GCC 7.2, however, GCC's behavior has changed.  It now changes its
>     behavior based on the conversion of the register variable to a global.
>     That somehow convinces it to *always* set up the frame pointer before
>     inserting *any* inline asm.  (Therefore, listing the variable as an
>     output constraint is a no-op and is no longer necessary.)
> 
> and the whole ASM_CALL_CONSTRAINT thing is just unnecessary.

I don't know if that GCC 7.2 thing from eight years ago was a fluke or
what, but without ASM_CALL_CONSTRAINT, those "call without frame pointer
save/setup" warnings are still very much active with recent compilers.

Below is what I get with empty ASM_CALL_CONSTRAINT + GCC 14 + defconfig
+ CONFIG_UNWINDER_FRAME_POINTER.

vmlinux.o: warning: objtool: .altinstr_replacement+0x569: call without frame pointer save/setup
vmlinux.o: warning: objtool: cyc2ns_read_end+0x12: call without frame pointer save/setup
vmlinux.o: warning: objtool: native_sched_clock_from_tsc+0x66: call without frame pointer save/setup
vmlinux.o: warning: objtool: kernel_fpu_end+0x26: call without frame pointer save/setup
vmlinux.o: warning: objtool: set_tls_desc+0x170: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid.isra.0+0x8c: call without frame pointer save/setup
vmlinux.o: warning: objtool: __ioremap_collect_map_flags+0xf4: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_modify_allowed+0x13a: call without frame pointer save/setup
vmlinux.o: warning: objtool: __virt_addr_valid+0x112: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0xd22: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0xd58: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0xd62: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0xd71: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0xd7b: call without frame pointer save/setup
vmlinux.o: warning: objtool: migrate_disable+0x57: call without frame pointer save/setup
vmlinux.o: warning: objtool: down_read_trylock+0x55: call without frame pointer save/setup
vmlinux.o: warning: objtool: down_write_trylock+0x38: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0xbb: call without frame pointer save/setup
vmlinux.o: warning: objtool: class_preempt_notrace_destructor.isra.0+0x13: call without frame pointer save/setup
vmlinux.o: warning: objtool: rcu_is_watching+0x2d: call without frame pointer save/setup
vmlinux.o: warning: objtool: __is_module_percpu_address+0xc4: call without frame pointer save/setup
vmlinux.o: warning: objtool: get_compat_sigevent+0x41: call without frame pointer save/setup
vmlinux.o: warning: objtool: kprobe_busy_end+0x1e: call without frame pointer save/setup
vmlinux.o: warning: objtool: ring_buffer_discard_commit+0x22a: call without frame pointer save/setup
vmlinux.o: warning: objtool: ring_buffer_nest_end+0x27: call without frame pointer save/setup
vmlinux.o: warning: objtool: saved_cmdlines_stop+0x19: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x124c: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0xbb: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0xbb: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0xbb: call without frame pointer save/setup
vmlinux.o: warning: objtool: __pageblock_pfn_to_page+0x1cb: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x1722: call without frame pointer save/setup
vmlinux.o: warning: objtool: fput+0xe6: call without frame pointer save/setup
vmlinux.o: warning: objtool: __fput_sync+0x57: call without frame pointer save/setup
vmlinux.o: warning: objtool: __getname_maybe_null+0x7: call without frame pointer save/setup
vmlinux.o: warning: objtool: __d_rehash+0x7c: call without frame pointer save/setup
vmlinux.o: warning: objtool: ___d_drop+0x84: call without frame pointer save/setup
vmlinux.o: warning: objtool: __d_lookup_unhash+0xde: call without frame pointer save/setup
vmlinux.o: warning: objtool: get_next_ino+0x3f: call without frame pointer save/setup
vmlinux.o: warning: objtool: mnt_get_write_access+0x68: call without frame pointer save/setup
vmlinux.o: warning: objtool: mnt_put_write_access+0x21: call without frame pointer save/setup
vmlinux.o: warning: objtool: mnt_put_write_access_file+0x2b: call without frame pointer save/setup
vmlinux.o: warning: objtool: mb_cache_entry_get+0x9d: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0xbb: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0xbb: call without frame pointer save/setup
vmlinux.o: warning: objtool: jbd2_journal_grab_journal_head+0x5c: call without frame pointer save/setup
vmlinux.o: warning: objtool: class_preempt_notrace_destructor.isra.0+0x13: call without frame pointer save/setup
vmlinux.o: warning: objtool: autofs_expire_multi+0xf: call without frame pointer save/setup
vmlinux.o: warning: objtool: ksys_msgsnd+0xa: call without frame pointer save/setup
vmlinux.o: warning: objtool: __x64_sys_msgsnd+0x16: call without frame pointer save/setup
vmlinux.o: warning: objtool: __ia32_sys_msgsnd+0x15: call without frame pointer save/setup
vmlinux.o: warning: objtool: compat_ksys_msgsnd+0xf: call without frame pointer save/setup
vmlinux.o: warning: objtool: __ia32_compat_sys_msgsnd+0x16: call without frame pointer save/setup
vmlinux.o: warning: objtool: __x64_sys_lsm_list_modules+0x21: call without frame pointer save/setup
vmlinux.o: warning: objtool: __ia32_sys_lsm_list_modules+0x20: call without frame pointer save/setup
vmlinux.o: warning: objtool: blk_account_io_merge_bio.part.0+0x6c: call without frame pointer save/setup
vmlinux.o: warning: objtool: blk_account_io_completion.part.0+0x5a: call without frame pointer save/setup
vmlinux.o: warning: objtool: iocg_commit_bio+0x30: call without frame pointer save/setup
vmlinux.o: warning: objtool: class_preempt_notrace_destructor.isra.0+0x13: call without frame pointer save/setup
vmlinux.o: warning: objtool: sg_miter_stop+0x6d: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x1c6b: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x1c82: call without frame pointer save/setup
vmlinux.o: warning: objtool: write_port+0x6f: call without frame pointer save/setup
vmlinux.o: warning: objtool: drm_clflush_page+0x67: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x1fae: call without frame pointer save/setup
vmlinux.o: warning: objtool: check_relocations+0x62: call without frame pointer save/setup
vmlinux.o: warning: objtool: scsi_kunmap_atomic_sg+0x21: call without frame pointer save/setup
vmlinux.o: warning: objtool: serport_ldisc_compat_ioctl+0xe: call without frame pointer save/setup
vmlinux.o: warning: objtool: serport_ldisc_ioctl+0xf: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x2286: call without frame pointer save/setup
vmlinux.o: warning: objtool: cpuidle_use_deepest_state+0x2a: call without frame pointer save/setup
vmlinux.o: warning: objtool: cpuidle_get_driver+0x20: call without frame pointer save/setup
vmlinux.o: warning: objtool: skb_abort_seq_read+0x28: call without frame pointer save/setup
vmlinux.o: warning: objtool: skb_ts_finish+0x28: call without frame pointer save/setup
vmlinux.o: warning: objtool: skb_seq_read+0x24e: call without frame pointer save/setup
vmlinux.o: warning: objtool: xdr_terminate_string+0x55: call without frame pointer save/setup
vmlinux.o: warning: objtool: class_preempt_notrace_destructor.isra.0+0x13: call without frame pointer save/setup
vmlinux.o: warning: objtool: class_preempt_notrace_destructor.isra.0+0x13: call without frame pointer save/setup
vmlinux.o: warning: objtool: find_bug+0xad: call without frame pointer save/setup
vmlinux.o: warning: objtool: generic_bug_clear_once+0x96: call without frame pointer save/setup
vmlinux.o: warning: objtool: delay_tsc+0x87: call without frame pointer save/setup
vmlinux.o: warning: objtool: .altinstr_replacement+0x5f6: call without frame pointer save/setup
vmlinux.o: warning: objtool: pfn_valid+0x81: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_spin_trylock+0x36: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_write_unlock+0x15: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_write_unlock_irq+0x16: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_write_unlock_irqrestore+0x1e: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_read_trylock+0x45: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_write_trylock+0x36: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_read_unlock+0x1b: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_read_unlock_irq+0x1c: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_read_unlock_irqrestore+0x24: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_spin_unlock+0x15: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_spin_unlock_irq+0x16: call without frame pointer save/setup
vmlinux.o: warning: objtool: _raw_spin_unlock_irqrestore+0x1e: call without frame pointer save/setup

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Tue, Mar 04, 2025 at 11:47:52AM -0800, Josh Poimboeuf wrote:
> On Tue, Mar 04, 2025 at 08:48:29AM -1000, Linus Torvalds wrote:
> > In your own words from 8 years go in commit f5caf621ee35 ("x86/asm:
> > Fix inline asm call constraints for Clang"), just having the register
> > variable makes the problem go away:
> > 
> >     With GCC 7.2, however, GCC's behavior has changed.  It now changes its
> >     behavior based on the conversion of the register variable to a global.
> >     That somehow convinces it to *always* set up the frame pointer before
> >     inserting *any* inline asm.  (Therefore, listing the variable as an
> >     output constraint is a no-op and is no longer necessary.)
> > 
> > and the whole ASM_CALL_CONSTRAINT thing is just unnecessary.
> 
> I don't know if that GCC 7.2 thing from eight years ago was a fluke or
> what, but without ASM_CALL_CONSTRAINT, those "call without frame pointer
> save/setup" warnings are still very much active with recent compilers.
> 
> Below is what I get with empty ASM_CALL_CONSTRAINT + GCC 14 + defconfig
> + CONFIG_UNWINDER_FRAME_POINTER.

And to clarify, yes, those still have the global register variable
defined.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@kernel.org> wrote:

> On Tue, Mar 04, 2025 at 08:01:58AM -1000, Linus Torvalds wrote:
> > On Tue, 4 Mar 2025 at 07:51, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > Put another way: the old code has years of testing and is
> > > significantly simpler. The new code is new and untested and more
> > > complicated and has already caused known new problems, never mind any
> > > unknown ones.
> > >
> > > It really doesn't sound like a good trade-off to me.
> 
> I'm utterly confused, what are these new problems you're referring to?
> 
> And how specifically is this more fragile?
> 
> AFAICT, there was one known bug before the patches.  Now there are zero
> known bugs.
> 
> Of course, it's entirely possible the build bots will shake out new
> objtool warnings over the next weeks.  But as of now, I haven't seen
> anything.

In any case I've zapped these two commits from tip:x86/asm for the time 
being:

  x86/asm: Fix ASM_CALL_CONSTRAINT for Clang 19 + KCOV + KMSAN
  x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

Until there's consensus.

I left the 3 preparatory patches, which make sense as standalone 
cleanups:

  KVM: VMX: Use named operands in inline asm
  x86/hyperv: Use named operands in inline asm
  x86/alternatives: Simplify alternative_call() interface

Thanks,

	Ingo

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 3, 2025 3:02:41 AM PST, tip-bot2 for Josh Poimboeuf <tip-bot2@linutronix.de> wrote:
>The following commit has been merged into the x86/asm branch of tip:
>
>Commit-ID:     e5ff90b179d45df71373cf79f99d20c9abe229cb
>Gitweb:        https://git.kernel.org/tip/e5ff90b179d45df71373cf79f99d20c9abe229cb
>Author:        Josh Poimboeuf <jpoimboe@kernel.org>
>AuthorDate:    Sun, 02 Mar 2025 17:21:03 -08:00
>Committer:     Ingo Molnar <mingo@kernel.org>
>CommitterDate: Mon, 03 Mar 2025 11:39:54 +01:00
>
>x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers
>
>With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
>asm statement with a call instruction to force the compiler to set up
>the frame pointer before doing the call.
>
>Without frame pointers, no such constraint is needed.  Make it
>conditional on frame pointers.
>
>Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
>Signed-off-by: Ingo Molnar <mingo@kernel.org>
>Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>Cc: Linus Torvalds <torvalds@linux-foundation.org>
>Cc: Brian Gerst <brgerst@gmail.com>
>Cc: H. Peter Anvin <hpa@zytor.com>
>Cc: linux-kernel@vger.kernel.org
>---
> arch/x86/include/asm/asm.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>index 0d268e6..f1db9e8 100644
>--- a/arch/x86/include/asm/asm.h
>+++ b/arch/x86/include/asm/asm.h
>@@ -232,7 +232,11 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
>  * gets set up by the containing function.  If you forget to do this, objtool
>  * may print a "call without frame pointer save/setup" warning.
>  */
>+#ifdef CONFIG_UNWINDER_FRAME_POINTER
> #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>+#else
>+#define ASM_CALL_CONSTRAINT
>+#endif
> 
> #endif /* __ASSEMBLY__ */
> 

Wait, why was this changed? I actually tested this form at least once and found that it didn't work under all circumstances...

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
> >+#ifdef CONFIG_UNWINDER_FRAME_POINTER
> > #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
> >+#else
> >+#define ASM_CALL_CONSTRAINT
> >+#endif
> > 
> > #endif /* __ASSEMBLY__ */
> > 
> 
> Wait, why was this changed? I actually tested this form at least once
> and found that it didn't work under all circumstances...

Do you have any more details about where this didn't work?  I tested
with several configs and it seems to work fine.  Objtool will complain
if it doesn't work.

See here for the justification (the previous version was producing crap
code in Clang):

  https://lore.kernel.org/dbea2ae2fb39bece21013f939ddeb15507baa7d3.1740964309.git.jpoimboe@kernel.org

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Mon, Mar 03, 2025 at 02:45:50PM -0800, Josh Poimboeuf wrote:
> On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
> > >+#ifdef CONFIG_UNWINDER_FRAME_POINTER
> > > #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
> > >+#else
> > >+#define ASM_CALL_CONSTRAINT
> > >+#endif
> > > 
> > > #endif /* __ASSEMBLY__ */
> > > 
> > 
> > Wait, why was this changed? I actually tested this form at least once
> > and found that it didn't work under all circumstances...
> 
> Do you have any more details about where this didn't work?  I tested
> with several configs and it seems to work fine.  Objtool will complain
> if it doesn't work.
> 
> See here for the justification (the previous version was producing crap
> code in Clang):

Gah, that link doesn't work because I forgot to cc lkml.

Here's the tip bot link:

  https://lore.kernel.org/all/174099976253.10177.12542657892256193630.tip-bot2@tip-bot2/

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 3, 2025 2:47:58 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>On Mon, Mar 03, 2025 at 02:45:50PM -0800, Josh Poimboeuf wrote:
>> On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
>> > >+#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> > > #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>> > >+#else
>> > >+#define ASM_CALL_CONSTRAINT
>> > >+#endif
>> > > 
>> > > #endif /* __ASSEMBLY__ */
>> > > 
>> > 
>> > Wait, why was this changed? I actually tested this form at least once
>> > and found that it didn't work under all circumstances...
>> 
>> Do you have any more details about where this didn't work?  I tested
>> with several configs and it seems to work fine.  Objtool will complain
>> if it doesn't work.
>> 
>> See here for the justification (the previous version was producing crap
>> code in Clang):
>
>Gah, that link doesn't work because I forgot to cc lkml.
>
>Here's the tip bot link:
>
>  https://lore.kernel.org/all/174099976253.10177.12542657892256193630.tip-bot2@tip-bot2/
>

One more thing: if we remove ASM_CALL_CONSTRAINTS, we will not be able to use the redzone in future FRED only kernel builds.

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Andrew Cooper]

> One more thing: if we remove ASM_CALL_CONSTRAINTS, we will not be able to use the redzone in future FRED only kernel builds.

That's easy enough to fix with "|| CONFIG_FRED_EXCLUSIVE" in some
theoretical future when it's a feasible config to use.

~Andrew

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 3, 2025 4:57:49 PM PST, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> One more thing: if we remove ASM_CALL_CONSTRAINTS, we will not be able to use the redzone in future FRED only kernel builds.
>
>That's easy enough to fix with "|| CONFIG_FRED_EXCLUSIVE" in some
>theoretical future when it's a feasible config to use.
>
>~Andrew

Assuming it hasn't bitrotted...

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Mon, Mar 03, 2025 at 06:05:07PM -0800, H. Peter Anvin wrote:
> On March 3, 2025 4:57:49 PM PST, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> One more thing: if we remove ASM_CALL_CONSTRAINTS, we will not be able to use the redzone in future FRED only kernel builds.
> >
> >That's easy enough to fix with "|| CONFIG_FRED_EXCLUSIVE" in some
> >theoretical future when it's a feasible config to use.
> >
> >~Andrew
> 
> Assuming it hasn't bitrotted...

If we start using red zone, I'm thinking it should be fairly easy to add
"red zone stack validation" to objtool.  Then the build bots should be
able to shake out pretty quickly where ASM_CALL_CONSTRAINT is needed.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Uros Bizjak]

On 4. 03. 25 01:35, H. Peter Anvin wrote:
> On March 3, 2025 2:47:58 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>> On Mon, Mar 03, 2025 at 02:45:50PM -0800, Josh Poimboeuf wrote:
>>> On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
>>>>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>>>>> #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>>>>> +#else
>>>>> +#define ASM_CALL_CONSTRAINT
>>>>> +#endif
>>>>>
>>>>> #endif /* __ASSEMBLY__ */
>>>>>
>>>>
>>>> Wait, why was this changed? I actually tested this form at least once
>>>> and found that it didn't work under all circumstances...
>>>
>>> Do you have any more details about where this didn't work?  I tested
>>> with several configs and it seems to work fine.  Objtool will complain
>>> if it doesn't work.
>>>
>>> See here for the justification (the previous version was producing crap
>>> code in Clang):
>>
>> Gah, that link doesn't work because I forgot to cc lkml.
>>
>> Here's the tip bot link:
>>
>>   https://lore.kernel.org/all/174099976253.10177.12542657892256193630.tip-bot2@tip-bot2/
>>
> 
> One more thing: if we remove ASM_CALL_CONSTRAINTS, we will not be able to use the redzone in future FRED only kernel builds.

Actually, GCC 15+ will introduce "redzone" clobber, so you will be able 
to write e.g.:

void foo (void) { asm ("" : : : "cc", "memory", "redzone"); }

Please see [1] and:

+@item "redzone"
+The @code{"redzone"} clobber tells the compiler that the assembly code
+may write to the stack red zone, area below the stack pointer which on
+some architectures in some calling conventions is guaranteed not to be
+changed by signal handlers, interrupts or exceptions and so the compiler
+can store there temporaries in leaf functions.  On targets which have
+no concept of the stack red zone, the clobber is ignored.
+It should be used e.g.@: in case the assembly code uses call instructions
+or pushes something to the stack without taking the red zone into account
+by subtracting red zone size from the stack pointer first and restoring
+it afterwards.
+

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117312

Uros.

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 4, 2025 7:35:30 AM PST, Uros Bizjak <ubizjak@gmail.com> wrote:
>
>
>On 4. 03. 25 01:35, H. Peter Anvin wrote:
>> On March 3, 2025 2:47:58 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>>> On Mon, Mar 03, 2025 at 02:45:50PM -0800, Josh Poimboeuf wrote:
>>>> On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
>>>>>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>>>>>> #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>>>>>> +#else
>>>>>> +#define ASM_CALL_CONSTRAINT
>>>>>> +#endif
>>>>>> 
>>>>>> #endif /* __ASSEMBLY__ */
>>>>>> 
>>>>> 
>>>>> Wait, why was this changed? I actually tested this form at least once
>>>>> and found that it didn't work under all circumstances...
>>>> 
>>>> Do you have any more details about where this didn't work?  I tested
>>>> with several configs and it seems to work fine.  Objtool will complain
>>>> if it doesn't work.
>>>> 
>>>> See here for the justification (the previous version was producing crap
>>>> code in Clang):
>>> 
>>> Gah, that link doesn't work because I forgot to cc lkml.
>>> 
>>> Here's the tip bot link:
>>> 
>>>   https://lore.kernel.org/all/174099976253.10177.12542657892256193630.tip-bot2@tip-bot2/
>>> 
>> 
>> One more thing: if we remove ASM_CALL_CONSTRAINTS, we will not be able to use the redzone in future FRED only kernel builds.
>
>Actually, GCC 15+ will introduce "redzone" clobber, so you will be able to write e.g.:
>
>void foo (void) { asm ("" : : : "cc", "memory", "redzone"); }
>
>Please see [1] and:
>
>+@item "redzone"
>+The @code{"redzone"} clobber tells the compiler that the assembly code
>+may write to the stack red zone, area below the stack pointer which on
>+some architectures in some calling conventions is guaranteed not to be
>+changed by signal handlers, interrupts or exceptions and so the compiler
>+can store there temporaries in leaf functions.  On targets which have
>+no concept of the stack red zone, the clobber is ignored.
>+It should be used e.g.@: in case the assembly code uses call instructions
>+or pushes something to the stack without taking the red zone into account
>+by subtracting red zone size from the stack pointer first and restoring
>+it afterwards.
>+
>
>[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117312
>
>Uros.

Very nice :)

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 3, 2025 2:45:48 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
>> >+#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> > #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>> >+#else
>> >+#define ASM_CALL_CONSTRAINT
>> >+#endif
>> > 
>> > #endif /* __ASSEMBLY__ */
>> > 
>> 
>> Wait, why was this changed? I actually tested this form at least once
>> and found that it didn't work under all circumstances...
>
>Do you have any more details about where this didn't work?  I tested
>with several configs and it seems to work fine.  Objtool will complain
>if it doesn't work.
>
>See here for the justification (the previous version was producing crap
>code in Clang):
>
>  https://lore.kernel.org/dbea2ae2fb39bece21013f939ddeb15507baa7d3.1740964309.git.jpoimboe@kernel.org
>

I need to dig it up, but I had a discussion about this with some gcc people about a year ago.

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Uros Bizjak]

On 4. 03. 25 00:59, H. Peter Anvin wrote:
> On March 3, 2025 2:45:48 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>> On Mon, Mar 03, 2025 at 02:31:50PM -0800, H. Peter Anvin wrote:
>>>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>>>> #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>>>> +#else
>>>> +#define ASM_CALL_CONSTRAINT
>>>> +#endif
>>>>
>>>> #endif /* __ASSEMBLY__ */
>>>>
>>>
>>> Wait, why was this changed? I actually tested this form at least once
>>> and found that it didn't work under all circumstances...
>>
>> Do you have any more details about where this didn't work?  I tested
>> with several configs and it seems to work fine.  Objtool will complain
>> if it doesn't work.
>>
>> See here for the justification (the previous version was producing crap
>> code in Clang):
>>
>>   https://lore.kernel.org/dbea2ae2fb39bece21013f939ddeb15507baa7d3.1740964309.git.jpoimboe@kernel.org
>>
> 
> I need to dig it up, but I had a discussion about this with some gcc people about a year ago.

It was discussed here [1].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117311

Uros.

[tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[tip-bot2 for Josh Poimboeuf]

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

Commit-ID:     05844663b4fcf22bb3a1494615ae3f25852c9abc
Gitweb:        https://git.kernel.org/tip/05844663b4fcf22bb3a1494615ae3f25852c9abc
Author:        Josh Poimboeuf <jpoimboe@kernel.org>
AuthorDate:    Sun, 02 Mar 2025 17:21:03 -08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 04 Mar 2025 11:21:40 +01:00

x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
asm statement with a call instruction to force the compiler to set up
the frame pointer before doing the call.

Without frame pointers, no such constraint is needed.  Make it
conditional on frame pointers.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/asm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 0d268e6..f1db9e8 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -232,7 +232,11 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
  * gets set up by the containing function.  If you forget to do this, objtool
  * may print a "call without frame pointer save/setup" warning.
  */
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
 #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
+#else
+#define ASM_CALL_CONSTRAINT
+#endif
 
 #endif /* __ASSEMBLY__ */
 

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 4, 2025 2:36:24 AM PST, tip-bot2 for Josh Poimboeuf <tip-bot2@linutronix.de> wrote:
>The following commit has been merged into the x86/asm branch of tip:
>
>Commit-ID:     05844663b4fcf22bb3a1494615ae3f25852c9abc
>Gitweb:        https://git.kernel.org/tip/05844663b4fcf22bb3a1494615ae3f25852c9abc
>Author:        Josh Poimboeuf <jpoimboe@kernel.org>
>AuthorDate:    Sun, 02 Mar 2025 17:21:03 -08:00
>Committer:     Ingo Molnar <mingo@kernel.org>
>CommitterDate: Tue, 04 Mar 2025 11:21:40 +01:00
>
>x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers
>
>With frame pointers enabled, ASM_CALL_CONSTRAINT is used in an inline
>asm statement with a call instruction to force the compiler to set up
>the frame pointer before doing the call.
>
>Without frame pointers, no such constraint is needed.  Make it
>conditional on frame pointers.
>
>Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
>Signed-off-by: Ingo Molnar <mingo@kernel.org>
>Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>Cc: Linus Torvalds <torvalds@linux-foundation.org>
>Cc: Brian Gerst <brgerst@gmail.com>
>Cc: H. Peter Anvin <hpa@zytor.com>
>Cc: linux-kernel@vger.kernel.org
>---
> arch/x86/include/asm/asm.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
>index 0d268e6..f1db9e8 100644
>--- a/arch/x86/include/asm/asm.h
>+++ b/arch/x86/include/asm/asm.h
>@@ -232,7 +232,11 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
>  * gets set up by the containing function.  If you forget to do this, objtool
>  * may print a "call without frame pointer save/setup" warning.
>  */
>+#ifdef CONFIG_UNWINDER_FRAME_POINTER
> #define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))
>+#else
>+#define ASM_CALL_CONSTRAINT
>+#endif
> 
> #endif /* __ASSEMBLY__ */
> 

So we are going to be using this version despite the gcc maintainers telling us it is not supported?

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> > #endif /* __ASSEMBLY__ */
> 
> So we are going to be using this version despite the gcc maintainers 
> telling us it is not supported?

No, neither patches are in the x86 tree at the moment.

Thanks,

	Ingo

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote:
> 
> * H. Peter Anvin <hpa@zytor.com> wrote:
> 
> > > #endif /* __ASSEMBLY__ */
> > 
> > So we are going to be using this version despite the gcc maintainers 
> > telling us it is not supported?
> 
> No, neither patches are in the x86 tree at the moment.

FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this
patch wouldn't have changed anything in that respect.

Regardless I plan to post a new patch set soon with a bunch of cleanups.

It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will
use the new __builtin_frame_address(0) input constraint for Clang only.

There will be a new asm_call() interface to hide the mess.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 7, 2025 3:21:57 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote:
>> 
>> * H. Peter Anvin <hpa@zytor.com> wrote:
>> 
>> > > #endif /* __ASSEMBLY__ */
>> > 
>> > So we are going to be using this version despite the gcc maintainers 
>> > telling us it is not supported?
>> 
>> No, neither patches are in the x86 tree at the moment.
>
>FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this
>patch wouldn't have changed anything in that respect.
>
>Regardless I plan to post a new patch set soon with a bunch of cleanups.
>
>It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will
>use the new __builtin_frame_address(0) input constraint for Clang only.
>
>There will be a new asm_call() interface to hide the mess.
>

Alternatively, you can co-opt the gcc BR I already filed on this and argue there that there are new reasons to support the alternate construct.

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Fri, Mar 07, 2025 at 03:29:00PM -0800, H. Peter Anvin wrote:
> On March 7, 2025 3:21:57 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote:
> >> 
> >> * H. Peter Anvin <hpa@zytor.com> wrote:
> >> 
> >> > > #endif /* __ASSEMBLY__ */
> >> > 
> >> > So we are going to be using this version despite the gcc maintainers 
> >> > telling us it is not supported?
> >> 
> >> No, neither patches are in the x86 tree at the moment.
> >
> >FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this
> >patch wouldn't have changed anything in that respect.
> >
> >Regardless I plan to post a new patch set soon with a bunch of cleanups.
> >
> >It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will
> >use the new __builtin_frame_address(0) input constraint for Clang only.
> >
> >There will be a new asm_call() interface to hide the mess.
> >
> 
> Alternatively, you can co-opt the gcc BR I already filed on this and
> argue there that there are new reasons to support the alternate
> construct.

We hopefully won't need those hacks much longer anyway, as I'll have
another series to propose removing frame pointers for x86-64.

x86-32 can keep frame pointers, but doesn't need the constraints.  It's
not supported for livepatch so it doesn't need to be 100% reliable.
Worst case, an unwind skips a frame, but the call address still shows up
on stack trace dumps prepended with '?'.

I plan to do the asm_call() series before the FP removal series because
it's presumably less disruptive, and it has a bunch more orthogonal
cleanups.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[David Laight]

On Fri, 7 Mar 2025 17:38:14 -0800
Josh Poimboeuf <jpoimboe@kernel.org> wrote:

...
> We hopefully won't need those hacks much longer anyway, as I'll have
> another series to propose removing frame pointers for x86-64.
> 
> x86-32 can keep frame pointers, but doesn't need the constraints.  It's
> not supported for livepatch so it doesn't need to be 100% reliable.
> Worst case, an unwind skips a frame, but the call address still shows up
> on stack trace dumps prepended with '?'.

Doesn't 'user copy hardening' also do stack following?
That needs to find all the stack frames (that have locals) and I think
is is more reliable with frame pointers.

	David

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Sat, Mar 08, 2025 at 08:15:30AM +0000, David Laight wrote:
> On Fri, 7 Mar 2025 17:38:14 -0800
> Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> 
> ...
> > We hopefully won't need those hacks much longer anyway, as I'll have
> > another series to propose removing frame pointers for x86-64.
> > 
> > x86-32 can keep frame pointers, but doesn't need the constraints.  It's
> > not supported for livepatch so it doesn't need to be 100% reliable.
> > Worst case, an unwind skips a frame, but the call address still shows up
> > on stack trace dumps prepended with '?'.
> 
> Doesn't 'user copy hardening' also do stack following?
> That needs to find all the stack frames (that have locals) and I think
> is is more reliable with frame pointers.

Yeah, that's arch_within_stack_frames(), which is frame pointer only.

ORC would actually be more reliable than frame pointers, but IIRC,
hardened usercopy didn't get an ORC implementation due to performance
concerns about doing an ORC unwind for every usercopy to/from the stack.

So yeah, hardened usercopy is one minor benefit of frame pointers vs ORC.

-- 
Josh

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[H. Peter Anvin]

On March 7, 2025 5:38:14 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>On Fri, Mar 07, 2025 at 03:29:00PM -0800, H. Peter Anvin wrote:
>> On March 7, 2025 3:21:57 PM PST, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>> >On Sat, Mar 08, 2025 at 12:05:29AM +0100, Ingo Molnar wrote:
>> >> 
>> >> * H. Peter Anvin <hpa@zytor.com> wrote:
>> >> 
>> >> > > #endif /* __ASSEMBLY__ */
>> >> > 
>> >> > So we are going to be using this version despite the gcc maintainers 
>> >> > telling us it is not supported?
>> >> 
>> >> No, neither patches are in the x86 tree at the moment.
>> >
>> >FWIW, the existing ASM_CALL_CONSTRAINT is also not supported, so this
>> >patch wouldn't have changed anything in that respect.
>> >
>> >Regardless I plan to post a new patch set soon with a bunch of cleanups.
>> >
>> >It will keep the existing ASM_CALL_CONSTRAINT in place for GCC, and will
>> >use the new __builtin_frame_address(0) input constraint for Clang only.
>> >
>> >There will be a new asm_call() interface to hide the mess.
>> >
>> 
>> Alternatively, you can co-opt the gcc BR I already filed on this and
>> argue there that there are new reasons to support the alternate
>> construct.
>
>We hopefully won't need those hacks much longer anyway, as I'll have
>another series to propose removing frame pointers for x86-64.
>
>x86-32 can keep frame pointers, but doesn't need the constraints.  It's
>not supported for livepatch so it doesn't need to be 100% reliable.
>Worst case, an unwind skips a frame, but the call address still shows up
>on stack trace dumps prepended with '?'.
>
>I plan to do the asm_call() series before the FP removal series because
>it's presumably less disruptive, and it has a bunch more orthogonal
>cleanups.
>

I should probably clarify that this wasn't flippant, but a serious request.

If this works by accident on existing gcc, and works on clang, that is a very good reason for making it the supported way of doing this going forward for both compilers. Per-compiler hacks are nasty, and although we are pretty good about coping with them in the kernel, some user space app developer is guaranteed to get it wrong. 

Frame pointers are actually more relevant in user space because user space tends to be compiled with a wider range of debug and architecture options, and of course there is simply way more user space code out there.

Re: [tip: x86/asm] x86/asm: Make ASM_CALL_CONSTRAINT conditional on frame pointers

[Josh Poimboeuf]

On Mon, Mar 10, 2025 at 07:49:56AM -0700, H. Peter Anvin wrote:
> >> Alternatively, you can co-opt the gcc BR I already filed on this and
> >> argue there that there are new reasons to support the alternate
> >> construct.
> 
> I should probably clarify that this wasn't flippant, but a serious
> request.
> 
> If this works by accident on existing gcc, and works on clang, that is
> a very good reason for making it the supported way of doing this going
> forward for both compilers. Per-compiler hacks are nasty, and although
> we are pretty good about coping with them in the kernel, some user
> space app developer is guaranteed to get it wrong. 
> 
> Frame pointers are actually more relevant in user space because user
> space tends to be compiled with a wider range of debug and
> architecture options, and of course there is simply way more user
> space code out there.

I opened a gcc bug:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119279

-- 
Josh