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

[Dave Hansen <dave.hansen@intel.com>]

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.

I'm also not sure we need to be saving the return code. It seems a bit
much, but we don't have to fix that now.