Skip to content

Commit 0d3db1f

Browse files
committed
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/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Borislav Petkov (AMD) <[email protected]> Acked-by: Sean Christopherson <[email protected]> Acked-by: Peter Zijlstra (Intel) <[email protected]> Link: https://lore.kernel.org/r/20240625112056.GDZnqoGDXgYuWBDUwu@fat_crate.local
1 parent f776e41 commit 0d3db1f

File tree

5 files changed

+29
-7
lines changed

5 files changed

+29
-7
lines changed

arch/x86/include/asm/alternative.h

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -246,9 +246,10 @@ static inline int alternatives_text_reserved(void *start, void *end)
246246
* references: i.e., if used for a function, it would add the PLT
247247
* suffix.
248248
*/
249-
#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \
250-
asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \
251-
: output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input)
249+
#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \
250+
asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \
251+
: ALT_OUTPUT_SP(output) \
252+
: [old] "i" (oldfunc), [new] "i" (newfunc), ## input)
252253

253254
/*
254255
* 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)
260261
output, input...) \
261262
asm_inline volatile(ALTERNATIVE_2("call %c[old]", "call %c[new1]", ft_flags1, \
262263
"call %c[new2]", ft_flags2) \
263-
: output, ASM_CALL_CONSTRAINT \
264+
: ALT_OUTPUT_SP(output) \
264265
: [old] "i" (oldfunc), [new1] "i" (newfunc1), \
265266
[new2] "i" (newfunc2), ## input)
266267

@@ -276,6 +277,8 @@ static inline int alternatives_text_reserved(void *start, void *end)
276277
*/
277278
#define ASM_NO_INPUT_CLOBBER(clbr...) "i" (0) : clbr
278279

280+
#define ALT_OUTPUT_SP(...) ASM_CALL_CONSTRAINT, ## __VA_ARGS__
281+
279282
/* Macro for creating assembler functions avoiding any C magic. */
280283
#define DEFINE_ASM_FUNC(func, instr, sec) \
281284
asm (".pushsection " #sec ", \"ax\"\n" \

arch/x86/include/asm/page_64.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ static inline void clear_page(void *page)
5454
clear_page_rep, X86_FEATURE_REP_GOOD,
5555
clear_page_erms, X86_FEATURE_ERMS,
5656
"=D" (page),
57-
"0" (page)
57+
"D" (page)
5858
: "cc", "memory", "rax", "rcx");
5959
}
6060

arch/x86/kernel/alternative.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1657,7 +1657,7 @@ static noinline void __init alt_reloc_selftest(void)
16571657
*/
16581658
asm_inline volatile (
16591659
ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS)
1660-
: /* output */
1660+
: ASM_CALL_CONSTRAINT
16611661
: [mem] "m" (__alt_reloc_selftest_addr)
16621662
: _ASM_ARG1
16631663
);

arch/x86/kvm/emulate.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1069,7 +1069,7 @@ static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
10691069

10701070
flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
10711071
asm("push %[flags]; popf; " CALL_NOSPEC
1072-
: "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));
1072+
: "=a"(rc), ASM_CALL_CONSTRAINT : [thunk_target]"r"(fop), [flags]"r"(flags));
10731073
return rc;
10741074
}
10751075

tools/objtool/Documentation/objtool.txt

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,25 @@ the objtool maintainers.
284284

285285
Otherwise the stack frame may not get created before the call.
286286

287+
objtool can help with pinpointing the exact function where it happens:
288+
289+
$ OBJTOOL_ARGS="--verbose" make arch/x86/kvm/
290+
291+
arch/x86/kvm/kvm.o: warning: objtool: .altinstr_replacement+0xc5: call without frame pointer save/setup
292+
arch/x86/kvm/kvm.o: warning: objtool: em_loop.part.0+0x29: (alt)
293+
arch/x86/kvm/kvm.o: warning: objtool: em_loop.part.0+0x0: <=== (sym)
294+
LD [M] arch/x86/kvm/kvm-intel.o
295+
0000 0000000000028220 <em_loop.part.0>:
296+
0000 28220: 0f b6 47 61 movzbl 0x61(%rdi),%eax
297+
0004 28224: 3c e2 cmp $0xe2,%al
298+
0006 28226: 74 2c je 28254 <em_loop.part.0+0x34>
299+
0008 28228: 48 8b 57 10 mov 0x10(%rdi),%rdx
300+
000c 2822c: 83 f0 05 xor $0x5,%eax
301+
000f 2822f: 48 c1 e0 04 shl $0x4,%rax
302+
0013 28233: 25 f0 00 00 00 and $0xf0,%eax
303+
0018 28238: 81 e2 d5 08 00 00 and $0x8d5,%edx
304+
001e 2823e: 80 ce 02 or $0x2,%dh
305+
...
287306

288307
2. file.o: warning: objtool: .text+0x53: unreachable instruction
289308

0 commit comments

Comments
 (0)