From: Dave Hansen <dave.hansen@linux.intel.com>
To: dave.hansen@linux.intel.com
Subject: Re: [PATCH v9 14/23] x86/virt/seamldr: Shut down the current TDX module
In-Reply-To: <20260513151045.1420990-15-chao.gao@intel.com>
References: <20260513151045.1420990-15-chao.gao@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[Draft review generated by x86-maintainer-agent applying Dave_Hansen.txt
 rules to the v9 series.  Strip this banner before sending.]

> +int tdx_module_shutdown(void)
> +{
> +	struct tdx_sys_info_handoff handoff = {};
> +	struct tdx_module_args args = {};
> +	int ret;
> +
> +	ret = get_tdx_sys_info_handoff(&handoff);
> +	WARN_ON_ONCE(ret);
> +
> +	/*
> +	 * Use the module's handoff version as it is the highest the
> +	 * module can produce and most likely supported by newer modules.
> +	 */
> +	args.rcx = handoff.module_hv;
> +	return seamcall_prerr(TDH_SYS_SHUTDOWN, &args);

This is the bug-shaped one (Dave_Hansen.txt RULE 8, used in
the wrong direction).  WARN_ON_ONCE() does not stop execution.  If
get_tdx_sys_info_handoff() fails, `handoff` is the zero-initialised
struct, `module_hv` is 0, and the SEAMCALL proceeds with that.  That
might happen to be "request handoff version 0", which may or may not
be a meaningful request to TDH.SYS.SHUTDOWN.

Three plausible options:

  1. Just propagate the error:

       ret = get_tdx_sys_info_handoff(&handoff);
       if (ret)
           return ret;

  2. If module_hv really is allowed to be 0 here (please confirm
     against the spec), say so in a comment so that the next person
     reading this doesn't have to look it up:

       /*
        * handoff.module_hv == 0 is the "ask the module to pick"
        * encoding; if get_tdx_sys_info_handoff() failed, falling back
        * to that is safe.  WARN once so we notice the metadata read
        * is broken.
        */

  3. Failing that, hold something concrete in the WARN payload
     (WARN_ON_ONCE(ret), then early-out).

Right now the code is silently doing a SEAMCALL with whatever
happened to be in the stack-allocated struct + a printk.  That's the
pattern Dave_Hansen.txt RULE 8 calls out specifically.

The patch-15 follow-on (clearing tdx_module_state, per-cpu
tdx_lp_initialized) is structurally correct -- but is split between
this patch and 15/23 for reasons that don't seem clear; the changelog
of 15/23 says "Reset all software flags guarding the initialization
flows to allow the global and per-CPU initializations to be triggered
again after updates," which is exactly what this patch needs to be
correct.  Consider folding 15 into this commit (split is
fine -- if there's a reason like keeping the state-machine patch
small, call it out).  RULE 3.