Commit 0d3db1f1 authored by Borislav Petkov (AMD)'s avatar Borislav Petkov (AMD)

x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer

objtool complains:

  arch/x86/kvm/kvm.o: warning: objtool: .altinstr_replacement+0xc5: call without frame pointer save/setup
  vmlinux.o: warning: objtool: .altinstr_replacement+0x2eb: call without frame pointer save/setup

Make sure %rSP is an output operand to the respective asm() statements.

The test_cc() hunk and ALT_OUTPUT_SP() courtesy of peterz. Also from him
add some helpful debugging info to the documentation.

Now on to the explanations:

tl;dr: The alternatives macros are pretty fragile.

If I do ALT_OUTPUT_SP(output) in order to be able to package in a %rsp
reference for objtool so that a stack frame gets properly generated, the
inline asm input operand with positional argument 0 in clear_page():

	"0" (page)

gets "renumbered" due to the added

	: "+r" (current_stack_pointer), "=D" (page)

and then gcc says:

  ./arch/x86/include/asm/page_64.h:53:9: error: inconsistent operand constraints in an ‘asm’

The fix is to use an explicit "D" constraint which points to a singleton
register class (gcc terminology) which ends up doing what is expected
here: the page pointer - input and output - should be in the same %rdi
register.

Other register classes have more than one register in them - example:
"r" and "=r" or "A":

  ‘A’
	The ‘a’ and ‘d’ registers.  This class is used for
	instructions that return double word results in the ‘ax:dx’
	register pair.  Single word values will be allocated either in
	‘ax’ or ‘dx’.

so using "D" and "=D" just works in this particular case.

And yes, one would say, sure, why don't you do "+D" but then:

  : "+r" (current_stack_pointer), "+D" (page)
  : [old] "i" (clear_page_orig), [new1] "i" (clear_page_rep), [new2] "i" (clear_page_erms),
  : "cc", "memory", "rax", "rcx")

now find the Waldo^Wcomma which throws a wrench into all this.

Because that silly macro has an "input..." consume-all last macro arg
and in it, one is supposed to supply input *and* clobbers, leading to
silly syntax snafus.

Yap, they need to be cleaned up, one fine day...

