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

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

On Thu, Mar 12, 2026 at 01:40:44PM -0700, Dave Hansen wrote:
>On 2/12/26 06:35, Chao Gao wrote:
>> +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 */
>> +	smp_wmb();
>> +	WRITE_ONCE(tdp_data.state, state);
>> +}
>
>This looks overly complicated.
>
>If it doesn't need to be scalable, just make it stupid and simple. Why
>not just protect the whole thing with a spinlock and be done with it?

Good suggestion. I copied this from multi_cpu_stop() without considering
whether it could be simplified.

Regarding scalability, I compared the update time and didn't see a
meaningful difference on a system with 240 CPUs.

I will make changes like this:

(Note: I'm also renaming tdp_data/tdp_state to update_data and
 module_update_state for clarity, since "tdp" isn't obvious as Kai pointed
 out.)

 static struct {
	enum module_update_state state;
-	atomic_t thread_ack;
-	atomic_t failed;
+	int thread_ack;
+	int failed;
+	raw_spinlock_t lock;
 } update_data;
 
 static void set_target_state(enum module_update_state state)
 {
	/* Reset ack counter. */
-	atomic_set(&update_data.thread_ack, num_online_cpus());
-	/*
-	 * 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(update_data.state, state);
+	update_data.thread_ack = num_online_cpus();
+	update_data.state = state;
 }
 
 /* Last one to ack a state moves to the next state. */
 static void ack_state(void)
 {
-	if (atomic_dec_and_test(&update_data.thread_ack))
+	guard(raw_spinlock)(&update_data.lock);
+	update_data.thread_ack--;
+	if (!update_data.thread_ack)
		set_target_state(update_data.state + 1);
 }