[PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[Paolo Bonzini]

In some cases tdx_tdvpr_pa() is not fully inlined into tdh_vp_enter(), which
causes the following warning:

  vmlinux.o: warning: objtool: tdh_vp_enter+0x8: call to tdx_tdvpr_pa() leaves .noinstr.text section

This happens if the compiler considers tdx_tdvpr_pa() to be "large", for example
because CONFIG_SPARSEMEM adds two function calls to page_to_section() and
__section_mem_map_addr():

({      const struct page *__pg = (pg);                         \
        int __sec = page_to_section(__pg);                      \
        (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec)));
\
})

Because exiting the noinstr section is a no-no, just mark tdh_vp_enter() for
full inlining.

Reported-by: kernel test robot <lkp@intel.com>
Analyzed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202505240530.5KktQ5mX-lkp@intel.com/
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/virt/vmx/tdx/tdx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index f5e2a937c1e7..2457d13c3f9e 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1517,7 +1517,7 @@ static void tdx_clflush_page(struct page *page)
 	clflush_cache_range(page_to_virt(page), PAGE_SIZE);
 }
 
-noinstr u64 tdh_vp_enter(struct tdx_vp *td, struct tdx_module_args *args)
+noinstr __flatten u64 tdh_vp_enter(struct tdx_vp *td, struct tdx_module_args *args)
 {
 	args->rcx = tdx_tdvpr_pa(td);
 
-- 
2.43.5

Re: [PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[Huang, Kai]

On Mon, 2025-05-26 at 16:45 -0400, Paolo Bonzini wrote:
> In some cases tdx_tdvpr_pa() is not fully inlined into tdh_vp_enter(), which
> causes the following warning:
> 
>   vmlinux.o: warning: objtool: tdh_vp_enter+0x8: call to tdx_tdvpr_pa() leaves .noinstr.text section
> 
> This happens if the compiler considers tdx_tdvpr_pa() to be "large", for example
> because CONFIG_SPARSEMEM adds two function calls to page_to_section() and
> __section_mem_map_addr():
> 
> ({      const struct page *__pg = (pg);                         \
>         int __sec = page_to_section(__pg);                      \
>         (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec)));
> \
> })

Just FYI the above warning can also be triggered when CONFIG_SPARSEMEM_VMEMMAP=y
(and CONFIG_SPARSEMEM=y as well), in which case the __page_to_pfn() is simply:

  #define __page_to_pfn(page)     (unsigned long)((page) - vmemmap)

The function call to page_to_section() and __section_mem_map_addr() only happens
when CONFIG_SPARSEMEM_VMEMMAP=n while CONFIG_SPARSEMEM=y.

