From: Dave Hansen <dave.hansen@linux.intel.com>
To: dave.hansen@linux.intel.com
Subject: Re: [PATCH v9 12/23] x86/virt/seamldr: Introduce skeleton for TDX module updates
In-Reply-To: <20260513151045.1420990-13-chao.gao@intel.com>
References: <20260513151045.1420990-13-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.]

> +static void __set_target_state(struct update_ctrl *ctrl,
> +			       enum module_update_state newstate)
> +{
> +	/* Reset ack counter. */
> +	ctrl->num_ack = 0;
> +	ctrl->state = newstate;
> +}
> ...
> +	if (ctrl->num_ack == num_online_cpus())
> +		__set_target_state(ctrl, ctrl->state + 1);

`ctrl->state + 1` is enum arithmetic, and the next reader will need to
know that the enum is ordered and contiguous from MODULE_UPDATE_START
through MODULE_UPDATE_DONE.  Today that's true.  Six patches from now,
when someone adds MODULE_UPDATE_SHUTDOWN_VERIFY between SHUTDOWN and
CPU_INSTALL, this loop will silently execute the new step without the
person realising it.  Dave_Hansen.txt RULE 8 says to lock that
invariant in.  Two cheap options:

  a) Add a sentinel and a BUILD_BUG_ON:

       enum module_update_state {
           MODULE_UPDATE_START,
           MODULE_UPDATE_SHUTDOWN,
           ...
           MODULE_UPDATE_DONE,
           __MODULE_UPDATE_NSTATES,
       };
       BUILD_BUG_ON(__MODULE_UPDATE_NSTATES > 8);  /* or whatever */

     and at least the count is pinned.

  b) Replace `ctrl->state + 1` with an explicit transition helper
     (next_state(ctrl->state)) backed by a static table.  More work,
     more typing, also more obvious when a new state is added.

Neither is mandatory but the enum-arithmetic-without-a-guard is the
kind of thing I'll flag every time.

> +	enum module_update_state newstate, curstate = MODULE_UPDATE_START;
> +	int ret = 0;
> +
> +	do {
> +		newstate = READ_ONCE(update_ctrl.state);
> +
> +		if (curstate == newstate) {
> +			cpu_relax();
> +			continue;
> +		}

The state machine is well-explained in the changelog and the
"adopted from multi_cpu_stop()" credit is the right way to make a
future maintainer chasing the lineage productive.  RULE 1.

The two stripped-out things (touch_nmi_watchdog, rcu_momentary_eqs)
are called out in the v9 changelog -- good.  If we ever find we
*do* need them, the existing comment links it back to multi_cpu_stop.

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>  [with the
                                                        enum guard]