[PATCH v3 10/16] x86/virt/tdx: Drop the outdated requirement that TDX be enabled in IRQ context

Sean Christopherson posted 16 patches 1 month, 2 weeks ago
[PATCH v3 10/16] x86/virt/tdx: Drop the outdated requirement that TDX be enabled in IRQ context
Posted by Sean Christopherson 1 month, 2 weeks ago
Remove TDX's outdated requirement that per-CPU enabling be done via IPI
function call, which was a stale artifact leftover from early versions of
the TDX enablement series.  The requirement that IRQs be disabled should
have been dropped as part of the revamped series that relied on a the KVM
rework to enable VMX at module load.

In other words, the kernel's "requirement" was never a requirement at all,
but instead a reflection of how KVM enabled VMX (via IPI callback) when
the TDX subsystem code was merged.

Note, accessing per-CPU information is safe even without disabling IRQs,
as tdx_online_cpu() is invoked via a cpuhp callback, i.e. from a per-CPU
thread.

Link: https://lore.kernel.org/all/ZyJOiPQnBz31qLZ7@google.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/tdx.c      | 9 +--------
 arch/x86/virt/vmx/tdx/tdx.c | 4 ----
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 0c790eb0bfa6..582469118b79 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -3294,17 +3294,10 @@ int tdx_gmem_max_mapping_level(struct kvm *kvm, kvm_pfn_t pfn, bool is_private)
 
 static int tdx_online_cpu(unsigned int cpu)
 {
-	unsigned long flags;
-	int r;
-
 	/* Sanity check CPU is already in post-VMXON */
 	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
 
-	local_irq_save(flags);
-	r = tdx_cpu_enable();
-	local_irq_restore(flags);
-
-	return r;
+	return tdx_cpu_enable();
 }
 
 static int tdx_offline_cpu(unsigned int cpu)
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 5ce4ebe99774..dfd82fac0498 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -148,8 +148,6 @@ static int try_init_module_global(void)
  * global initialization SEAMCALL if not done) on local cpu to make this
  * cpu be ready to run any other SEAMCALLs.
  *
- * Always call this function via IPI function calls.
- *
  * Return 0 on success, otherwise errors.
  */
 int tdx_cpu_enable(void)
@@ -160,8 +158,6 @@ int tdx_cpu_enable(void)
 	if (!boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM))
 		return -ENODEV;
 
-	lockdep_assert_irqs_disabled();
-
 	if (__this_cpu_read(tdx_lp_initialized))
 		return 0;
 
-- 
2.53.0.310.g728cabbaf7-goog
Re: [PATCH v3 10/16] x86/virt/tdx: Drop the outdated requirement that TDX be enabled in IRQ context
Posted by Huang, Kai 1 month, 2 weeks ago
On Fri, 2026-02-13 at 17:26 -0800, Sean Christopherson wrote:
> Remove TDX's outdated requirement that per-CPU enabling be done via IPI
> function call, which was a stale artifact leftover from early versions of
> the TDX enablement series.  The requirement that IRQs be disabled should
> have been dropped as part of the revamped series that relied on a the KVM
> rework to enable VMX at module load.
> 
> In other words, the kernel's "requirement" was never a requirement at all,
> but instead a reflection of how KVM enabled VMX (via IPI callback) when
> the TDX subsystem code was merged.
> 
> Note, accessing per-CPU information is safe even without disabling IRQs,
> as tdx_online_cpu() is invoked via a cpuhp callback, i.e. from a per-CPU
> thread.
> 
> Link: https://lore.kernel.org/all/ZyJOiPQnBz31qLZ7@google.com
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Hi Sean,

The first call of tdx_cpu_enable() will also call into
try_init_module_global() (in order to do TDH_SYS_INIT), which also has a
lockdep_assert_irqs_disabled() + a raw spinlock to make sure TDH_SYS_INIT is
only called once when tdx_cpu_enable() are called from IRQ disabled context.

This patch only changes tdx_cpu_enable() but doesn't change
try_init_module_global(), thus the first call of tdx_cpu_enable() will still
trigger the lockdep_assert_irqs_disabled() failure warning.

I've tried this series on my local and I did see such WARNING during
boot[*].  We need to fix that too.

But hmm, Chao's "Runtime TDX module update" series actually needs to call
tdx_cpu_enable() when IRQ disabled, IIUC, since it is called via
stop_machine_cpuslocked():

https://lore.kernel.org/kvm/20260212143606.534586-18-chao.gao@intel.com/

Maybe we can just keep tdx_cpu_enabled() as-is?

[*] lockdep WARNING():

[    7.755642] ------------[ cut here ]------------
[    7.756639] __lockdep_enabled && this_cpu_read(hardirqs_enabled)
[    7.756642] WARNING: arch/x86/virt/vmx/tdx/tdx.c:119 at
try_init_module_global+0x189/0x1c0, CPU#0: cpuhp/0/21