> 
> Because exiting the noinstr section is a no-no, just mark tdh_vp_enter() for
> full inlining.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Analyzed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202505240530.5KktQ5mX-lkp@intel.com/
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index f5e2a937c1e7..2457d13c3f9e 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1517,7 +1517,7 @@ static void tdx_clflush_page(struct page *page)
>  	clflush_cache_range(page_to_virt(page), PAGE_SIZE);
>  }
>  
> -noinstr u64 tdh_vp_enter(struct tdx_vp *td, struct tdx_module_args *args)
> +noinstr __flatten u64 tdh_vp_enter(struct tdx_vp *td, struct tdx_module_args *args)
>  {
>  	args->rcx = tdx_tdvpr_pa(td);
>  

I didn't know this __flatten attribute.  Thanks.

I did some test and can confirm this patch can silence the warning mentioned in
the changelog.

However, with this patch applied, I also tested the case that
CONFIG_SPARSEMEM_VMEMMAP=n and CONFIG_SPARSEMEM=y [1], but I still got:

vmlinux.o: warning: objtool: tdh_vp_enter+0x10: call to page_to_section() leaves
.noinstr.text section

Not sure why, but it seems __flatten failed to work as expected, as least
recursively.

Then I did some dig.  The gcc doc [2] explicitly says it will do it recursively:

  flatten
  
  Generally, inlining into a function is limited. For a function marked with 
  this attribute, every call inside this function is inlined including the calls
  such inlining introduces to the function (but not recursive calls to the 
  function itself), if possible. 

But the clang doc [3] doesn't explicitly say it:

  flatten

  The flatten attribute causes calls within the attributed function to be 
  inlined unless it is impossible to do so, for example if the body of the 
  callee is unavailable or if the callee has the noinline attribute.

One "AI Overview" provided by google also says below:

  Compiler Behavior:
  While GCC supports recursive inlining with flatten, other compilers like  
  Clang might only perform a single level of inlining.

So it seems __flatten may not work as expected in clang.  But I guess we can
blame clang on this, so not sure whether it matters.

[1]: after fixing the build error I mentioned to you in the last reply.
[2]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
[3]: https://clang.llvm.org/docs/AttributeReference.html

Re: [PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[Huang, Kai]

On Mon, 2025-05-26 at 23:10 +0000, Huang, Kai wrote:
> On Mon, 2025-05-26 at 16:45 -0400, Paolo Bonzini wrote:
> > In some cases tdx_tdvpr_pa() is not fully inlined into tdh_vp_enter(), which
> > causes the following warning:
> > 
> >   vmlinux.o: warning: objtool: tdh_vp_enter+0x8: call to tdx_tdvpr_pa() leaves .noinstr.text section
> > 
> > This happens if the compiler considers tdx_tdvpr_pa() to be "large", for example
> > because CONFIG_SPARSEMEM adds two function calls to page_to_section() and
> > __section_mem_map_addr():
> > 
> > ({      const struct page *__pg = (pg);                         \
> >         int __sec = page_to_section(__pg);                      \
> >         (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec)));
> > \
> > })
> 
> Just FYI the above warning can also be triggered when CONFIG_SPARSEMEM_VMEMMAP=y
> (and CONFIG_SPARSEMEM=y as well), in which case the __page_to_pfn() is simply:
> 
>   #define __page_to_pfn(page)     (unsigned long)((page) - vmemmap)
> 
> The function call to page_to_section() and __section_mem_map_addr() only happens
> when CONFIG_SPARSEMEM_VMEMMAP=n while CONFIG_SPARSEMEM=y.
> 

[...]


> 
> I did some test and can confirm this patch can silence the warning mentioned in
> the changelog.
> 
> However, with this patch applied, I also tested the case that
> CONFIG_SPARSEMEM_VMEMMAP=n and CONFIG_SPARSEMEM=y [1], but I still got:
> 
> vmlinux.o: warning: objtool: tdh_vp_enter+0x10: call to page_to_section() leaves
> .noinstr.text section
> 
> Not sure why, but it seems __flatten failed to work as expected, as least
> recursively.
> 

I found a recently merged patch from Kirill has made CONFIG_SPARSEMEM_VMEMAP
always true on x86_64:

https://lore.kernel.org/lkml/174748683804.406.11521945321481191771.tip-bot2@tip-bot2/

So we don't need to worry about the case that CONFIG_SPARSEMEM_VMEMMAP=n and
CONFIG_SPARSEMEM=y.

The changelog could be simplified/updated though I think (since seems there's no
need to mention the "large" case now).

So:

Tested-by: Kai Huang <kai.huang@intel.com>
Acked-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[Sean Christopherson]

