arch/x86/virt/vmx/tdx/seamcall.S | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Build warnings about missing ENDBR around making SEAMCALLs[*] were
observed when using some randconfig[1] to build today's Linus's tree.
In the C code, the low level SEAMCALL assembly functions (__seamcall(),
__seamcall_ret() and __seamcall_saved_ret()) are indirectly called via
the common sc_retry() function:
static inline u64 sc_retry(sc_func_t func, u64 fn,
struct tdx_module_args *args)
{ ... }
#define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
It turns out compilers may not always be smart enough to figure out how
to call those assembly functions directly.
The disassembly of the vmlinux built from the aforementioned config
confirms that __seamcall*() are indirectly called:
<sc_retry>:
......
4c 89 ee mov %r13,%rsi
4c 89 e7 mov %r12,%rdi
e8 35 8c 7d 01 call ffffffff82b3e220 <__pi___x86_indirect_thunk_rbp>
4c 39 f0 cmp %r14,%rax
In this case ENDBR is needed at the beginning of __seamcall*().
Change SYM_FUNC_START() to SYM_TYPED_FUNC_START() for __seamcall*() to
add ENDBR to them.
When the compiler can generate direct call for __seamcall*(), the
additional ENDBR is safe since it has no impact to directly called
functions.
When kernel IBT was added to the kernel, initially the SYM_FUNC_START()
had the ENDBR added in commit
c4691712b546 ("x86/linkage: Add ENDBR to SYM_FUNC_START*()")
However when the commit
582077c94052 ("x86/cfi: Clean up linkage")
removed the ENDBR from the SYM_FUNC_START() and added it to the
SYM_TYPED_FUNC_START(), it didn't touch the SEAMCALL assembly.
[*] Aforementioned build warning:
vmlinux.o: warning: objtool: try_init_module_global+0x5d: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: read_sys_metadata_field+0x4a: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: do_global_key_config+0x36: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_phymem_page_reclaim+0x71: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_phymem_cache_wb+0x41: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_tdr+0x95: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdx_cpu_enable+0x7b: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: init_tdmr+0x59: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: config_tdx_module.constprop.0+0x19d: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_vp_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_vp_init+0x76: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_vp_wr+0x87: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_vp_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_vp_flush+0x4c: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_vp_create+0x85: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mng_create+0x73: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mem_page_aug+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mem_sept_add+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mem_page_add+0xce: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mng_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mem_page_remove+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mem_track+0x4c: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mng_init+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mng_key_freeid+0x4c: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mng_vpflushdone+0x4c: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mr_finalize+0x4c: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mr_extend+0x77: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mng_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_mng_key_config+0x4c: relocation to !ENDBR: __seamcall+0x0
vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_hkid+0x7d: relocation to !ENDBR: __seamcall+0x0
Fixes: 582077c94052 ("x86/cfi: Clean up linkage")
Link: https://download.01.org/0day-ci/archive/20250524/202505240530.5KktQ5mX-lkp@intel.com/config [1]
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
arch/x86/virt/vmx/tdx/seamcall.S | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
index 6854c52c374b..637226ae935d 100644
--- a/arch/x86/virt/vmx/tdx/seamcall.S
+++ b/arch/x86/virt/vmx/tdx/seamcall.S
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include <linux/linkage.h>
+#include <linux/cfi_types.h>
#include <asm/frame.h>
#include "tdxcall.S"
@@ -18,7 +19,7 @@
* Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
* fails, or the completion status of the SEAMCALL leaf function.
*/
-SYM_FUNC_START(__seamcall)
+SYM_TYPED_FUNC_START(__seamcall)
TDX_MODULE_CALL host=1
SYM_FUNC_END(__seamcall)
@@ -37,7 +38,7 @@ SYM_FUNC_END(__seamcall)
* Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
* fails, or the completion status of the SEAMCALL leaf function.
*/
-SYM_FUNC_START(__seamcall_ret)
+SYM_TYPED_FUNC_START(__seamcall_ret)
TDX_MODULE_CALL host=1 ret=1
SYM_FUNC_END(__seamcall_ret)
@@ -59,6 +60,6 @@ SYM_FUNC_END(__seamcall_ret)
* Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
* fails, or the completion status of the SEAMCALL leaf function.
*/
-SYM_FUNC_START(__seamcall_saved_ret)
+SYM_TYPED_FUNC_START(__seamcall_saved_ret)
TDX_MODULE_CALL host=1 ret=1 saved=1
SYM_FUNC_END(__seamcall_saved_ret)
base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282
--
2.49.0On Wed, Jun 04, 2025 at 12:38:48PM +1200, Kai Huang wrote:
> Build warnings about missing ENDBR around making SEAMCALLs[*] were
> observed when using some randconfig[1] to build today's Linus's tree.
>
> In the C code, the low level SEAMCALL assembly functions (__seamcall(),
> __seamcall_ret() and __seamcall_saved_ret()) are indirectly called via
> the common sc_retry() function:
>
> static inline u64 sc_retry(sc_func_t func, u64 fn,
> struct tdx_module_args *args)
> { ... }
>
> #define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
>
> It turns out compilers may not always be smart enough to figure out how
> to call those assembly functions directly.
Did you try something like so?
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 8b19294600c4..9662580c4b94 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -106,8 +106,8 @@ void tdx_init(void);
typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
-static inline u64 sc_retry(sc_func_t func, u64 fn,
- struct tdx_module_args *args)
+static __always_inline u64 sc_retry(const sc_func_t func, u64 fn,
+ struct tdx_module_args *args)
{
int retry = RDRAND_RETRY_LOOPS;
u64 ret;
On Thu, Jun 05, 2025 at 04:07:39PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 04, 2025 at 12:38:48PM +1200, Kai Huang wrote:
> > Build warnings about missing ENDBR around making SEAMCALLs[*] were
> > observed when using some randconfig[1] to build today's Linus's tree.
> >
> > In the C code, the low level SEAMCALL assembly functions (__seamcall(),
> > __seamcall_ret() and __seamcall_saved_ret()) are indirectly called via
> > the common sc_retry() function:
> >
> > static inline u64 sc_retry(sc_func_t func, u64 fn,
> > struct tdx_module_args *args)
> > { ... }
> >
> > #define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
> >
> > It turns out compilers may not always be smart enough to figure out how
> > to call those assembly functions directly.
>
> Did you try something like so?
This seems to cure things for me.
---
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 4a1922ec80cf..e8cd717f4f0f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -100,8 +100,8 @@ void tdx_init(void);
typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
-static inline u64 sc_retry(sc_func_t func, u64 fn,
- struct tdx_module_args *args)
+static __always_inline u64 sc_retry(const sc_func_t func, u64 fn,
+ struct tdx_module_args *args)
{
int retry = RDRAND_RETRY_LOOPS;
u64 ret;
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 7fdb37387886..ed87eae7b414 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -69,7 +69,7 @@ static inline void seamcall_err_ret(u64 fn, u64 err,
args->r9, args->r10, args->r11);
}
-static inline int sc_retry_prerr(sc_func_t func, sc_err_func_t err_func,
+static __always_inline int sc_retry_prerr(const sc_func_t func, sc_err_func_t err_func,
u64 fn, struct tdx_module_args *args)
{
u64 sret = sc_retry(func, fn, args);
On 6/5/25 07:21, Peter Zijlstra wrote:
> -static inline u64 sc_retry(sc_func_t func, u64 fn,
> - struct tdx_module_args *args)
> +static __always_inline u64 sc_retry(const sc_func_t func, u64 fn,
> + struct tdx_module_args *args)
> {
> int retry = RDRAND_RETRY_LOOPS;
Practically, I can see how this works. If the compiler doesn't inline
sc_retry(), it stops being able to guarantee that the function pointer
value is known.
My only worry is that the compiler decides to be stupid for some other
reason, it could start generating indirect calls again.
Are we confident that __always_inline will keep the compiler from
generating indirect calls?
On Thu, Jun 05, 2025 at 07:54:48AM -0700, Dave Hansen wrote:
> On 6/5/25 07:21, Peter Zijlstra wrote:
> > -static inline u64 sc_retry(sc_func_t func, u64 fn,
> > - struct tdx_module_args *args)
> > +static __always_inline u64 sc_retry(const sc_func_t func, u64 fn,
> > + struct tdx_module_args *args)
> > {
> > int retry = RDRAND_RETRY_LOOPS;
>
> Practically, I can see how this works. If the compiler doesn't inline
> sc_retry(), it stops being able to guarantee that the function pointer
> value is known.
>
> My only worry is that the compiler decides to be stupid for some other
> reason, it could start generating indirect calls again.
>
> Are we confident that __always_inline will keep the compiler from
> generating indirect calls?
Fairly sure. I've used this elsewhere too.
Also, if it ever decides to be that stupid, I'm going to log a bug and
call it broken.
On Thu, 2025-06-05 at 16:59 +0200, Peter Zijlstra wrote:
> On Thu, Jun 05, 2025 at 07:54:48AM -0700, Dave Hansen wrote:
> > On 6/5/25 07:21, Peter Zijlstra wrote:
> > > -static inline u64 sc_retry(sc_func_t func, u64 fn,
> > > - struct tdx_module_args *args)
> > > +static __always_inline u64 sc_retry(const sc_func_t func, u64 fn,
> > > + struct tdx_module_args *args)
> > > {
> > > int retry = RDRAND_RETRY_LOOPS;
> >
> > Practically, I can see how this works. If the compiler doesn't inline
> > sc_retry(), it stops being able to guarantee that the function pointer
> > value is known.
> >
> > My only worry is that the compiler decides to be stupid for some other
> > reason, it could start generating indirect calls again.
> >
> > Are we confident that __always_inline will keep the compiler from
> > generating indirect calls?
>
> Fairly sure. I've used this elsewhere too.
>
> Also, if it ever decides to be that stupid, I'm going to log a bug and
> call it broken.
Thanks Peter for helping.
I verified Peter's change, and I confirm that fixes those warnings. Both
gcc and clang (with CFI) build successfully w/o those warnings.
Hi Dave,
Just wondering are you happy with Peter's change?
On 6/5/25 16:20, Huang, Kai wrote: > Just wondering are you happy with Peter's change? Yes, it's fine with me.
On 6/3/25 17:38, Kai Huang wrote:
> Build warnings about missing ENDBR around making SEAMCALLs[*] were
> observed when using some randconfig[1] to build today's Linus's tree.
What actually caused it? CONFIG_CC_OPTIMIZE_FOR_SIZE?
> In the C code, the low level SEAMCALL assembly functions (__seamcall(),
> __seamcall_ret() and __seamcall_saved_ret()) are indirectly called via
> the common sc_retry() function:
>
> static inline u64 sc_retry(sc_func_t func, u64 fn,
> struct tdx_module_args *args)
> { ... }
>
> #define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
>
> It turns out compilers may not always be smart enough to figure out how
> to call those assembly functions directly.
So, reading this, it didn't quite click into my brain that you were
referring to "direct call" and "indirect call" *instructions*.
I think I probably would have talked about "function pointers" because
that's what you do in C. A function pointer mostly means an indirect
call. But not always, like was intended here.
> The disassembly of the vmlinux built from the aforementioned config
> confirms that __seamcall*() are indirectly called:
>
> <sc_retry>:
> ......
>
> 4c 89 ee mov %r13,%rsi
> 4c 89 e7 mov %r12,%rdi
> e8 35 8c 7d 01 call ffffffff82b3e220 <__pi___x86_indirect_thunk_rbp>
> 4c 39 f0 cmp %r14,%rax
>
> In this case ENDBR is needed at the beginning of __seamcall*().
>
> Change SYM_FUNC_START() to SYM_TYPED_FUNC_START() for __seamcall*() to
> add ENDBR to them.
... and why is this? How do we know this is the correct fix? Show your
work, please.
> When the compiler can generate direct call for __seamcall*(), the
> additional ENDBR is safe since it has no impact to directly called
> functions.
Right. Direct calls are always OK. Indirect call targets need to be
handled specially.
> When kernel IBT was added to the kernel, initially the SYM_FUNC_START()
> had the ENDBR added in commit
>
> c4691712b546 ("x86/linkage: Add ENDBR to SYM_FUNC_START*()")
>
> However when the commit
>
> 582077c94052 ("x86/cfi: Clean up linkage")
>
> removed the ENDBR from the SYM_FUNC_START() and added it to the
> SYM_TYPED_FUNC_START(), it didn't touch the SEAMCALL assembly.
I'm not sure why we need the history lesson here.
> [*] Aforementioned build warning:
>
> vmlinux.o: warning: objtool: try_init_module_global+0x5d: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: read_sys_metadata_field+0x4a: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: do_global_key_config+0x36: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_phymem_page_reclaim+0x71: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_phymem_cache_wb+0x41: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_tdr+0x95: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdx_cpu_enable+0x7b: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: init_tdmr+0x59: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: config_tdx_module.constprop.0+0x19d: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_vp_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_vp_init+0x76: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_vp_wr+0x87: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_vp_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_vp_flush+0x4c: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_vp_create+0x85: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mng_create+0x73: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mem_page_aug+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mem_sept_add+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mem_page_add+0xce: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mng_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mem_page_remove+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mem_track+0x4c: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mng_init+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mng_key_freeid+0x4c: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mng_vpflushdone+0x4c: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mr_finalize+0x4c: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mr_extend+0x77: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mng_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_mng_key_config+0x4c: relocation to !ENDBR: __seamcall+0x0
> vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
> vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_hkid+0x7d: relocation to !ENDBR: __seamcall+0x0
One or two of those would have sufficed...
> diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
> index 6854c52c374b..637226ae935d 100644
> --- a/arch/x86/virt/vmx/tdx/seamcall.S
> +++ b/arch/x86/virt/vmx/tdx/seamcall.S
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #include <linux/linkage.h>
> +#include <linux/cfi_types.h>
> #include <asm/frame.h>
>
> #include "tdxcall.S"
> @@ -18,7 +19,7 @@
> * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
> * fails, or the completion status of the SEAMCALL leaf function.
> */
> -SYM_FUNC_START(__seamcall)
> +SYM_TYPED_FUNC_START(__seamcall)
> TDX_MODULE_CALL host=1
> SYM_FUNC_END(__seamcall)
Personally, I'd add a _bit_ to these comments to mention that these
functions can be called via an indirect call.
Circling back around now that I've divined out what is happening here...
Is this even really about ENDBR at all? I thought that clang has some
CFI checking that's completely independent from CET/IBT and thus ENDBR.
Wouldn't this also break with that?
At the risk of giving a man a fish... Isn't this a much more coherent
changelog at about 1/4 the size?
--
Subject: x86/virt/tdx: Annotate TDX assembly to allow indirect calls
A TDX helper function (sc_retry()) passes around function pointers to
assembly functions. Normally, the compiler realizes that the function
pointer targets are completely static, can be resolved at compile time,
and generates direct call instructions.
But, other times (like when CONFIG_CC_OPTIMIZE_FOR_SIZE=y), the compiler
will instead generate indirect call instructions.
Indirect calls to assembly functions require special annotation so that
both hardware and software implementations of Control Flow Integrity
mechanisms can work correctly.
The TDX functions are declared as if they are only called directly (via
SYM_FUNC_START). Move them over to another macro (SYM_TYPED_FUNC_START)
which will annotate them as being called indirectly (see
include/linux/cfi_types.h).
This was found through randconfig testing, presumably setting
CONFIG_CC_OPTIMIZE_FOR_SIZE=1 when objtool spewed a bunch of these:
vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to
!ENDBR: __seamcall_ret+0x0
On Wed, 2025-06-04 at 12:46 -0700, Dave Hansen wrote: > Circling back around now that I've divined out what is happening here... > Is this even really about ENDBR at all? I thought that clang has some > CFI checking that's completely independent from CET/IBT and thus ENDBR. > Wouldn't this also break with that? Hi Dave, With your reminder on clang and CFI, I did more test on this patch, and unfortunately I found building with clang and CONFIG_CFI_CLANG=y generates below build error: ld.lld: error: undefined symbol: __kcfi_typeid___seamcall_saved_ret >>> referenced by usercopy_64.c >>> vmlinux.o:(__cfi___seamcall_saved_ret) It is because currently __seaamcall_saved_ret() is only called and is directly called by tdh_vp_enter(), but not via sc_retry(). Making tdh_vp_enter() call __seamcall_saved_ret() via sc_retry() fixes the error, but I am not sure it's a good idea since tdh_vp_enter() is in performance critical path (although the additional cost of sc_retry() is minor comparing to the SEAMCALL itself). I think we can keep using SYM_FUNC_START for __seamcall_saved_ret() but only change to using SYM_TYPED_FUNC_START for the other two. I've sent out v2 based on above by replying to this v1 patch. Appreciate if you can review. Thanks for your time.
On Wed, 2025-06-04 at 12:46 -0700, Dave Hansen wrote:
> On 6/3/25 17:38, Kai Huang wrote:
> > Build warnings about missing ENDBR around making SEAMCALLs[*] were
> > observed when using some randconfig[1] to build today's Linus's tree.
>
> What actually caused it? CONFIG_CC_OPTIMIZE_FOR_SIZE?
Yes. I just verified that building with "-O2" doesn't have this warning but
"-Os" does.
I spent hours but didn't figure this out. So, thank you!
>
> > In the C code, the low level SEAMCALL assembly functions (__seamcall(),
> > __seamcall_ret() and __seamcall_saved_ret()) are indirectly called via
> > the common sc_retry() function:
> >
> > static inline u64 sc_retry(sc_func_t func, u64 fn,
> > struct tdx_module_args *args)
> > { ... }
> >
> > #define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
> >
> > It turns out compilers may not always be smart enough to figure out how
> > to call those assembly functions directly.
>
> So, reading this, it didn't quite click into my brain that you were
> referring to "direct call" and "indirect call" *instructions*.
>
> I think I probably would have talked about "function pointers" because
> that's what you do in C. A function pointer mostly means an indirect
> call. But not always, like was intended here.
I was thinking the function pointers are passed as function argument when
sc_retry() gets called, so maybe in some cases that I don't know the
compiler just cannot figure out exactly what those function pointers are.
>
> > The disassembly of the vmlinux built from the aforementioned config
> > confirms that __seamcall*() are indirectly called:
> >
> > <sc_retry>:
> > ......
> >
> > 4c 89 ee mov %r13,%rsi
> > 4c 89 e7 mov %r12,%rdi
> > e8 35 8c 7d 01 call ffffffff82b3e220 <__pi___x86_indirect_thunk_rbp>
> > 4c 39 f0 cmp %r14,%rax
> >
> > In this case ENDBR is needed at the beginning of __seamcall*().
> >
> > Change SYM_FUNC_START() to SYM_TYPED_FUNC_START() for __seamcall*() to
> > add ENDBR to them.
>
> ... and why is this? How do we know this is the correct fix? Show your
> work, please.
I don't know details of how CFI works, but my understanding is
SYM_TYPED_FUNC_START() is the standard one for indirectly called globals
with CFI handled:
<asm/linkage.h>:
/* SYM_TYPED_FUNC_START -- use for indirectly called globals, w/ CFI type */
#define SYM_TYPED_FUNC_START(name) \
SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_F_ALIGN) \
ENDBR
And the SYM_TYPED_START() handles CFI for CLANG:
...
#define __CFI_TYPE(name) \
SYM_START(__cfi_##name, SYM_L_LOCAL, SYM_A_NONE) \
CFI_PRE_PADDING \
.byte 0xb8 ASM_NL \
.long __kcfi_typeid_##name ASM_NL \
CFI_POST_PADDING \
SYM_FUNC_END(__cfi_##name)
...
<linux/cfi_types.h>:
...
#define SYM_TYPED_ENTRY(name, linkage, align...) \
linkage(name) ASM_NL \
align ASM_NL \
__CFI_TYPE(name) ASM_NL \
name:
#define SYM_TYPED_START(name, linkage, align...) \
SYM_TYPED_ENTRY(name, linkage, align)
...
The ENDBR right after "SYM_TYPED_START()" is for IBT (which IIUC is
independent to CFI CLANG as you mentioned below).
But I confess I don't know details about CFI so perhaps Peter could help to
confirm?
>
> > When the compiler can generate direct call for __seamcall*(), the
> > additional ENDBR is safe since it has no impact to directly called
> > functions.
>
> Right. Direct calls are always OK. Indirect call targets need to be
> handled specially.
>
> > When kernel IBT was added to the kernel, initially the SYM_FUNC_START()
> > had the ENDBR added in commit
> >
> > c4691712b546 ("x86/linkage: Add ENDBR to SYM_FUNC_START*()")
> >
> > However when the commit
> >
> > 582077c94052 ("x86/cfi: Clean up linkage")
> >
> > removed the ENDBR from the SYM_FUNC_START() and added it to the
> > SYM_TYPED_FUNC_START(), it didn't touch the SEAMCALL assembly.
>
> I'm not sure why we need the history lesson here.
I dig the history mainly because I thought we might need a "Fixes" tag.
And I then conclude the commit 582077c94052 ("x86/cfi: Clean up linkage")
seems to be the one broke it. So to justify the Fixes tag, I added the
history here.
>
> > [*] Aforementioned build warning:
> >
> > vmlinux.o: warning: objtool: try_init_module_global+0x5d: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: read_sys_metadata_field+0x4a: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: do_global_key_config+0x36: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_page_reclaim+0x71: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_cache_wb+0x41: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_tdr+0x95: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdx_cpu_enable+0x7b: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: init_tdmr+0x59: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: config_tdx_module.constprop.0+0x19d: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_init+0x76: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_wr+0x87: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_vp_flush+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_vp_create+0x85: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mng_create+0x73: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mem_page_aug+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mem_sept_add+0xb4: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mem_page_add+0xce: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_addcx+0x91: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mem_page_remove+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mem_track+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mng_init+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_key_freeid+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mng_vpflushdone+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mr_finalize+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mr_extend+0x77: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_rd+0x6d: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_mng_key_config+0x4c: relocation to !ENDBR: __seamcall+0x0
> > vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to !ENDBR: __seamcall_ret+0x0
> > vmlinux.o: warning: objtool: tdh_phymem_page_wbinvd_hkid+0x7d: relocation to !ENDBR: __seamcall+0x0
>
> One or two of those would have sufficed...
Will keep this in mind. Thanks.
>
> > diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
> > index 6854c52c374b..637226ae935d 100644
> > --- a/arch/x86/virt/vmx/tdx/seamcall.S
> > +++ b/arch/x86/virt/vmx/tdx/seamcall.S
> > @@ -1,5 +1,6 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > #include <linux/linkage.h>
> > +#include <linux/cfi_types.h>
> > #include <asm/frame.h>
> >
> > #include "tdxcall.S"
> > @@ -18,7 +19,7 @@
> > * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
> > * fails, or the completion status of the SEAMCALL leaf function.
> > */
> > -SYM_FUNC_START(__seamcall)
> > +SYM_TYPED_FUNC_START(__seamcall)
> > TDX_MODULE_CALL host=1
> > SYM_FUNC_END(__seamcall)
> Personally, I'd add a _bit_ to these comments to mention that these
> functions can be called via an indirect call.
There are three comments since we have three helpers.
I am not sure duplicating the "indirect call" in those three comments is
good idea? Or perhaps we can just add an overall comment at the beginning
of those three helpers like below?
diff --git a/arch/x86/virt/vmx/tdx/seamcall.S
b/arch/x86/virt/vmx/tdx/seamcall.S
index 6854c52c374b..a78e46ec4a12 100644
--- a/arch/x86/virt/vmx/tdx/seamcall.S
+++ b/arch/x86/virt/vmx/tdx/seamcall.S
@@ -4,6 +4,11 @@
#include "tdxcall.S"
+/*
+ * Use SYM_TYPED_FUNC_START() for those functions since they can be
+ * called via an indirect call.
+ */
+
/*
* __seamcall() - Host-side interface functions to SEAM software
* (the P-SEAMLDR or the TDX module).
>
> Circling back around now that I've divined out what is happening here...
> Is this even really about ENDBR at all? I thought that clang has some
> CFI checking that's completely independent from CET/IBT and thus ENDBR.
> Wouldn't this also break with that?
It doesn't seem this would break anything to me, as replied above.
But it would be great if Peter can jump in to confirm.
>
> At the risk of giving a man a fish...
>
Thanks for the fish. :-)
> Isn't this a much more coherent
> changelog at about 1/4 the size?
Yes it's more concise. I'll use it in next version.
Could you also let me know whether I should keep the Fixes tag, and the
history in the changelog?
Thanks.
>
> --
>
> Subject: x86/virt/tdx: Annotate TDX assembly to allow indirect calls
>
> A TDX helper function (sc_retry()) passes around function pointers to
> assembly functions. Normally, the compiler realizes that the function
> pointer targets are completely static, can be resolved at compile time,
> and generates direct call instructions.
>
> But, other times (like when CONFIG_CC_OPTIMIZE_FOR_SIZE=y), the compiler
> will instead generate indirect call instructions.
>
> Indirect calls to assembly functions require special annotation so that
> both hardware and software implementations of Control Flow Integrity
> mechanisms can work correctly.
>
> The TDX functions are declared as if they are only called directly (via
> SYM_FUNC_START). Move them over to another macro (SYM_TYPED_FUNC_START)
> which will annotate them as being called indirectly (see
> include/linux/cfi_types.h).
>
> This was found through randconfig testing, presumably setting
> CONFIG_CC_OPTIMIZE_FOR_SIZE=1 when objtool spewed a bunch of these:
>
> vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to
> !ENDBR: __seamcall_ret+0x0
>
On 6/4/25 16:16, Huang, Kai wrote: > Could you also let me know whether I should keep the Fixes tag, and the > history in the changelog? Yeah, I think you should keep the Fixes: tag, unless I'm missing something. As for the history lesson, you can just say that you think that the TDX code was OK when merged, but wasn't converted with the rest of the Fixes: commit. That's all you need.
On Wed, 2025-06-04 at 16:23 -0700, Dave Hansen wrote: > On 6/4/25 16:16, Huang, Kai wrote: > > Could you also let me know whether I should keep the Fixes tag, and the > > history in the changelog? > > Yeah, I think you should keep the Fixes: tag, unless I'm missing something. > > As for the history lesson, you can just say that you think that the TDX > code was OK when merged, but wasn't converted with the rest of the > Fixes: commit. That's all you need. Will do. Thanks for quick feedback.
On Wed, 2025-06-04 at 23:27 +0000, Huang, Kai wrote:
> On Wed, 2025-06-04 at 16:23 -0700, Dave Hansen wrote:
> > On 6/4/25 16:16, Huang, Kai wrote:
> > > Could you also let me know whether I should keep the Fixes tag, and the
> > > history in the changelog?
> >
> > Yeah, I think you should keep the Fixes: tag, unless I'm missing something.
> >
> > As for the history lesson, you can just say that you think that the TDX
> > code was OK when merged, but wasn't converted with the rest of the
> > Fixes: commit. That's all you need.
>
> Will do. Thanks for quick feedback.
After second thought, I think I was wrong about the target commit to Fixes,
because at that time I didn't consider CFI on CLANG.
When TDX code was merged, the SYM_FUNC_START() and SYM_TYPED_FUNC_START()
were already like this:
/* SYM_TYPED_FUNC_START -- use for indirectly called globals, w/ CFI type */
#define SYM_TYPED_FUNC_START(name) \
SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_F_ALIGN) \
ENDBR
/* SYM_FUNC_START -- use for global functions */
#define SYM_FUNC_START(name) \
SYM_START(name, SYM_L_GLOBAL, SYM_F_ALIGN) \
ENDBR
So the TDX code was supposed to use SYM_TYPED_FUNC_START() at the first
place when sc_retry() logic was introduced in commit
1e66a7e27539 ("x86/virt/tdx: Handle SEAMCALL no entropy error in common
code")
Note, the seamcall*() assembly was introduced at an earlier commit
c33621b4c5ad ("x86/virt/tdx: Wire up basic SEAMCALL functions")
But arguably that commit didn't know SEAMCALL helpers could be called
indirectly.
So, thanks for bringing up the CFI.
And if you don't have further comments, I think I will add below as Fixes
commit w/o mentioning history in the changelog.
Fixes: 1e66a7e27539 ("x86/virt/tdx: Handle SEAMCALL no entropy error in
common code")
A TDX helper function (sc_retry()) passes around function pointers to
assembly functions. Normally, the compiler realizes that the function
pointer targets are completely static, can be resolved at compile time,
and generates direct call instructions.
But, other times (like when CONFIG_CC_OPTIMIZE_FOR_SIZE=y), the compiler
will instead generate indirect call instructions.
Indirect calls to assembly functions require special annotation so that
both hardware and software implementations of Control Flow Integrity
mechanisms can work correctly.
The TDX functions are declared as if they are only called directly (via
SYM_FUNC_START). Move them over to another macro (SYM_TYPED_FUNC_START)
which will annotate them as being called indirectly (see
include/linux/cfi_types.h).
This was found through randconfig testing, presumably setting
CONFIG_CC_OPTIMIZE_FOR_SIZE=1 when objtool spewed a bunch of these:
vmlinux.o: warning: objtool: tdh_mem_range_block+0x7e: relocation to
!ENDBR: __seamcall_ret+0x0
Note that currently only __seamcall() and __seamcall_ret() are called
via sc_retry(). __seamcall_saved_ret() is only called and is directly
called by tdh_vp_enter() since this SEAMCALL doesn't return "running out
of entropy" error which sc_retry() handles.
Using SYM_TYPED_FUNC_START for __seamcall_saved_ret() results in build
error when building the kernel with clang and CONFIG_CFI_CLANG=y:
ld.lld: error: undefined symbol: __kcfi_typeid___seamcall_saved_ret
>>> referenced by usercopy_64.c
>>> vmlinux.o:(__cfi___seamcall_saved_ret)
Making tdh_vp_enter() call __seamcall_saved_ret() via sc_retry() fixes
the error, but don't do this way since tdh_vp_enter() is in performance
critical path. Instead, keep using SYM_FUNC_START for it but only move
to SYM_TYPED_FUNC_START for the other two.
While on it, remove seamcall_saved_ret() since it is not used.
Fixes: 1e66a7e27539 ("x86/virt/tdx: Handle SEAMCALL no entropy error in common code")
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
arch/x86/include/asm/tdx.h | 1 -
arch/x86/virt/vmx/tdx/seamcall.S | 10 ++++++++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 8b19294600c4..c6ec8601bbb8 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -121,7 +121,6 @@ static inline u64 sc_retry(sc_func_t func, u64 fn,
#define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
#define seamcall_ret(_fn, _args) sc_retry(__seamcall_ret, (_fn), (_args))
-#define seamcall_saved_ret(_fn, _args) sc_retry(__seamcall_saved_ret, (_fn), (_args))
int tdx_cpu_enable(void);
int tdx_enable(void);
const char *tdx_dump_mce_info(struct mce *m);
diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
index 6854c52c374b..552495761b36 100644
--- a/arch/x86/virt/vmx/tdx/seamcall.S
+++ b/arch/x86/virt/vmx/tdx/seamcall.S
@@ -1,9 +1,15 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include <linux/linkage.h>
+#include <linux/cfi_types.h>
#include <asm/frame.h>
#include "tdxcall.S"
+/*
+ * Use SYM_TYPED_FUNC_START() for __seamcall() and __seamcall_ret()
+ * since they can be called via an indirect call.
+ */
+
/*
* __seamcall() - Host-side interface functions to SEAM software
* (the P-SEAMLDR or the TDX module).
@@ -18,7 +24,7 @@
* Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
* fails, or the completion status of the SEAMCALL leaf function.
*/
-SYM_FUNC_START(__seamcall)
+SYM_TYPED_FUNC_START(__seamcall)
TDX_MODULE_CALL host=1
SYM_FUNC_END(__seamcall)
@@ -37,7 +43,7 @@ SYM_FUNC_END(__seamcall)
* Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself
* fails, or the completion status of the SEAMCALL leaf function.
*/
-SYM_FUNC_START(__seamcall_ret)
+SYM_TYPED_FUNC_START(__seamcall_ret)
TDX_MODULE_CALL host=1 ret=1
SYM_FUNC_END(__seamcall_ret)
base-commit: ec7714e4947909190ffb3041a03311a975350fe0
--
2.49.0© 2016 - 2025 Red Hat, Inc.