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

[Edgecombe, Rick P]

On Thu, 2026-03-12 at 22:09 +0800, Chao Gao wrote:
> On Thu, Mar 12, 2026 at 10:00:20AM +0800, Edgecombe, Rick P wrote:
> > On Thu, 2026-02-12 at 06:35 -0800, Chao Gao wrote:
> > > TDX Module updates require careful synchronization with other TDX
> > > operations on the host. During updates, only update-related SEAMCALLs are
> > > permitted; all other SEAMCALLs must be blocked.
> > > 
> > > However, SEAMCALLs can be invoked from different contexts (normal and IRQ
> > > context) and run in parallel across CPUs. And, all TD vCPUs must remain
> > > out of guest mode during updates.
> > > 
> > 
> > Above it says only update-related SEAMCALLs are permitted. Does that not already
> > exclude SEAMCALLs that might allow entering the TD?
> 
> Those SEAMCALLs would return errors, and TDs would be killed if those errors
> aren't handled properly.
> 
> One may argue that we can handle errors and retry after updates. But this just
> provides a new form of synchronization, which is not as clean as the
> well-defined synchronization provided by the kernel.

Ah ok, so it's not about the SEAMCALL resulting in entering the TD, it's about
SEAMCALLs that are operating on TDs. That makes sense. But probably don't focus
on the TD entering part then. It's just any seamcalls that are not allowed
should not be called and they need to be excluded. It's simpler.

> 
> > 
> > >  No single lock primitive can satisfy
> > > all these synchronization requirements, so stop_machine() is used as the
> > > only well-understood mechanism that can meet them all.
> > > 
> > > The TDX Module update process consists of several steps as described in
> > > Intel® Trust Domain Extensions (Intel® TDX) Module Base Architecture
> > > Specification, Revision 348549-007, Chapter 4.5 "TD-Preserving TDX Module
> > > Update"
> > > 
> > >   - shut down the old module
> > >   - install the new module
> > >   - global and per-CPU initialization
> > >   - restore state information
> > > 
> > > Some steps must execute on a single CPU, others must run serially across
> > > all CPUs, and some can run concurrently on all CPUs. There are also
> > > ordering requirements between steps, so all CPUs must work in a step-locked
> > > manner.
> > 
> > Does the fact that they can run on other CPUs add any synchronization
> > requirements? If not I'd leave it off.
> 
> I'm not sure I understand the concern.
> 
> Lockstep synchronization is needed specifically because we have both multiple
> CPUs and multiple steps.
> 
> If updates only required a single CPU, stop_machine() would be sufficient.

The last part "some can run concurrently on all CPUs", how does it affect the
design? They can run concurrently, but don't have to... So it's a non-
requirement?

It seems the main argument here is, this thing has lots of complex ordering
requirements. So we do it lockstep as a simple pattern to bring sanity. It's a
fine fuzzy argument I think. The way you list the types of requirements all
specifically has me trying to find the connection between each requirement and
lockstep. That is where I get lost. If the reader doesn't need to do the work of
understanding, don't ask them. And if they do, it probably needs to be clearer.

> 
> > 
> > > 
> > > In summary, TDX Module updates create two requirements:
> > 
> > The stop_machine() part seems more like a solution then a requirement.
> > 
> > > 
> > > 1. The entire update process must use stop_machine() to synchronize with
> > >    other TDX workloads
> > > 2. Update steps must be performed in a step-locked manner
> > > 
> > > To prepare for implementing concrete TDX Module update steps, establish
> > > the framework by mimicking multi_cpu_stop(), which is a good example of
> > > performing a multi-step task in step-locked manner.
> > > 
> > 
> > Offline Chao pointed that Paul suggested this after considering refactoring out
> > the common code. I think it might still be worth mentioning why you can't use
> > multi_cpu_stop() directly. I guess there are some differences. what are they.
> 
> To be clear, Paul didn't actually suggest this approach. His feedback indicated
> he wasn't concerned about duplicating some of multi_cpu_stop()'s code, i.e., no
> need to refactor out some common code.

Right, sorry for oversimplifying.

> 
> https://lore.kernel.org/all/a7affba9-0cea-4493-b868-392158b59d83@paulmck-laptop/#t
> 
> We can't use multi_cpu_stop() directly because it only provides lockstep
> execution for its own infrastructure, not for the function it runs. If we
> passed a function that performs steps A, B, and C to multi_cpu_stop(), there's
> no guarantee that all CPUs complete step A before any CPU begins step B.

If it could be said more concisely, it seems relevant.

> 
> > 
> > >  Specifically, use a
> > > global state machine to control each CPU's work and require all CPUs to
> > > acknowledge completion before proceeding to the next step.
> > 
> > Maybe add a bit more about the reasoning for requiring the other steps to ack.
> > Tie it back to the lockstep part.
> > 
> 
> Ok. How about:
> 
> Specifically, add a global state machine where each state represents a step in
> the above update flow. The state advances only after all CPUs acknowledge
> completing their work in the current state. This acknowledgment mechanism is
> what ensures lockstep execution.

Looks good.

> 
> <snip>
> 
> > > +static int do_seamldr_install_module(void *params)
> > > +{
> > > +	enum tdp_state newstate, curstate = TDP_START;
> > > +	int ret = 0;
> > > +
> > > +	do {
> > > +		/* Chill out and re-read tdp_data */
> > > +		cpu_relax();
> > > +		newstate = READ_ONCE(tdp_data.state);
> > > +
> > > +		if (newstate != curstate) {
> > > +			curstate = newstate;
> > > +			switch (curstate) {
> > 
> > Maybe a little comment here like "todo add the steps".
> 
> Sure.