On Mon, May 26, 2025, Paolo Bonzini wrote:
> In some cases tdx_tdvpr_pa() is not fully inlined into tdh_vp_enter(), which
> causes the following warning:
> 
>   vmlinux.o: warning: objtool: tdh_vp_enter+0x8: call to tdx_tdvpr_pa() leaves .noinstr.text section
> 
> This happens if the compiler considers tdx_tdvpr_pa() to be "large", for example
> because CONFIG_SPARSEMEM adds two function calls to page_to_section() and
> __section_mem_map_addr():
> 
> ({      const struct page *__pg = (pg);                         \
>         int __sec = page_to_section(__pg);                      \
>         (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec)));
> \
> })
> 
> Because exiting the noinstr section is a no-no, just mark tdh_vp_enter() for
> full inlining.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Analyzed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202505240530.5KktQ5mX-lkp@intel.com/
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index f5e2a937c1e7..2457d13c3f9e 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1517,7 +1517,7 @@ static void tdx_clflush_page(struct page *page)
>  	clflush_cache_range(page_to_virt(page), PAGE_SIZE);
>  }
>  
> -noinstr u64 tdh_vp_enter(struct tdx_vp *td, struct tdx_module_args *args)
> +noinstr __flatten u64 tdh_vp_enter(struct tdx_vp *td, struct tdx_module_args *args)
>  {
>  	args->rcx = tdx_tdvpr_pa(td);

The "standard" kernel way of handling this it to mark the offending helper
__always_inline, i.e. tag tdx_tdvpr_pa() __always_inline.  Ditto for tdx_tdr_pa().
Especially since they're already "inline".

Re: [PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[Edgecombe, Rick P]

On Tue, 2025-05-27 at 13:54 +0000, Sean Christopherson wrote:
> The "standard" kernel way of handling this it to mark the offending helper
> __always_inline, i.e. tag tdx_tdvpr_pa() __always_inline.
> 

It looks like __flatten was added after a very similar situation:
https://lore.kernel.org/lkml/CAK8P3a2ZWfNeXKSm8K_SUhhwkor17jFo3xApLXjzfPqX0eUDUA@mail.gmail.com/#t

Since flatten gives the inline decision to the caller instead of the callee,
clang could have the option to keep a non-inline version of tdx_tdvpr_pa() for
whatever reasoning it has. The non-standard behavior around recursive inlining
is unfortunate, but we don't need it here.

The downside is that we would not learn if some code changed in page_to_phys()
and we ended up pulling in some big piece of code for the recursive behavior.

Overall I like the flatten version, but this works too:

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 5699dfe500d9..371b4423a639 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1501,7 +1501,7 @@ static inline u64 tdx_tdr_pa(struct tdx_td *td)
        return page_to_phys(td->tdr_page);
 }
 
-static inline u64 tdx_tdvpr_pa(struct tdx_vp *td)
+static __always_inline u64 tdx_tdvpr_pa(struct tdx_vp *td)
 {
        return page_to_phys(td->tdvpr_page);
 }


>   Ditto for tdx_tdr_pa().
> Especially since they're already "inline".

I don't see why tdx_tdr_pa() is required to be inlined. Why force the compiler?

Re: [PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

[Sean Christopherson]

On Tue, May 27, 2025, Rick P Edgecombe wrote:
> On Tue, 2025-05-27 at 13:54 +0000, Sean Christopherson wrote:
> > The "standard" kernel way of handling this it to mark the offending helper
> > __always_inline, i.e. tag tdx_tdvpr_pa() __always_inline.
> > 
> 
> It looks like __flatten was added after a very similar situation:
> https://lore.kernel.org/lkml/CAK8P3a2ZWfNeXKSm8K_SUhhwkor17jFo3xApLXjzfPqX0eUDUA@mail.gmail.com/#t
> 
> Since flatten gives the inline decision to the caller instead of the callee,
> clang could have the option to keep a non-inline version of tdx_tdvpr_pa() for
> whatever reasoning it has. The non-standard behavior around recursive inlining
> is unfortunate, but we don't need it here.
> 
> The downside is that we would not learn if some code changed in page_to_phys()
> and we ended up pulling in some big piece of code for the recursive behavior.

It's not just recursive behavior, it's any new code that comes along.  It's not
likely to happen in this scenario, but in general it's not at all uncommon for a
noinstr function to gain new code.
 
It's also quite weird to "flatten" a function with an explicit call to assembly.

> Overall I like the flatten version, but this works too:

I'm not a fan of "flatten", IMO it's too big of a hammer for general use.

> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 5699dfe500d9..371b4423a639 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1501,7 +1501,7 @@ static inline u64 tdx_tdr_pa(struct tdx_td *td)
>         return page_to_phys(td->tdr_page);
>  }
>  
> -static inline u64 tdx_tdvpr_pa(struct tdx_vp *td)
> +static __always_inline u64 tdx_tdvpr_pa(struct tdx_vp *td)
>  {
>         return page_to_phys(td->tdvpr_page);
>  }
> 
> 
> >   Ditto for tdx_tdr_pa().
> > Especially since they're already "inline".
> 
> I don't see why tdx_tdr_pa() is required to be inlined. Why force the compiler?

Because tagging a local static function with "inline" is pointless, and looks
weird next a similar __always_inline version.  Either drop the "inline" or make
it __always_inline.  Modern compilers don't need an "inline" hint for something
like this (or pretty much anything).