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

[Edgecombe, Rick P]

On Mon, 2026-05-18 at 15:43 +0800, Chao Gao wrote:
> commit 26bb389c5762fd6a496fbed1cc55e4978e99a5cb
> Author: Chao Gao <chao.gao@xxxxxxxxx>
> 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
                          ^ harder then what? Maybe just "hard"
>     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@xxxxxxxxx>

This is a great improvement by itself, irrespective of this series. The original
code made my head hurt when I first saw it:
https://lore.kernel.org/all/726dccd6d46d0bd471ec0b2f6861f8e45bade26c.camel@xxxxxxxxx/

The handling of things outside the lock is one thing, but also the function
scopes statics stood out to me as strange. So yea, maybe two patches, this one
and another to get rid of the function scoped statics?

> 
> 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;
>  }