linux-kernel - Re: [PATCH] x86/tdx: mark tdh_vp_enter() as _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aDYXV-sggmWmaUyJ@google.com>
Date: Tue, 27 May 2025 12:49:43 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Rick P Edgecombe <rick.p.edgecombe@...el.com>
Cc: "pbonzini@...hat.com" <pbonzini@...hat.com>, Xiaoyao Li <xiaoyao.li@...el.com>, 
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, lkp <lkp@...el.com>, 
	Kai Huang <kai.huang@...el.com>
Subject: Re: [PATCH] x86/tdx: mark tdh_vp_enter() as __flatten

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).