* [tip: x86/alternatives] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer @ 2024-06-19 8:39 tip-bot2 for Borislav Petkov (AMD) 2024-06-20 8:48 ` Borislav Petkov 0 siblings, 1 reply; 5+ messages in thread From: tip-bot2 for Borislav Petkov (AMD) @ 2024-06-19 8:39 UTC (permalink / raw) To: linux-tip-commits Cc: kernel test robot, Borislav Petkov (AMD), Sean Christopherson, x86, linux-kernel The following commit has been merged into the x86/alternatives branch of tip: Commit-ID: 93f78dadee5e56ae48aff567583d503868aa3bf2 Gitweb: https://git.kernel.org/tip/93f78dadee5e56ae48aff567583d503868aa3bf2 Author: Borislav Petkov (AMD) <bp@alien8.de> AuthorDate: Tue, 18 Jun 2024 21:57:27 +02:00 Committer: Borislav Petkov (AMD) <bp@alien8.de> CommitterDate: Wed, 19 Jun 2024 10:33:25 +02:00 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 courtesy of peterz. Also from him add some helpful debugging info to the documentation. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202406141648.jO9qNGLa-lkp@intel.com/ Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Acked-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/alternative.h | 2 +- arch/x86/kernel/alternative.c | 2 +- arch/x86/kvm/emulate.c | 2 +- tools/objtool/Documentation/objtool.txt | 19 +++++++++++++++++++ 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h index 89fa50d..8cff462 100644 --- a/arch/x86/include/asm/alternative.h +++ b/arch/x86/include/asm/alternative.h @@ -248,7 +248,7 @@ static inline int alternatives_text_reserved(void *start, void *end) */ #define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ - : output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) + : output, ASM_CALL_CONSTRAINT : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) /* * Like alternative_call, but there are two features and respective functions. diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 37596a4..333b161 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -1657,7 +1657,7 @@ static noinline void __init alt_reloc_selftest(void) */ asm_inline volatile ( ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS) - : /* output */ + : ASM_CALL_CONSTRAINT : [mem] "m" (__alt_reloc_selftest_addr) : _ASM_ARG1 ); diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 5d4c861..c8cc578 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -1069,7 +1069,7 @@ 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) : [thunk_target]"r"(fop), [flags]"r"(flags)); + : "=a"(rc), ASM_CALL_CONSTRAINT : [thunk_target]"r"(fop), [flags]"r"(flags)); return rc; } diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt index fe39c2a..7c3ee95 100644 --- a/tools/objtool/Documentation/objtool.txt +++ b/tools/objtool/Documentation/objtool.txt @@ -284,6 +284,25 @@ the objtool maintainers. 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [tip: x86/alternatives] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer 2024-06-19 8:39 [tip: x86/alternatives] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer tip-bot2 for Borislav Petkov (AMD) @ 2024-06-20 8:48 ` Borislav Petkov 2024-06-25 11:20 ` [PATCH -v2] " Borislav Petkov 0 siblings, 1 reply; 5+ messages in thread From: Borislav Petkov @ 2024-06-20 8:48 UTC (permalink / raw) To: linux-kernel Cc: linux-tip-commits, kernel test robot, Sean Christopherson, x86 On Wed, Jun 19, 2024 at 08:39:52AM -0000, tip-bot2 for Borislav Petkov (AMD) wrote: > The following commit has been merged into the x86/alternatives branch of tip: > > Commit-ID: 93f78dadee5e56ae48aff567583d503868aa3bf2 > Gitweb: https://git.kernel.org/tip/93f78dadee5e56ae48aff567583d503868aa3bf2 > Author: Borislav Petkov (AMD) <bp@alien8.de> > AuthorDate: Tue, 18 Jun 2024 21:57:27 +02:00 > Committer: Borislav Petkov (AMD) <bp@alien8.de> > CommitterDate: Wed, 19 Jun 2024 10:33:25 +02:00 > > 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 courtesy of peterz. Also from him add some helpful > debugging info to the documentation. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202406141648.jO9qNGLa-lkp@intel.com/ > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Acked-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/alternative.h | 2 +- > arch/x86/kernel/alternative.c | 2 +- > arch/x86/kvm/emulate.c | 2 +- > tools/objtool/Documentation/objtool.txt | 19 +++++++++++++++++++ > 4 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h > index 89fa50d..8cff462 100644 > --- a/arch/x86/include/asm/alternative.h > +++ b/arch/x86/include/asm/alternative.h > @@ -248,7 +248,7 @@ static inline int alternatives_text_reserved(void *start, void *end) > */ > #define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ > asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ > - : output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) > + : output, ASM_CALL_CONSTRAINT : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) Yeah, this doesn't fly currently: https://lore.kernel.org/r/202406200507.AXxJ6Bmw-lkp@intel.com because those atomic64_32.h macros do alternative_atomic64(set, /* no output */, "S" (v), "b" (low), "c" (high) so without an output, it ends up becoming: asm __inline volatile("# ALT: oldinstr\n" ... ".popsection\n" : , "+r" (current_stack_pointer) : [old] "i" ... note the preceding ",". And I can't do "output..." macro argument with ellipsis and paste with "## output" because "input..." already does that. :-\ So I am not sure what to do here. Removing the ASM_CALL_CONSTRAINT works, let's see whether it passes build tests. Or add dummy output arguments to the three atomic macros which have no output? Hm. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH -v2] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer 2024-06-20 8:48 ` Borislav Petkov @ 2024-06-25 11:20 ` Borislav Petkov 2024-06-25 15:42 ` Sean Christopherson 2024-07-01 10:58 ` [tip: x86/alternatives] " tip-bot2 for Borislav Petkov (AMD) 0 siblings, 2 replies; 5+ messages in thread From: Borislav Petkov @ 2024-06-25 11:20 UTC (permalink / raw) To: linux-kernel Cc: linux-tip-commits, kernel test robot, Sean Christopherson, x86, Michael Matz On Thu, Jun 20, 2024 at 10:48:53AM +0200, Borislav Petkov wrote: > On Wed, Jun 19, 2024 at 08:39:52AM -0000, tip-bot2 for Borislav Petkov (AMD) wrote: > > The following commit has been merged into the x86/alternatives branch of tip: > > > > Commit-ID: 93f78dadee5e56ae48aff567583d503868aa3bf2 > > Gitweb: https://git.kernel.org/tip/93f78dadee5e56ae48aff567583d503868aa3bf2 > > Author: Borislav Petkov (AMD) <bp@alien8.de> > > AuthorDate: Tue, 18 Jun 2024 21:57:27 +02:00 > > Committer: Borislav Petkov (AMD) <bp@alien8.de> > > CommitterDate: Wed, 19 Jun 2024 10:33:25 +02:00 > > > > 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 courtesy of peterz. Also from him add some helpful > > debugging info to the documentation. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202406141648.jO9qNGLa-lkp@intel.com/ > > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > > Acked-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/include/asm/alternative.h | 2 +- > > arch/x86/kernel/alternative.c | 2 +- > > arch/x86/kvm/emulate.c | 2 +- > > tools/objtool/Documentation/objtool.txt | 19 +++++++++++++++++++ > > 4 files changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h > > index 89fa50d..8cff462 100644 > > --- a/arch/x86/include/asm/alternative.h > > +++ b/arch/x86/include/asm/alternative.h > > @@ -248,7 +248,7 @@ static inline int alternatives_text_reserved(void *start, void *end) > > */ > > #define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ > > asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ > > - : output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) > > + : output, ASM_CALL_CONSTRAINT : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) > > Yeah, this doesn't fly currently: > > https://lore.kernel.org/r/202406200507.AXxJ6Bmw-lkp@intel.com > > because those atomic64_32.h macros do > > alternative_atomic64(set, /* no output */, > "S" (v), "b" (low), "c" (high) > > so without an output, it ends up becoming: > > asm __inline volatile("# ALT: oldinstr\n" ... ".popsection\n" : , "+r" (current_stack_pointer) : [old] "i" ... > > note the preceding ",". > > And I can't do "output..." macro argument with ellipsis and paste with "## > output" because "input..." already does that. :-\ > > So I am not sure what to do here. Removing the ASM_CALL_CONSTRAINT works, > let's see whether it passes build tests. > > Or add dummy output arguments to the three atomic macros which have no > output? Ok, after a lot of back'n'forth, here's v2: --- From: "Borislav Petkov (AMD)" <bp@alien8.de> Date: Tue, 18 Jun 2024 21:57:27 +0200 Subject: [PATCH] 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... Cc: Michael Matz <matz@suse.de> Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202406141648.jO9qNGLa-lkp@intel.com/ Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> --- arch/x86/include/asm/alternative.h | 11 +++++++---- arch/x86/include/asm/page_64.h | 2 +- arch/x86/kernel/alternative.c | 2 +- arch/x86/kvm/emulate.c | 2 +- tools/objtool/Documentation/objtool.txt | 19 +++++++++++++++++++ 5 files changed, 29 insertions(+), 7 deletions(-) diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h index 89fa50d27a08..ca9ae606aab9 100644 --- a/arch/x86/include/asm/alternative.h +++ b/arch/x86/include/asm/alternative.h @@ -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 * suffix. */ -#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ - asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ - : output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) +#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ + asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ + : ALT_OUTPUT_SP(output) \ + : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) /* * 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) output, input...) \ asm_inline volatile(ALTERNATIVE_2("call %c[old]", "call %c[new1]", ft_flags1, \ "call %c[new2]", ft_flags2) \ - : output, ASM_CALL_CONSTRAINT \ + : ALT_OUTPUT_SP(output) \ : [old] "i" (oldfunc), [new1] "i" (newfunc1), \ [new2] "i" (newfunc2), ## input) @@ -276,6 +277,8 @@ static inline int alternatives_text_reserved(void *start, void *end) */ #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. */ #define DEFINE_ASM_FUNC(func, instr, sec) \ asm (".pushsection " #sec ", \"ax\"\n" \ diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h index cc6b8e087192..af4302d79b59 100644 --- a/arch/x86/include/asm/page_64.h +++ b/arch/x86/include/asm/page_64.h @@ -54,7 +54,7 @@ static inline void clear_page(void *page) clear_page_rep, X86_FEATURE_REP_GOOD, clear_page_erms, X86_FEATURE_ERMS, "=D" (page), - "0" (page) + "D" (page) : "cc", "memory", "rax", "rcx"); } diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 37596a417094..333b16181357 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -1657,7 +1657,7 @@ static noinline void __init alt_reloc_selftest(void) */ asm_inline volatile ( ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS) - : /* output */ + : ASM_CALL_CONSTRAINT : [mem] "m" (__alt_reloc_selftest_addr) : _ASM_ARG1 ); diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 5d4c86133453..c8cc578646d0 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -1069,7 +1069,7 @@ 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) : [thunk_target]"r"(fop), [flags]"r"(flags)); + : "=a"(rc), ASM_CALL_CONSTRAINT : [thunk_target]"r"(fop), [flags]"r"(flags)); return rc; } diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt index fe39c2a8ef0d..7c3ee959b63c 100644 --- a/tools/objtool/Documentation/objtool.txt +++ b/tools/objtool/Documentation/objtool.txt @@ -284,6 +284,25 @@ the objtool maintainers. 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.43.0 -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -v2] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer 2024-06-25 11:20 ` [PATCH -v2] " Borislav Petkov @ 2024-06-25 15:42 ` Sean Christopherson 2024-07-01 10:58 ` [tip: x86/alternatives] " tip-bot2 for Borislav Petkov (AMD) 1 sibling, 0 replies; 5+ messages in thread From: Sean Christopherson @ 2024-06-25 15:42 UTC (permalink / raw) To: Borislav Petkov Cc: linux-kernel, linux-tip-commits, kernel test robot, x86, Michael Matz On Tue, Jun 25, 2024, Borislav Petkov wrote: > --- > From: "Borislav Petkov (AMD)" <bp@alien8.de> > Date: Tue, 18 Jun 2024 21:57:27 +0200 > Subject: [PATCH] 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... > > Cc: Michael Matz <matz@suse.de> > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202406141648.jO9qNGLa-lkp@intel.com/ > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > --- Acked-by: Sean Christopherson <seanjc@google.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip: x86/alternatives] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer 2024-06-25 11:20 ` [PATCH -v2] " Borislav Petkov 2024-06-25 15:42 ` Sean Christopherson @ 2024-07-01 10:58 ` tip-bot2 for Borislav Petkov (AMD) 1 sibling, 0 replies; 5+ messages in thread From: tip-bot2 for Borislav Petkov (AMD) @ 2024-07-01 10:58 UTC (permalink / raw) To: linux-tip-commits Cc: kernel test robot, Borislav Petkov (AMD), Sean Christopherson, Peter Zijlstra (Intel), x86, linux-kernel The following commit has been merged into the x86/alternatives branch of tip: Commit-ID: 0d3db1f14abb4eb28613fbeb1e2ad92bac76debf Gitweb: https://git.kernel.org/tip/0d3db1f14abb4eb28613fbeb1e2ad92bac76debf Author: Borislav Petkov (AMD) <bp@alien8.de> AuthorDate: Tue, 18 Jun 2024 21:57:27 +02:00 Committer: Borislav Petkov (AMD) <bp@alien8.de> CommitterDate: Mon, 01 Jul 2024 12:41:11 +02:00 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: kernel test robot <lkp@intel.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Acked-by: Sean Christopherson <seanjc@google.com> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lore.kernel.org/r/20240625112056.GDZnqoGDXgYuWBDUwu@fat_crate.local --- arch/x86/include/asm/alternative.h | 11 +++++++---- arch/x86/include/asm/page_64.h | 2 +- arch/x86/kernel/alternative.c | 2 +- arch/x86/kvm/emulate.c | 2 +- tools/objtool/Documentation/objtool.txt | 19 +++++++++++++++++++ 5 files changed, 29 insertions(+), 7 deletions(-) diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h index 89fa50d..ca9ae60 100644 --- a/arch/x86/include/asm/alternative.h +++ b/arch/x86/include/asm/alternative.h @@ -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 * suffix. */ -#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ - asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ - : output : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) +#define alternative_call(oldfunc, newfunc, ft_flags, output, input...) \ + asm_inline volatile(ALTERNATIVE("call %c[old]", "call %c[new]", ft_flags) \ + : ALT_OUTPUT_SP(output) \ + : [old] "i" (oldfunc), [new] "i" (newfunc), ## input) /* * 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) output, input...) \ asm_inline volatile(ALTERNATIVE_2("call %c[old]", "call %c[new1]", ft_flags1, \ "call %c[new2]", ft_flags2) \ - : output, ASM_CALL_CONSTRAINT \ + : ALT_OUTPUT_SP(output) \ : [old] "i" (oldfunc), [new1] "i" (newfunc1), \ [new2] "i" (newfunc2), ## input) @@ -276,6 +277,8 @@ static inline int alternatives_text_reserved(void *start, void *end) */ #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. */ #define DEFINE_ASM_FUNC(func, instr, sec) \ asm (".pushsection " #sec ", \"ax\"\n" \ diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h index cc6b8e0..af4302d 100644 --- a/arch/x86/include/asm/page_64.h +++ b/arch/x86/include/asm/page_64.h @@ -54,7 +54,7 @@ static inline void clear_page(void *page) clear_page_rep, X86_FEATURE_REP_GOOD, clear_page_erms, X86_FEATURE_ERMS, "=D" (page), - "0" (page) + "D" (page) : "cc", "memory", "rax", "rcx"); } diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 37596a4..333b161 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -1657,7 +1657,7 @@ static noinline void __init alt_reloc_selftest(void) */ asm_inline volatile ( ALTERNATIVE("", "lea %[mem], %%" _ASM_ARG1 "; call __alt_reloc_selftest;", X86_FEATURE_ALWAYS) - : /* output */ + : ASM_CALL_CONSTRAINT : [mem] "m" (__alt_reloc_selftest_addr) : _ASM_ARG1 ); diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 5d4c861..c8cc578 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -1069,7 +1069,7 @@ 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) : [thunk_target]"r"(fop), [flags]"r"(flags)); + : "=a"(rc), ASM_CALL_CONSTRAINT : [thunk_target]"r"(fop), [flags]"r"(flags)); return rc; } diff --git a/tools/objtool/Documentation/objtool.txt b/tools/objtool/Documentation/objtool.txt index fe39c2a..7c3ee95 100644 --- a/tools/objtool/Documentation/objtool.txt +++ b/tools/objtool/Documentation/objtool.txt @@ -284,6 +284,25 @@ the objtool maintainers. 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-07-01 10:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-19 8:39 [tip: x86/alternatives] x86/alternatives, kvm: Fix a couple of CALLs without a frame pointer tip-bot2 for Borislav Petkov (AMD) 2024-06-20 8:48 ` Borislav Petkov 2024-06-25 11:20 ` [PATCH -v2] " Borislav Petkov 2024-06-25 15:42 ` Sean Christopherson 2024-07-01 10:58 ` [tip: x86/alternatives] " tip-bot2 for Borislav Petkov (AMD)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox