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

[Huang, Kai]

>  
> +/*
> + * During a TDX Module update, all CPUs start from TDP_START and progress

Nit:  start from TDP_START or TDP_START + 1 ?

The code below says:

+	set_target_state(TDP_START + 1);
+	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?

> +
> +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?

> +	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.)