Re: [PATCH v3 10/16] x86/virt/tdx: Drop the outdated requirement that TDX be enabled in IRQ context
Posted by Sean Christopherson 1 month, 2 weeks ago
On Tue, Feb 17, 2026, Kai Huang wrote:
> On Fri, 2026-02-13 at 17:26 -0800, Sean Christopherson wrote:
> > Remove TDX's outdated requirement that per-CPU enabling be done via IPI
> > function call, which was a stale artifact leftover from early versions of
> > the TDX enablement series.  The requirement that IRQs be disabled should
> > have been dropped as part of the revamped series that relied on a the KVM
> > rework to enable VMX at module load.
> > 
> > In other words, the kernel's "requirement" was never a requirement at all,
> > but instead a reflection of how KVM enabled VMX (via IPI callback) when
> > the TDX subsystem code was merged.
> > 
> > Note, accessing per-CPU information is safe even without disabling IRQs,
> > as tdx_online_cpu() is invoked via a cpuhp callback, i.e. from a per-CPU
> > thread.
> > 
> > Link: https://lore.kernel.org/all/ZyJOiPQnBz31qLZ7@google.com
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > 
> 
> Hi Sean,
> 
> The first call of tdx_cpu_enable() will also call into
> try_init_module_global() (in order to do TDH_SYS_INIT), which also has a
> lockdep_assert_irqs_disabled() + a raw spinlock to make sure TDH_SYS_INIT is
> only called once when tdx_cpu_enable() are called from IRQ disabled context.
> 
> This patch only changes tdx_cpu_enable() but doesn't change
> try_init_module_global(), thus the first call of tdx_cpu_enable() will still
> trigger the lockdep_assert_irqs_disabled() failure warning.
> 
> I've tried this series on my local and I did see such WARNING during
> boot[*].  We need to fix that too.
> 
> But hmm, Chao's "Runtime TDX module update" series actually needs to call
> tdx_cpu_enable() when IRQ disabled, IIUC, since it is called via
> stop_machine_cpuslocked():
> 
> https://lore.kernel.org/kvm/20260212143606.534586-18-chao.gao@intel.com/
> 
> Maybe we can just keep tdx_cpu_enabled() as-is?

Can't we simply delete the lockdep assert there as well?  It should be totally
fine to have a function that can be called from task or IRQ context, so long as
the function is prepared for that possibility.  I.e. just because it _can_ be
called from IRQ context doesn't mean it _must_ be called from IRQ context.

E.g. as a fixup

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index bdee937b84d4..f8f5e046159b 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -106,8 +106,7 @@ static __always_inline int sc_retry_prerr(sc_func_t func,
 
 /*
  * Do the module global initialization once and return its result.
- * It can be done on any cpu.  It's always called with interrupts
- * disabled.
+ * It can be done on any cpu, and from task or IRQ context.
  */
 static int try_init_module_global(void)
 {
@@ -116,8 +115,6 @@ static int try_init_module_global(void)
        static bool sysinit_done;
        static int sysinit_ret;
 
-       lockdep_assert_irqs_disabled();
-
        raw_spin_lock(&sysinit_lock);
 
        if (sysinit_done)
Re: [PATCH v3 10/16] x86/virt/tdx: Drop the outdated requirement that TDX be enabled in IRQ context
Posted by Huang, Kai 1 month, 2 weeks ago
On Tue, 2026-02-17 at 07:25 -0800, Sean Christopherson wrote:
> On Tue, Feb 17, 2026, Kai Huang wrote:
> > On Fri, 2026-02-13 at 17:26 -0800, Sean Christopherson wrote:
> > > Remove TDX's outdated requirement that per-CPU enabling be done via IPI
> > > function call, which was a stale artifact leftover from early versions of
> > > the TDX enablement series.  The requirement that IRQs be disabled should
> > > have been dropped as part of the revamped series that relied on a the KVM
> > > rework to enable VMX at module load.
> > > 
> > > In other words, the kernel's "requirement" was never a requirement at all,
> > > but instead a reflection of how KVM enabled VMX (via IPI callback) when
> > > the TDX subsystem code was merged.
> > > 
> > > Note, accessing per-CPU information is safe even without disabling IRQs,
> > > as tdx_online_cpu() is invoked via a cpuhp callback, i.e. from a per-CPU
> > > thread.
> > > 
> > > Link: https://lore.kernel.org/all/ZyJOiPQnBz31qLZ7@google.com
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > 
> > 
> > Hi Sean,
> > 
> > The first call of tdx_cpu_enable() will also call into
> > try_init_module_global() (in order to do TDH_SYS_INIT), which also has a
> > lockdep_assert_irqs_disabled() + a raw spinlock to make sure TDH_SYS_INIT is
> > only called once when tdx_cpu_enable() are called from IRQ disabled context.
> > 
> > This patch only changes tdx_cpu_enable() but doesn't change
> > try_init_module_global(), thus the first call of tdx_cpu_enable() will still
> > trigger the lockdep_assert_irqs_disabled() failure warning.
> > 
> > I've tried this series on my local and I did see such WARNING during
> > boot[*].  We need to fix that too.
> > 
> > But hmm, Chao's "Runtime TDX module update" series actually needs to call
> > tdx_cpu_enable() when IRQ disabled, IIUC, since it is called via
> > stop_machine_cpuslocked():
> > 
> > https://lore.kernel.org/kvm/20260212143606.534586-18-chao.gao@intel.com/
> > 
> > Maybe we can just keep tdx_cpu_enabled() as-is?
> 
> Can't we simply delete the lockdep assert there as well?  It should be totally
> fine to have a function that can be called from task or IRQ context, so long as
> the function is prepared for that possibility.  I.e. just because it _can_ be
> called from IRQ context doesn't mean it _must_ be called from IRQ context.
> 
> E.g. as a fixup

Yeah we can.  LGTM.