Message-ID: <5049e63e22c66aa0a97912417c29eff007468ab4.camel@intel.com>
Date: Fri, 27 Jun 2025 00:52:22 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "bp@...en8.de" <bp@...en8.de>,
	"Huang, Kai" <kai.huang@...el.com>, "peterz@...radead.org"
	<peterz@...radead.org>, "hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "thomas.lendacky@....com" <thomas.lendacky@....com>,
	"tglx@...utronix.de" <tglx@...utronix.de>
CC: "ashish.kalra@....com" <ashish.kalra@....com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "x86@...nel.org" <x86@...nel.org>, "sagis@...gle.com"
	<sagis@...gle.com>, "Chatre, Reinette" <reinette.chatre@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"Williams, Dan J" <dan.j.williams@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"nik.borisov@...e.com" <nik.borisov@...e.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>, "Chen, Farrah" <farrah.chen@...el.com>
Subject: Re: [PATCH v3 2/6] x86/virt/tdx: Mark memory cache state incoherent
 when making SEAMCALL

On Thu, 2025-06-26 at 23:36 +0000, Huang, Kai wrote:
> (I'll fix all wording comments above)
> 
> > > 
> > > v2 -> v3:
> > >  - Change to use __always_inline for do_seamcall() to avoid indirect
> > >    call instructions of making SEAMCALL.
> > 
> > How did this come about?
> 
> We had a "missing ENDBR" build warning recently got fixed, which was caused
> by compiler fails to inline the 'static inline sc_retry()'.  It got fixed by
> changing to __always_inline, so we need to use __always_inline here too
> otherwise the compiler may still refuse to inline it.

Oh, right.
> 
> See commit 0b3bc018e86a ("x86/virt/tdx: Avoid indirect calls to TDX assembly
> functions")
>  
> > 
> > >  - Remove the senstence "not all SEAMCALLs generate dirty cachelines of
> > >    TDX private memory but just treat all of them do." in changelog and
> > >    the code comment. -- Dave
> > > 
> > > ---
> > >  arch/x86/include/asm/tdx.h | 29 ++++++++++++++++++++++++++++-
> > >  1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > > index 7ddef3a69866..d4c624c69d7f 100644
> > > --- a/arch/x86/include/asm/tdx.h
> > > +++ b/arch/x86/include/asm/tdx.h
> > > @@ -102,10 +102,37 @@ u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
> > >  u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
> > >  void tdx_init(void);
> > >  
> > > +#include <linux/preempt.h>
> > >  #include <asm/archrandom.h>
> > > +#include <asm/processor.h>
> > >  
> > >  typedef u64 (*sc_func_t)(u64 fn, struct tdx_module_args *args);
> > >  
> > > +static __always_inline u64 do_seamcall(sc_func_t func, u64 fn,
> > > +				       struct tdx_module_args *args)
> > > +{
> > 
> > So now we have:
> > 
> > seamcall()
> > 	sc_retry()
> > 		do_seamcall()
> > 			__seamcall()
> > 
> > 
> > do_seamcall() is only called from sc_retry(). Why add yet another helper in the
> > stack? You could just build it into sc_retry().
> 
> It's just more readable if we have the do_seamcall().  It's always inlined
> anyway.

Don't you think that is a questionable chain of names? I was thinking that we
might want to do a future cleanup of all these wrappers. But I wondered if it
was one of those "least worse" options kind of things, and you already tried
something and threw your hands up. I think the existing layers are already
questionable. Which we don't need to cleanup for this series.

> 
> > 
> > Oh, and __seamcall_*() variety is called directly too, so skips the
> > do_seamcall() per-cpu var logic in those cases. So, maybe do_seamcall() is
> > needed, but it needs a better name considering where it would get called from.
> > 
> > These wrappers need an overhaul I think, but maybe for now just have
> > do_dirty_seamcall() which is called from tdh_vp_enter() and sc_retry().
> 
> Right.  I forgot TDH.VP.ENTER and TDH_PHYMEM_PAGE_RDMD are called directly
> using __seamcall*().
> 
> We can move preempt_disable()/enable() out of do_seamcall() to sc_retry()
> and instead add a lockdep_assert_preemption_disabled() there, and then
> change tdh_vp_enter() and paddr_is_tdx_private() to call do_seamcall()
> instead.

Can you play around with it and find a good fix? It needs to mark the per-cpu
var and not cause the inline warnings for tdh_vp_enter().

> 
> > 
> > Oh no, actually scratch that! The inline/flatten issue will happen again if we
> > add the per-cpu vars to tdh_vp_enter()... Which means we probably need to set
> > the per-cpu var in tdx_vcpu_enter_exit(). And the other __seamcall() caller is
> > the machine check handler...
> 
> this_cpu_write() itself won't do any function call so it's fine.
> 
> Well, lockdep_assert_preemption_disabled() does have a WARN_ON_ONCE(), but
> AFAICT using it in noinstr code is fine:

I was looking at preempt_latency_start(). But yea, it looked like there were a
few that *shouldn't* be non-inlined, but as we saw recently...

> 
> /*                                                                         
>  * This instrumentation_begin() is strictly speaking incorrect; but it     
>  * suppresses the complaints from WARN()s in noinstr code. If such a WARN()
>  * were to trigger, we'd rather wreck the machine in an attempt to get the 
>  * message out than not know about it.
>  */                                                                        
> #define __WARN_FLAGS(cond_str, flags)                           \          
> do {                                                            \          
>         __auto_type __flags = BUGFLAG_WARNING|(flags);          \          
>         instrumentation_begin();                                \          
>         _BUG_FLAGS(cond_str, ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \  
>         instrumentation_end();                                  \          
> } while (0)  
> 
> We can also just remove the lockdep_assert_preemption_disabled() in
> do_seamcall() if this is really a concern.

The concern is weird compiler/config generates a problem like this:
https://lore.kernel.org/lkml/20250624101351.8019-1-kai.huang@intel.com/

Do you think it's not valid?

> 
> > 
> > Am I missing something? It seems this patch is incomplete. If some of these
> > missed SEAMCALLs don't dirty a cacheline, then the justification that it works
> > by just covering all seamcalls needs to be updated.
> 
> I think we just want to treat all SEAMCALLs can dirty cachelines.

Right, that was the idea. I was leaving open the option that it was on purpose
to avoid these other problems. But, yes, let's stick with the (hopefully)
simpler system.

> 
> > 
> > 
> > Side topic. Do all the SEAMCALL wrappers calling into the seamcall_*() variety
> > of wrappers need the entropy retry logic? 
> > 
> 
> The purpose of doing it in common code is that we don't need to have
> duplicated code to handle running out of entropy for different SEAMCALLs.
> 
> > I think no, and some callers actually
> > depend on it not happening.
> 
> Besides TDH.VP.ENTER TDH.PHYMEM.PAGE.RDMD, which we know running out of
> entropy cannot happen, I am not aware we have any SEAMCALL that "depends on"
> it not happening.  Could you elaborate?

Some SEAMCALLs are expected to succeed, like in the BUSY error code breaking
schemes for the S-EPT ones.

Powered by blists - more mailing lists