Re: [PATCH v3 2/6] x86/virt/tdx: Mark memory cache state incoherent when making SEAMCALL

[Huang, Kai]

On Fri, 2025-06-27 at 00:52 +0000, Edgecombe, Rick P wrote:
> 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.

I agree we should revisit this in the future.

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

Yeah already did.  The below diff [*] doesn't trigger warning for
tdh_vp_enter()  for both gcc and clang with the Kconfig that can trigger the
warning fixed by the patch here:

https://lore.kernel.org/lkml/20250624101351.8019-1-kai.huang@xxxxxxxxx/

I'll see whether I can do more.

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

Note with the diff [*] tdh_vp_enter() will only call
lockdep_assert_preemption_disabled() which doesn't call
preempt_latency_start().

> 
> > 
> > /*                                                                         
> >  * 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@xxxxxxxxx/
> 
> Do you think it's not valid?

I absolutely do.

However WARN_ON_ONCE() in noinstr is fine since there's
instrumentation_begin()/end() inside.

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

If they succeed then the sc_retry() will just call SEAMCALL once.  If we
really want some SEAMCALL not to handle running out of entropy at all, e.g.,
they can be called in critical path, then we can change that to
do_seamcall() instead.

[*] the incremental diff:

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index d4c624c69d7f..6865f62436ad 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -113,7 +113,7 @@ static __always_inline u64 do_seamcall(sc_func_t func,
u64 fn,
 {
        u64 ret;
 
-       preempt_disable();
+       lockdep_assert_preemption_disabled();
 
        /*
         * SEAMCALLs are made to the TDX module and can generate dirty
@@ -128,8 +128,6 @@ static __always_inline u64 do_seamcall(sc_func_t func,
u64 fn,
 
        ret = func(fn, args);
 
-       preempt_enable();
-
        return ret;
 }
 
@@ -140,7 +138,9 @@ static __always_inline u64 sc_retry(sc_func_t func, u64
fn,
        u64 ret;
 
        do {
+               preempt_disable();
                ret = do_seamcall(func, fn, args);
+               preempt_enable();
        } while (ret == TDX_RND_NO_ENTROPY && --retry);
 
        return ret;
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index c7a9a087ccaf..d6ee4e5a75d2 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1266,7 +1266,7 @@ static bool paddr_is_tdx_private(unsigned long phys)
                return false;
 
        /* Get page type from the TDX module */
-       sret = __seamcall_ret(TDH_PHYMEM_PAGE_RDMD, &args);
+       sret = do_seamcall(__seamcall_ret, TDH_PHYMEM_PAGE_RDMD, &args);
 
        /*
         * The SEAMCALL will not return success unless there is a
@@ -1522,7 +1522,7 @@ noinstr __flatten u64 tdh_vp_enter(struct tdx_vp *td,
struct tdx_module_args *ar
 {
        args->rcx = tdx_tdvpr_pa(td);
 
-       return __seamcall_saved_ret(TDH_VP_ENTER, args);
+       return do_seamcall(__seamcall_saved_ret, TDH_VP_ENTER, args);
 }
 EXPORT_SYMBOL_GPL(tdh_vp_enter);