* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.