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@xxxxxxxxx/
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.
