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

[Huang, Kai]

On Tue, 2026-02-24 at 14:00 +0800, Chao Gao wrote:
> 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().

OK thanks for clarification.

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

Thanks.

> 
> > 
> > > +
> > > +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.
> 	 */
> 	

LGTM.

> > 
> > > +	smp_wmb();
> > > +	WRITE_ONCE(tdp_data.state, state);
> > > +}