Closes: https://lore.kernel.org/oe-kbuild-all/202406141648.jO9qNGLa-lkp@intel.com/Reported-by: default avatarkernel test robot <lkp@intel.com>
Signed-off-by: default avatarBorislav Petkov (AMD) <bp@alien8.de>
Acked-by: default avatarSean Christopherson <seanjc@google.com>
Acked-by: default avatarPeter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20240625112056.GDZnqoGDXgYuWBDUwu@fat_crate.local
parent f776e41f
...@@ -246,9 +246,10 @@ static inline int alternatives_text_reserved(void *start, void *end) ...@@ -246,9 +246,10 @@ static inline int alternatives_text_reserved(void *start, void *end)
* references: i.e., if used for a function, it would add the PLT * references: i.e., if used for a function, it would add the PLT
* suffix. * suffix.
*/ */
#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ #define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \
asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \
: output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) : ALT_OUTPUT_SP(output) \
: [old] "i" (oldfunc), [new] "i" (newfunc), ## input)
/* /*
* Like alternative_call, but there are two features and respective functions. * Like alternative_call, but there are two features and respective functions.
...@@ -260,7 +261,7 @@ static inline int alternatives_text_reserved(void *start, void *end) ...@@ -260,7 +261,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
output, input...) \ output, input...) \
asm_inline volatile(ALTERNATIVE_2("call %c[old]", "call %c[new1]", ft_flags1, \ asm_inline volatile(ALTERNATIVE_2("call %c[old]", "call %c[new1]", ft_flags1, \
"call %c[new2]", ft_flags2) \ "call %c[new2]", ft_flags2) \
: output, ASM_CALL_CONSTRAINT \ : ALT_OUTPUT_SP(output) \
: [old] "i" (oldfunc), [new1] "i" (newfunc1), \ : [old] "i" (oldfunc), [new1] "i" (newfunc1), \
[new2] "i" (newfunc2), ## input) [new2] "i" (newfunc2), ## input)
...@@ -276,6 +277,8 @@ static inline int alternatives_text_reserved(void *start, void *end) ...@@ -276,6 +277,8 @@ static inline int alternatives_text_reserved(void *start, void *end)
*/ */
#define ASM_NO_INPUT_CLOBBER(clbr...) "i" (0) : clbr #define ASM_NO_INPUT_CLOBBER(clbr...) "i" (0) : clbr
#define ALT_OUTPUT_SP(...) ASM_CALL_CONSTRAINT, ## __VA_ARGS__
/* Macro for creating assembler functions avoiding any C magic. */ /* Macro for creating assembler functions avoiding any C magic. */
#define DEFINE_ASM_FUNC(func, instr, sec) \ #define DEFINE_ASM_FUNC(func, instr, sec) \
asm (".pushsection " #sec ", \"ax\"\n" \ asm (".pushsection " #sec ", \"ax\"\n" \
......
...@@ -54,7 +54,7 @@ static inline void clear_page(void *page) ...@@ -54,7 +54,7 @@ static inline void clear_page(void *page)
clear_page_rep, X86_FEATURE_REP_GOOD, clear_page_rep, X86_FEATURE_REP_GOOD,
clear_page_erms, X86_FEATURE_ERMS, clear_page_erms, X86_FEATURE_ERMS,
"=D" (page), "=D" (page),
"0" (page) "D" (page)
: "cc", "memory", "rax", "rcx"); : "cc", "memory", "rax", "rcx");
} }
......
...@@ -1657,7 +1657,7 @@ static noinline void __init alt_reloc_selftest(void) ...@@ -1657,7 +1657,7 @@ static noinline void __init alt_reloc_selftest(void)
*/ */
asm_inline volatile ( asm_inline volatile (
ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS) ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS)
: /* output */ : ASM_CALL_CONSTRAINT
: [mem] "m" (__alt_reloc_selftest_addr) : [mem] "m" (__alt_reloc_selftest_addr)
: _ASM_ARG1 : _ASM_ARG1
); );
......
...@@ -1069,7 +1069,7 @@ static __always_inline u8 test_cc(unsigned int condition, unsigned long flags) ...@@ -1069,7 +1069,7 @@ static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF; flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
asm("push %[flags]; popf; " CALL_NOSPEC asm("push %[flags]; popf; " CALL_NOSPEC
: "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags)); : "=a"(rc), ASM_CALL_CONSTRAINT : [thunk_target]"r"(fop), [flags]"r"(flags));
return rc; return rc;
} }
......
...@@ -284,6 +284,25 @@ the objtool maintainers. ...@@ -284,6 +284,25 @@ the objtool maintainers.
Otherwise the stack frame may not get created before the call. Otherwise the stack frame may not get created before the call.
objtool can help with pinpointing the exact function where it happens:
$ OBJTOOL_ARGS="--verbose" make arch/x86/kvm/
arch/x86/kvm/kvm.o: warning: objtool: .altinstr_replacement+0xc5: call without frame pointer save/setup
arch/x86/kvm/kvm.o: warning: objtool: em_loop.part.0+0x29: (alt)
arch/x86/kvm/kvm.o: warning: objtool: em_loop.part.0+0x0: <=== (sym)
LD [M] arch/x86/kvm/kvm-intel.o
0000 0000000000028220 <em_loop.part.0>:
0000 28220: 0f b6 47 61 movzbl 0x61(%rdi),%eax
0004 28224: 3c e2 cmp $0xe2,%al
0006 28226: 74 2c je 28254 <em_loop.part.0+0x34>
0008 28228: 48 8b 57 10 mov 0x10(%rdi),%rdx
000c 2822c: 83 f0 05 xor $0x5,%eax
000f 2822f: 48 c1 e0 04 shl $0x4,%rax
0013 28233: 25 f0 00 00 00 and $0xf0,%eax
0018 28238: 81 e2 d5 08 00 00 and $0x8d5,%edx
001e 2823e: 80 ce 02 or $0x2,%dh
...
2. file.o: warning: objtool: .text+0x53: unreachable instruction 2. file.o: warning: objtool: .text+0x53: unreachable instruction
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment