Re: [PATCH v4 11/24] x86/virt/seamldr: Introduce skeleton for TDX Module updates

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

On Mon, Feb 23, 2026 at 05:25:53PM +0800, Huang, Kai wrote:
>
>>  
>> +/*
>> + * During a TDX Module update, all CPUs start from TDP_START and progress
>
>Nit:  start from TDP_START or TDP_START + 1 ?

TDP_START. See:

+static int do_seamldr_install_module(void *params)
+{
+       enum tdp_state newstate, curstate = TDP_START;
				 ^^^^^^^^^^^^^^^^^^^^

>
>The code below says:
>
>+	set_target_state(TDP_START + 1);

set_target_state() sets a global target (or next) state for all CPUs. Each CPU
compares its current state to the target. If they don't match, the CPU performs
the required task and then acks the state.

The global target state must be reset at the start of each update to trigger
the do-while loop in do_seamldr_install_module().

>+	ret = stop_machine_cpuslocked(do_seamldr_install_module, params,
>cpu_online_mask);
>
>> + * to TDP_DONE. Each state is associated with certain work. For some
>> + * states, just one CPU needs to perform the work, while other CPUs just
>> + * wait during those states.
>> + */
>> +enum tdp_state {
>> +	TDP_START,
>> +	TDP_DONE,
>> +};
>
>Nit:  just curious, what does "TDP" mean?
>
>Maybe something more obvious?

It stands for TD Preserving. Since this term isn't commonly used outside
Intel, "TDX Module updates" is clearer. I'll change this enum to:

enum module_update_state {
	MODULE_UPDATE_START,
	MODULE_UPDATE_SHUTDOWN,
	MODULE_UPDATE_CPU_INSTALL,
	MODULE_UPDATE_CPU_INIT,
	MODULE_UPDATE_RUN_UPDATE,
	MODULE_UPDATE_DONE,
};

>
>> +
>> +static struct {
>> +	enum tdp_state state;
>> +	atomic_t thread_ack;
>> +} tdp_data;
>> +
>> +static void set_target_state(enum tdp_state state)
>> +{
>> +	/* Reset ack counter. */
>> +	atomic_set(&tdp_data.thread_ack, num_online_cpus());
>> +	/* Ensure thread_ack is updated before the new state */
>
>Nit:  perhaps add "so that ..." part to the comment?

how about:

	/*
	 * Ensure thread_ack is updated before the new state.
	 * Otherwise, other CPUs may see the new state and ack
	 * it before thread_ack is reset. An ack before reset
	 * is effectively lost, causing the system to wait
	 * forever for thread_ack to become zero.
	 */
	
>
>> +	smp_wmb();
>> +	WRITE_ONCE(tdp_data.state, state);
>> +}
>> +
>> +/* Last one to ack a state moves to the next state. */
>> +static void ack_state(void)
>> +{
>> +	if (atomic_dec_and_test(&tdp_data.thread_ack))
>> +		set_target_state(tdp_data.state + 1);
>> +}
>> +
>> +/*
>> + * See multi_cpu_stop() from where this multi-cpu state-machine was
>> + * adopted, and the rationale for touch_nmi_watchdog()
>> + */
>
>Nit:  add a period to the end of the sentence.
>
>(btw, I found using period or not isn't consistent even among the 'one-line-
>sentence' comments, maybe you want to make that consistent.) 

Will do. Thanks for this suggestion.