Re: [PATCH v9 01/23] x86/virt/tdx: Consolidate TDX global initialization states

[Chao Gao <chao.gao@intel.com>]

On Fri, May 15, 2026 at 09:14:02AM -0700, Dave Hansen wrote:
>On 5/13/26 08:09, Chao Gao wrote:
>...
>> Group the states into a single structure so they can be reset together, for
>> example with memset(), and so a newly added state won't be missed.
>...
>> +struct tdx_module_state {
>> +	bool initialized;
>> +	bool sysinit_done;
>> +	int sysinit_ret;
>> +};
>...
>> @@ -113,30 +119,28 @@ static int try_init_module_global(void)
>>  {
>>  	struct tdx_module_args args = {};
>>  	static DEFINE_RAW_SPINLOCK(sysinit_lock);
>> -	static bool sysinit_done;
>> -	static int sysinit_ret;
>
>This doesn't look right to me.
>
>>  	raw_spin_lock(&sysinit_lock);
>>  
>> -	if (sysinit_done)
>> +	if (tdx_module_state.sysinit_done)
>>  		goto out;
>>  
>>  	/* RCX is module attributes and all bits are reserved */
>>  	args.rcx = 0;
>> -	sysinit_ret = seamcall_prerr(TDH_SYS_INIT, &args);
>> +	tdx_module_state.sysinit_ret = seamcall_prerr(TDH_SYS_INIT, &args);
>>  
>>  	/*
>>  	 * The first SEAMCALL also detects the TDX module, thus
>>  	 * it can fail due to the TDX module is not loaded.
>>  	 * Dump message to let the user know.
>>  	 */
>> -	if (sysinit_ret == -ENODEV)
>> +	if (tdx_module_state.sysinit_ret == -ENODEV)
>>  		pr_err("module not loaded\n");
>>  
>> -	sysinit_done = true;
>> +	tdx_module_state.sysinit_done = true;
>>  out:
>>  	raw_spin_unlock(&sysinit_lock);
>> -	return sysinit_ret;
>> +	return tdx_module_state.sysinit_ret;
>>  }
>
>But I think it's because 'sysinit_ret' is really a funky thing. It's
>just here so that the module only _tries_ to be initialized once. That
>one-time init records its error code in sysinit_ret and then secondary
>callers pick it up.
>
>Here's how you do that in a non-confusing way (some context chopped out):
>
>static int try_init_module_global(void)
>{
>        struct tdx_module_args args = {};
>        static DEFINE_RAW_SPINLOCK(sysinit_lock);
>        int ret;
>
>        raw_spin_lock(&sysinit_lock);
>
>	/* Return the "cached" return code: */
>        if (tdx_module_state.sysinit_done) {
>		ret = tdx_module_state.sysinit_ret;
>                goto out;
>	}
>
>        ret = seamcall_prerr(TDH_SYS_INIT, &args);
>
>	/* Save the return code for later callers: */
>	tdx_module_state.sysinit_ret  = ret;
>        tdx_module_state.sysinit_done = true;
>out:
>        raw_spin_unlock(&sysinit_lock);
>        return ret;
>}
>
>See how it sets the module state in _one_ place? It also only touches
>the module state under the lock so it's more obvious that it is correct
>and there are no races or tearing or other nonsense.
>
>*That* is a proper refactoring.

Yes, that is a clear readability improvement.

To follow the "one change per patch" rule, I am inclined to do that as a
separate preparatory cleanup before moving all states into a structure.

commit 26bb389c5762fd6a496fbed1cc55e4978e99a5cb
Author: Chao Gao <chao.gao@intel.com>
Date:   Sun May 17 20:03:00 2026 -0700

    x86/virt/tdx: Clarify try_init_module_global() result caching
    
    TDX module global initialization is executed only once. The first call
    caches both the result and the "done" state, and later callers reuse the
    saved result. A lock protects that cached state.
    
    The current code is harder to read because sysinit_done is accessed under
    the lock, while sysinit_ret is not.
    
    To improve readability, move sysinit_ret accesses within the lock.
    
    Group sysinit_ret/sysinit_done updates right after initialization so
    Caching the result is separate from the initialization itself.
    
    Signed-off-by: Chao Gao <chao.gao@intel.com>

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index c0c6281b08a5..ad56f142dd0b 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -115,28 +115,34 @@ static int try_init_module_global(void)
	static DEFINE_RAW_SPINLOCK(sysinit_lock);
	static bool sysinit_done;
	static int sysinit_ret;
+	int ret;
 
	raw_spin_lock(&sysinit_lock);
 
-	if (sysinit_done)
+	/* Return the "cached" return code. */
+	if (sysinit_done) {
+		ret = sysinit_ret;
		goto out;
+	}
 
	/* RCX is module attributes and all bits are reserved */
	args.rcx = 0;
-	sysinit_ret = seamcall_prerr(TDH_SYS_INIT, &args);
+	ret = seamcall_prerr(TDH_SYS_INIT, &args);
 
	/*
	 * The first SEAMCALL also detects the TDX module, thus
	 * it can fail due to the TDX module is not loaded.
	 * Dump message to let the user know.
	 */
-	if (sysinit_ret == -ENODEV)
+	if (ret == -ENODEV)
		pr_err("module not loaded\n");
 
+	/* Save the return code for later callers. */
	sysinit_done = true;
+	sysinit_ret = ret;
 out:
	raw_spin_unlock(&sysinit_lock);
-	return sysinit_ret;
+	return ret;
 }
 
 /**