On TDX platforms, at hardware level dirty cachelines with and without
TDX keyID can coexist, and CPU can flush them back to memory in random
order. During kexec, the caches must be flushed before jumping to the
new kernel to avoid silent memory corruption when a cacheline with a
different encryption property is written back over whatever encryption
properties the new kernel is using.
A percpu boolean is used to mark whether the cache of a given CPU may be
in an incoherent state, and the kexec performs WBINVD on the CPUs with
that boolean turned on.
For TDX, only the TDX module or the TDX guests can generate dirty
cachelines of TDX private memory, i.e., they are only generated when the
kernel does SEAMCALL.
Turn on that boolean when the kernel does SEAMCALL so that kexec can
correctly flush cache.
SEAMCALL can be made from both task context and IRQ disabled context.
Given SEAMCALL is just a lengthy instruction (e.g., thousands of cycles)
from kernel's point of view and preempt_{disable|enable}() is cheap
compared to it, simply unconditionally disable preemption during setting
the percpu boolean and making SEAMCALL.
Signed-off-by: Kai Huang <kai.huang@intel.com>
Tested-by: Farrah Chen <farrah.chen@intel.com>
---
v2 -> v3:
- Change to use __always_inline for do_seamcall() to avoid indirect
call instructions of making SEAMCALL.
- 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)
+{
+ u64 ret;
+
+ preempt_disable();
+
+ /*
+ * SEAMCALLs are made to the TDX module and can generate dirty
+ * cachelines of TDX private memory. Mark cache state incoherent
+ * so that the cache can be flushed during kexec.
+ *
+ * This needs to be done before actually making the SEAMCALL,
+ * because kexec-ing CPU could send NMI to stop remote CPUs,
+ * in which case even disabling IRQ won't help here.
+ */
+ this_cpu_write(cache_state_incoherent, true);
+
+ ret = func(fn, args);
+
+ preempt_enable();
+
+ return ret;
+}
+
static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
struct tdx_module_args *args)
{
@@ -113,7 +140,7 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
u64 ret;
do {
- ret = func(fn, args);
+ ret = do_seamcall(func, fn, args);
} while (ret == TDX_RND_NO_ENTROPY && --retry);
return ret;
--
2.49.0On Thu, 2025-06-26 at 22:48 +1200, Kai Huang wrote:
> On TDX platforms, at hardware level dirty cachelines with and without
> TDX keyID can coexist, and CPU can flush them back to memory in random
> order.
>
Same comment as the previous patch.
> During kexec, the caches must be flushed before jumping to the
> new kernel to avoid silent memory corruption when a cacheline with a
> different encryption property is written back over whatever encryption
> properties the new kernel is using.
>
> A percpu boolean is used to mark whether the cache of a given CPU may be
> in an incoherent state, and the kexec performs WBINVD on the CPUs with
> that boolean turned on.
>
> For TDX, only the TDX module or the TDX guests can generate dirty
> cachelines of TDX private memory, i.e., they are only generated when the
> kernel does SEAMCALL.
^a
>
> Turn on that boolean when the kernel does SEAMCALL so that kexec can
Nit: "Turn on" is a little ambiguous. "Set"?
> correctly flush cache.
>
> SEAMCALL can be made from both task context and IRQ disabled context.
SEAMCALLs
> Given SEAMCALL is just a lengthy instruction (e.g., thousands of cycles)
> from kernel's point of view and preempt_{disable|enable}() is cheap
> compared to it, simply unconditionally disable preemption during setting
> the percpu boolean and making SEAMCALL.
>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
> ---
>
> v2 -> v3:
> - Change to use __always_inline for do_seamcall() to avoid indirect
> call instructions of making SEAMCALL.
How did this come about?
> - 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().
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().
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...
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.
Side topic. Do all the SEAMCALL wrappers calling into the seamcall_*() variety
of wrappers need the entropy retry logic? I think no, and some callers actually
depend on it not happening.
> + u64 ret;
> +
> + preempt_disable();
> +
> + /*
> + * SEAMCALLs are made to the TDX module and can generate dirty
> + * cachelines of TDX private memory. Mark cache state incoherent
> + * so that the cache can be flushed during kexec.
> + *
> + * This needs to be done before actually making the SEAMCALL,
> + * because kexec-ing CPU could send NMI to stop remote CPUs,
> + * in which case even disabling IRQ won't help here.
> + */
> + this_cpu_write(cache_state_incoherent, true);
> +
> + ret = func(fn, args);
> +
> + preempt_enable();
> +
> + return ret;
> +}
> +
> static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
> struct tdx_module_args *args)
> {
> @@ -113,7 +140,7 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
> u64 ret;
>
> do {
> - ret = func(fn, args);
> + ret = do_seamcall(func, fn, args);
> } while (ret == TDX_RND_NO_ENTROPY && --retry);
>
> return ret;
(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?
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.
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@intel.com/
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@intel.com/
>
> 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);
© 2016 - 2025 Red Hat, Inc.