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

["Huang, Kai" <kai.huang@intel.com>]

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

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.

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

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

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

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

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