tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx
head: 7b804135d4d1f0a2b9dda69c6303d3f2dcbe9d37
commit: c641cfb5c157b6c3062a824fd8ba190bf06fb952 [8/12] x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
config: x86_64-rhel-8.3-bpf (https://download.01.org/0day-ci/archive/20230914/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> vmlinux.o: warning: objtool: __tdx_hypercall+0x128: __tdx_hypercall_failed() is missing a __noreturn annotation
objdump-func vmlinux.o __tdx_hypercall:
0000 0000000000000050 <__tdx_hypercall_failed>:
0000 50: f3 0f 1e fa endbr64
0004 54: 48 c7 c7 00 00 00 00 mov $0x0,%rdi 57: R_X86_64_32S .rodata.str1.8+0xe18
000b 5b: e8 00 00 00 00 call 60 <__tdx_hypercall> 5c: R_X86_64_PLT32 panic-0x4
0000 0000000000000060 <__tdx_hypercall>:
0000 60: f3 0f 1e fa endbr64
0004 64: 53 push %rbx
0005 65: 48 89 fb mov %rdi,%rbx
0008 68: 48 83 ec 70 sub $0x70,%rsp
000c 6c: 65 48 8b 04 25 28 00 00 00 mov %gs:0x28,%rax
0015 75: 48 89 44 24 68 mov %rax,0x68(%rsp)
001a 7a: 31 c0 xor %eax,%eax
001c 7c: 48 8b 47 58 mov 0x58(%rdi),%rax
0020 80: 48 89 e6 mov %rsp,%rsi
0023 83: 48 c7 04 24 cc ff 00 00 movq $0xffcc,(%rsp)
002b 8b: 48 89 44 24 08 mov %rax,0x8(%rsp)
0030 90: 48 8b 07 mov (%rdi),%rax
0033 93: 48 89 44 24 10 mov %rax,0x10(%rsp)
0038 98: 48 8b 47 08 mov 0x8(%rdi),%rax
003c 9c: 48 89 44 24 18 mov %rax,0x18(%rsp)
0041 a1: 48 8b 47 10 mov 0x10(%rdi),%rax
0045 a5: 48 89 44 24 20 mov %rax,0x20(%rsp)
004a aa: 48 8b 47 18 mov 0x18(%rdi),%rax
004e ae: 48 89 44 24 28 mov %rax,0x28(%rsp)
0053 b3: 48 8b 47 20 mov 0x20(%rdi),%rax
0057 b7: 48 89 44 24 30 mov %rax,0x30(%rsp)
005c bc: 48 8b 47 28 mov 0x28(%rdi),%rax
0060 c0: 48 89 44 24 38 mov %rax,0x38(%rsp)
0065 c5: 48 8b 47 30 mov 0x30(%rdi),%rax
0069 c9: 48 89 44 24 40 mov %rax,0x40(%rsp)
006e ce: 48 8b 47 38 mov 0x38(%rdi),%rax
0072 d2: 48 89 44 24 48 mov %rax,0x48(%rsp)
0077 d7: 48 8b 47 50 mov 0x50(%rdi),%rax
007b db: 48 89 44 24 50 mov %rax,0x50(%rsp)
0080 e0: 48 8b 47 40 mov 0x40(%rdi),%rax
0084 e4: 48 89 44 24 58 mov %rax,0x58(%rsp)
0089 e9: 48 8b 47 48 mov 0x48(%rdi),%rax
008d ed: 31 ff xor %edi,%edi
008f ef: 48 89 44 24 60 mov %rax,0x60(%rsp)
0094 f4: e8 00 00 00 00 call f9 <__tdx_hypercall+0x99> f5: R_X86_64_PLT32 __tdcall_hypercall-0x4
0099 f9: 48 85 c0 test %rax,%rax
009c fc: 0f 85 81 00 00 00 jne 183 <__tdx_hypercall+0x123>
00a2 102: 48 8b 54 24 28 mov 0x28(%rsp),%rdx
00a7 107: 48 8b 44 24 10 mov 0x10(%rsp),%rax
00ac 10c: 48 89 53 18 mov %rdx,0x18(%rbx)
00b0 110: 48 8b 54 24 30 mov 0x30(%rsp),%rdx
00b5 115: 48 89 03 mov %rax,(%rbx)
00b8 118: 48 8b 44 24 18 mov 0x18(%rsp),%rax
00bd 11d: 48 89 53 20 mov %rdx,0x20(%rbx)
00c1 121: 48 8b 54 24 38 mov 0x38(%rsp),%rdx
00c6 126: 48 89 43 08 mov %rax,0x8(%rbx)
00ca 12a: 48 8b 44 24 20 mov 0x20(%rsp),%rax
00cf 12f: 48 89 53 28 mov %rdx,0x28(%rbx)
00d3 133: 48 8b 54 24 40 mov 0x40(%rsp),%rdx
00d8 138: 48 89 43 10 mov %rax,0x10(%rbx)
00dc 13c: 48 89 53 30 mov %rdx,0x30(%rbx)
00e0 140: 48 8b 54 24 48 mov 0x48(%rsp),%rdx
00e5 145: 48 89 53 38 mov %rdx,0x38(%rbx)
00e9 149: 48 8b 54 24 58 mov 0x58(%rsp),%rdx
00ee 14e: 48 89 53 40 mov %rdx,0x40(%rbx)
00f2 152: 48 8b 54 24 60 mov 0x60(%rsp),%rdx
00f7 157: 48 89 53 48 mov %rdx,0x48(%rbx)
00fb 15b: 48 8b 54 24 50 mov 0x50(%rsp),%rdx
0100 160: 48 89 53 50 mov %rdx,0x50(%rbx)
0104 164: 48 8b 54 24 08 mov 0x8(%rsp),%rdx
0109 169: 48 89 53 58 mov %rdx,0x58(%rbx)
010d 16d: 48 8b 54 24 68 mov 0x68(%rsp),%rdx
0112 172: 65 48 2b 14 25 28 00 00 00 sub %gs:0x28,%rdx
011b 17b: 75 10 jne 18d <__tdx_hypercall+0x12d>
011d 17d: 48 83 c4 70 add $0x70,%rsp
0121 181: 5b pop %rbx
0122 182: c3 ret
0123 183: e8 00 00 00 00 call 188 <__tdx_hypercall+0x128> 184: R_X86_64_PLT32 __tdx_hypercall_failed-0x4
0128 188: e9 75 ff ff ff jmp 102 <__tdx_hypercall+0xa2>
012d 18d: e8 00 00 00 00 call 192 <__tdx_hypercall+0x132> 18e: R_X86_64_PLT32 __stack_chk_fail-0x4
0132 192: 66 2e 0f 1f 84 00 00 00 00 00 cs nopw 0x0(%rax,%rax,1)
013c 19c: 0f 1f 40 00 nopl 0x0(%rax)
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, 2023-09-14 at 01:23 +0000, Huang, Kai wrote:
> On Thu, 2023-09-14 at 09:05 +0800, kernel test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx
> > head: 7b804135d4d1f0a2b9dda69c6303d3f2dcbe9d37
> > commit: c641cfb5c157b6c3062a824fd8ba190bf06fb952 [8/12] x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
> > config: x86_64-rhel-8.3-bpf (https://download.01.org/0day-ci/archive/20230914/[email protected]/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/[email protected]/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > > Reported-by: kernel test robot <[email protected]>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >
> > All warnings (new ones prefixed by >>):
> >
> > > > vmlinux.o: warning: objtool: __tdx_hypercall+0x128: __tdx_hypercall_failed() is missing a __noreturn annotation
> >
>
> Hmm.. The __tdx_hypercall_failed() is already annotated with __noreturn (I
> explicitly added it to silent such warning):
>
> /* Called from __tdx_hypercall() for unrecoverable failure */
> noinstr void __noreturn __tdx_hypercall_failed(void)
> {
> instrumentation_begin();
> panic("TDVMCALL failed. TDX module bug?");
> }
>
> Not sure why the objtool is still complaining this?
>
It appears the __noreturn must be annotated to the function declaration but not
the function body. I'll send out the fix as soon as I confirm the fix with LKP.
Sorry for the noise.
>
On Thu, 2023-09-14 at 09:05 +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx
> head: 7b804135d4d1f0a2b9dda69c6303d3f2dcbe9d37
> commit: c641cfb5c157b6c3062a824fd8ba190bf06fb952 [8/12] x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
> config: x86_64-rhel-8.3-bpf (https://download.01.org/0day-ci/archive/20230914/[email protected]/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> > > vmlinux.o: warning: objtool: __tdx_hypercall+0x128: __tdx_hypercall_failed() is missing a __noreturn annotation
>
Hmm.. The __tdx_hypercall_failed() is already annotated with __noreturn (I
explicitly added it to silent such warning):
/* Called from __tdx_hypercall() for unrecoverable failure */
noinstr void __noreturn __tdx_hypercall_failed(void)
{
instrumentation_begin();
panic("TDVMCALL failed. TDX module bug?");
}
Not sure why the objtool is still complaining this?
>
> objdump-func vmlinux.o __tdx_hypercall:
> 0000 0000000000000050 <__tdx_hypercall_failed>:
> 0000 50: f3 0f 1e fa endbr64
> 0004 54: 48 c7 c7 00 00 00 00 mov $0x0,%rdi 57: R_X86_64_32S .rodata.str1.8+0xe18
> 000b 5b: e8 00 00 00 00 call 60 <__tdx_hypercall> 5c: R_X86_64_PLT32 panic-0x4
> 0000 0000000000000060 <__tdx_hypercall>:
> 0000 60: f3 0f 1e fa endbr64
> 0004 64: 53 push %rbx
> 0005 65: 48 89 fb mov %rdi,%rbx
> 0008 68: 48 83 ec 70 sub $0x70,%rsp
> 000c 6c: 65 48 8b 04 25 28 00 00 00 mov %gs:0x28,%rax
> 0015 75: 48 89 44 24 68 mov %rax,0x68(%rsp)
> 001a 7a: 31 c0 xor %eax,%eax
> 001c 7c: 48 8b 47 58 mov 0x58(%rdi),%rax
> 0020 80: 48 89 e6 mov %rsp,%rsi
> 0023 83: 48 c7 04 24 cc ff 00 00 movq $0xffcc,(%rsp)
> 002b 8b: 48 89 44 24 08 mov %rax,0x8(%rsp)
> 0030 90: 48 8b 07 mov (%rdi),%rax
> 0033 93: 48 89 44 24 10 mov %rax,0x10(%rsp)
> 0038 98: 48 8b 47 08 mov 0x8(%rdi),%rax
> 003c 9c: 48 89 44 24 18 mov %rax,0x18(%rsp)
> 0041 a1: 48 8b 47 10 mov 0x10(%rdi),%rax
> 0045 a5: 48 89 44 24 20 mov %rax,0x20(%rsp)
> 004a aa: 48 8b 47 18 mov 0x18(%rdi),%rax
> 004e ae: 48 89 44 24 28 mov %rax,0x28(%rsp)
> 0053 b3: 48 8b 47 20 mov 0x20(%rdi),%rax
> 0057 b7: 48 89 44 24 30 mov %rax,0x30(%rsp)
> 005c bc: 48 8b 47 28 mov 0x28(%rdi),%rax
> 0060 c0: 48 89 44 24 38 mov %rax,0x38(%rsp)
> 0065 c5: 48 8b 47 30 mov 0x30(%rdi),%rax
> 0069 c9: 48 89 44 24 40 mov %rax,0x40(%rsp)
> 006e ce: 48 8b 47 38 mov 0x38(%rdi),%rax
> 0072 d2: 48 89 44 24 48 mov %rax,0x48(%rsp)
> 0077 d7: 48 8b 47 50 mov 0x50(%rdi),%rax
> 007b db: 48 89 44 24 50 mov %rax,0x50(%rsp)
> 0080 e0: 48 8b 47 40 mov 0x40(%rdi),%rax
> 0084 e4: 48 89 44 24 58 mov %rax,0x58(%rsp)
> 0089 e9: 48 8b 47 48 mov 0x48(%rdi),%rax
> 008d ed: 31 ff xor %edi,%edi
> 008f ef: 48 89 44 24 60 mov %rax,0x60(%rsp)
> 0094 f4: e8 00 00 00 00 call f9 <__tdx_hypercall+0x99> f5: R_X86_64_PLT32 __tdcall_hypercall-0x4
> 0099 f9: 48 85 c0 test %rax,%rax
> 009c fc: 0f 85 81 00 00 00 jne 183 <__tdx_hypercall+0x123>
> 00a2 102: 48 8b 54 24 28 mov 0x28(%rsp),%rdx
> 00a7 107: 48 8b 44 24 10 mov 0x10(%rsp),%rax
> 00ac 10c: 48 89 53 18 mov %rdx,0x18(%rbx)
> 00b0 110: 48 8b 54 24 30 mov 0x30(%rsp),%rdx
> 00b5 115: 48 89 03 mov %rax,(%rbx)
> 00b8 118: 48 8b 44 24 18 mov 0x18(%rsp),%rax
> 00bd 11d: 48 89 53 20 mov %rdx,0x20(%rbx)
> 00c1 121: 48 8b 54 24 38 mov 0x38(%rsp),%rdx
> 00c6 126: 48 89 43 08 mov %rax,0x8(%rbx)
> 00ca 12a: 48 8b 44 24 20 mov 0x20(%rsp),%rax
> 00cf 12f: 48 89 53 28 mov %rdx,0x28(%rbx)
> 00d3 133: 48 8b 54 24 40 mov 0x40(%rsp),%rdx
> 00d8 138: 48 89 43 10 mov %rax,0x10(%rbx)
> 00dc 13c: 48 89 53 30 mov %rdx,0x30(%rbx)
> 00e0 140: 48 8b 54 24 48 mov 0x48(%rsp),%rdx
> 00e5 145: 48 89 53 38 mov %rdx,0x38(%rbx)
> 00e9 149: 48 8b 54 24 58 mov 0x58(%rsp),%rdx
> 00ee 14e: 48 89 53 40 mov %rdx,0x40(%rbx)
> 00f2 152: 48 8b 54 24 60 mov 0x60(%rsp),%rdx
> 00f7 157: 48 89 53 48 mov %rdx,0x48(%rbx)
> 00fb 15b: 48 8b 54 24 50 mov 0x50(%rsp),%rdx
> 0100 160: 48 89 53 50 mov %rdx,0x50(%rbx)
> 0104 164: 48 8b 54 24 08 mov 0x8(%rsp),%rdx
> 0109 169: 48 89 53 58 mov %rdx,0x58(%rbx)
> 010d 16d: 48 8b 54 24 68 mov 0x68(%rsp),%rdx
> 0112 172: 65 48 2b 14 25 28 00 00 00 sub %gs:0x28,%rdx
> 011b 17b: 75 10 jne 18d <__tdx_hypercall+0x12d>
> 011d 17d: 48 83 c4 70 add $0x70,%rsp
> 0121 181: 5b pop %rbx
> 0122 182: c3 ret
> 0123 183: e8 00 00 00 00 call 188 <__tdx_hypercall+0x128> 184: R_X86_64_PLT32 __tdx_hypercall_failed-0x4
> 0128 188: e9 75 ff ff ff jmp 102 <__tdx_hypercall+0xa2>
> 012d 18d: e8 00 00 00 00 call 192 <__tdx_hypercall+0x132> 18e: R_X86_64_PLT32 __stack_chk_fail-0x4
> 0132 192: 66 2e 0f 1f 84 00 00 00 00 00 cs nopw 0x0(%rax,%rax,1)
> 013c 19c: 0f 1f 40 00 nopl 0x0(%rax)
>
On Thu, Sep 14, 2023 at 03:21:29AM +0000, Huang, Kai wrote:
> On Thu, 2023-09-14 at 01:23 +0000, Huang, Kai wrote:
> > On Thu, 2023-09-14 at 09:05 +0800, kernel test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx
> > > head: 7b804135d4d1f0a2b9dda69c6303d3f2dcbe9d37
> > > commit: c641cfb5c157b6c3062a824fd8ba190bf06fb952 [8/12] x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
> > > config: x86_64-rhel-8.3-bpf (https://download.01.org/0day-ci/archive/20230914/[email protected]/config)
> > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/[email protected]/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > > Reported-by: kernel test robot <[email protected]>
> > > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > > > > vmlinux.o: warning: objtool: __tdx_hypercall+0x128: __tdx_hypercall_failed() is missing a __noreturn annotation
> > >
> >
> > Hmm.. The __tdx_hypercall_failed() is already annotated with __noreturn (I
> > explicitly added it to silent such warning):
> >
> > /* Called from __tdx_hypercall() for unrecoverable failure */
> > noinstr void __noreturn __tdx_hypercall_failed(void)
> > {
> > instrumentation_begin();
> > panic("TDVMCALL failed. TDX module bug?");
> > }
> >
> > Not sure why the objtool is still complaining this?
> >
>
> It appears the __noreturn must be annotated to the function declaration but not
> the function body. I'll send out the fix as soon as I confirm the fix with LKP.
FWIW, the reason being that...
The point of noreturn is that the caller should know to stop generating
code. For that the declaration needs the attribute, because call sites
typically do not have access to the function definition in C.
On Thu, 2023-09-14 at 10:59 +0200, Peter Zijlstra wrote:
> On Thu, Sep 14, 2023 at 07:54:10AM +0000, Huang, Kai wrote:
>
> > > The point of noreturn is that the caller should know to stop generating
> > > code. For that the declaration needs the attribute, because call sites
> > > typically do not have access to the function definition in C.
> >
> > Ah that makes perfect sense. Thanks!
> >
> > Then I assume we don't need to annotate __noreturn in the function body, but
> > only in the declaration? Because the compiler must already have seen the
> > declaration when it generates the code for the function body.
>
> I think so, I'm sure it'll tell you if it disagrees :-)
>
> > Btw, I happened to notice that the objtool documentation suggests that we should
> > also add the the function to tools/objtool/noreturns.h:
> >
> > 3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
> >
> > The call from foo() to bar() doesn't return, but bar() is missing the
> > __noreturn annotation. NOTE: In addition to annotating the function
> > with __noreturn, please also add it to tools/objtool/noreturns.h.
> >
> > Is it a behaviour that we still need to follow?
>
> Yes. objtool has some heuristics to detect noreturn, but is is very
> difficult. Sadly noreturn is not something that is reflected in the ELF
> object file so we have to guess.
>
> For now manually adding it to the objtool list is required, we're trying
> to get to the point where it is generated/validated by the compiler,
> perhaps with a plugin, but we're not there yet.
Thanks Peter!
On Thu, Sep 14, 2023 at 10:02:40AM +0000, Huang, Kai wrote:
> On Thu, 2023-09-14 at 10:59 +0200, Peter Zijlstra wrote:
> > On Thu, Sep 14, 2023 at 07:54:10AM +0000, Huang, Kai wrote:
> >
> > > > The point of noreturn is that the caller should know to stop generating
> > > > code. For that the declaration needs the attribute, because call sites
> > > > typically do not have access to the function definition in C.
> > >
> > > Ah that makes perfect sense. Thanks!
> > >
> > > Then I assume we don't need to annotate __noreturn in the function body, but
> > > only in the declaration? Because the compiler must already have seen the
> > > declaration when it generates the code for the function body.
> >
> > I think so, I'm sure it'll tell you if it disagrees :-)
> >
> > > Btw, I happened to notice that the objtool documentation suggests that we should
> > > also add the the function to tools/objtool/noreturns.h:
> > >
> > > 3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
> > >
> > > The call from foo() to bar() doesn't return, but bar() is missing the
> > > __noreturn annotation. NOTE: In addition to annotating the function
> > > with __noreturn, please also add it to tools/objtool/noreturns.h.
> > >
> > > Is it a behaviour that we still need to follow?
> >
> > Yes. objtool has some heuristics to detect noreturn, but is is very
> > difficult. Sadly noreturn is not something that is reflected in the ELF
> > object file so we have to guess.
> >
> > For now manually adding it to the objtool list is required, we're trying
> > to get to the point where it is generated/validated by the compiler,
> > perhaps with a plugin, but we're not there yet.
>
> Sorry one more question, do you know what Kconfig option triggers this
> __noreturn objtool check? I couldn't reproduce it on my own machine but have to
> depend on LKP for now.
Nope, sorry. Could be very GCC version specific through, this is all
about a specific code-gen pattern.
AFAICT we emit this warning when objtool finds this function is a
noreturn but code-gen continued.
On Thu, 2023-09-14 at 10:59 +0200, Peter Zijlstra wrote:
> On Thu, Sep 14, 2023 at 07:54:10AM +0000, Huang, Kai wrote:
>
> > > The point of noreturn is that the caller should know to stop generating
> > > code. For that the declaration needs the attribute, because call sites
> > > typically do not have access to the function definition in C.
> >
> > Ah that makes perfect sense. Thanks!
> >
> > Then I assume we don't need to annotate __noreturn in the function body, but
> > only in the declaration? Because the compiler must already have seen the
> > declaration when it generates the code for the function body.
>
> I think so, I'm sure it'll tell you if it disagrees :-)
>
> > Btw, I happened to notice that the objtool documentation suggests that we should
> > also add the the function to tools/objtool/noreturns.h:
> >
> > 3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
> >
> > The call from foo() to bar() doesn't return, but bar() is missing the
> > __noreturn annotation. NOTE: In addition to annotating the function
> > with __noreturn, please also add it to tools/objtool/noreturns.h.
> >
> > Is it a behaviour that we still need to follow?
>
> Yes. objtool has some heuristics to detect noreturn, but is is very
> difficult. Sadly noreturn is not something that is reflected in the ELF
> object file so we have to guess.
>
> For now manually adding it to the objtool list is required, we're trying
> to get to the point where it is generated/validated by the compiler,
> perhaps with a plugin, but we're not there yet.
Sorry one more question, do you know what Kconfig option triggers this
__noreturn objtool check? I couldn't reproduce it on my own machine but have to
depend on LKP for now.
Hey,
On Thu, 14 Sep 2023, Peter Zijlstra wrote:
> > > > It appears the __noreturn must be annotated to the function declaration
> > > > but not the function body. I'll send out the fix as soon as I confirm
> > > > the fix with LKP.
> > >
> > > FWIW, the reason being that...
> > >
> > > The point of noreturn is that the caller should know to stop generating
> > > code. For that the declaration needs the attribute, because call sites
> > > typically do not have access to the function definition in C.
> >
> > BTW., arguably shouldn't the compiler generate a warning to begin with,
> > when it encounters a noreturn function definition whose prototype doesn't
> > have the attribute?
>
> Yeah, I would agree with that,
That makes sense, yeah. We actually have a warning -Wmissing-attributes
that would fit this usecase, but currently doesn't implement this case (it
only applies to aliases, not to decl vs. def).
> but I think the problem is that gnu
> attributes are all considered 'optional' and do not factor into the
> actual signature.
That actually depends on the attribute :) Most attributes are like that,
true, but some aren't optional in that sense as they influence the
callee-caller contract (e.g. those that change the ABI, like fastcall).
Those must then be part of the functions type.
'noreturn' is optional in that sense. But a warning might still be
warranted.
Ciao,
Michael.
On Thu, Sep 14, 2023 at 04:16:47PM +0200, Ingo Molnar wrote:
>
> * Peter Zijlstra <[email protected]> wrote:
>
> > > It appears the __noreturn must be annotated to the function declaration
> > > but not the function body. I'll send out the fix as soon as I confirm
> > > the fix with LKP.
> >
> > FWIW, the reason being that...
> >
> > The point of noreturn is that the caller should know to stop generating
> > code. For that the declaration needs the attribute, because call sites
> > typically do not have access to the function definition in C.
>
> BTW., arguably shouldn't the compiler generate a warning to begin with,
> when it encounters a noreturn function definition whose prototype doesn't
> have the attribute?
Yeah, I would agree with that, but I think the problem is that gnu
attributes are all considered 'optional' and do not factor into the
actual signature.
Added Michael to Cc so he may clarify if I'm talking nonsense.
* Peter Zijlstra <[email protected]> wrote:
> > It appears the __noreturn must be annotated to the function declaration
> > but not the function body. I'll send out the fix as soon as I confirm
> > the fix with LKP.
>
> FWIW, the reason being that...
>
> The point of noreturn is that the caller should know to stop generating
> code. For that the declaration needs the attribute, because call sites
> typically do not have access to the function definition in C.
BTW., arguably shouldn't the compiler generate a warning to begin with,
when it encounters a noreturn function definition whose prototype doesn't
have the attribute?
Thanks,
Ingo
On Thu, Sep 14, 2023 at 07:54:10AM +0000, Huang, Kai wrote:
> > The point of noreturn is that the caller should know to stop generating
> > code. For that the declaration needs the attribute, because call sites
> > typically do not have access to the function definition in C.
>
> Ah that makes perfect sense. Thanks!
>
> Then I assume we don't need to annotate __noreturn in the function body, but
> only in the declaration? Because the compiler must already have seen the
> declaration when it generates the code for the function body.
I think so, I'm sure it'll tell you if it disagrees :-)
> Btw, I happened to notice that the objtool documentation suggests that we should
> also add the the function to tools/objtool/noreturns.h:
>
> 3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
>
> The call from foo() to bar() doesn't return, but bar() is missing the
> __noreturn annotation. NOTE: In addition to annotating the function
> with __noreturn, please also add it to tools/objtool/noreturns.h.
>
> Is it a behaviour that we still need to follow?
Yes. objtool has some heuristics to detect noreturn, but is is very
difficult. Sadly noreturn is not something that is reflected in the ELF
object file so we have to guess.
For now manually adding it to the objtool list is required, we're trying
to get to the point where it is generated/validated by the compiler,
perhaps with a plugin, but we're not there yet.
On Thu, 2023-09-14 at 09:29 +0200, Peter Zijlstra wrote:
> On Thu, Sep 14, 2023 at 03:21:29AM +0000, Huang, Kai wrote:
> > On Thu, 2023-09-14 at 01:23 +0000, Huang, Kai wrote:
> > > On Thu, 2023-09-14 at 09:05 +0800, kernel test robot wrote:
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx
> > > > head: 7b804135d4d1f0a2b9dda69c6303d3f2dcbe9d37
> > > > commit: c641cfb5c157b6c3062a824fd8ba190bf06fb952 [8/12] x86/tdx: Make TDX_HYPERCALL asm similar to TDX_MODULE_CALL
> > > > config: x86_64-rhel-8.3-bpf (https://download.01.org/0day-ci/archive/20230914/[email protected]/config)
> > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/[email protected]/reproduce)
> > > >
> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > the same patch/commit), kindly add following tags
> > > > > Reported-by: kernel test robot <[email protected]>
> > > > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > >
> > > > All warnings (new ones prefixed by >>):
> > > >
> > > > > > vmlinux.o: warning: objtool: __tdx_hypercall+0x128: __tdx_hypercall_failed() is missing a __noreturn annotation
> > > >
> > >
> > > Hmm.. The __tdx_hypercall_failed() is already annotated with __noreturn (I
> > > explicitly added it to silent such warning):
> > >
> > > /* Called from __tdx_hypercall() for unrecoverable failure */
> > > noinstr void __noreturn __tdx_hypercall_failed(void)
> > > {
> > > instrumentation_begin();
> > > panic("TDVMCALL failed. TDX module bug?");
> > > }
> > >
> > > Not sure why the objtool is still complaining this?
> > >
> >
> > It appears the __noreturn must be annotated to the function declaration but not
> > the function body. I'll send out the fix as soon as I confirm the fix with LKP.
>
> FWIW, the reason being that...
>
> The point of noreturn is that the caller should know to stop generating
> code. For that the declaration needs the attribute, because call sites
> typically do not have access to the function definition in C.
Ah that makes perfect sense. Thanks!
Then I assume we don't need to annotate __noreturn in the function body, but
only in the declaration? Because the compiler must already have seen the
declaration when it generates the code for the function body.
Btw, I happened to notice that the objtool documentation suggests that we should
also add the the function to tools/objtool/noreturns.h:
3. file.o: warning: objtool: foo+0x48c: bar() is missing a __noreturn annotation
The call from foo() to bar() doesn't return, but bar() is missing the
__noreturn annotation. NOTE: In addition to annotating the function
with __noreturn, please also add it to tools/objtool/noreturns.h.
Is it a behaviour that we still need to follow?
I am asking because the old kernel code doesn't have it so perhaps I am missing
something here.