Re: [PATCH v9 13/23] x86/virt/seamldr: Abort updates after a failed step

[Chao Gao]

On Tue, May 19, 2026 at 10:34:31AM +0800, Edgecombe, Rick P wrote:
>On Wed, 2026-05-13 at 08:09 -0700, Chao Gao wrote:
>> A TDX module update is a multi-step process, and any step can fail.
>> 
>> The current update flow continues to later steps after an error.
>> Continuing after a failure can leave the TDX module in an unrecoverable
>> state.
>
>I get what you are saying here, but "continuing" vs "leaving" is a tiny bit
>confusing to me. Maybe: Continuing with subsequent update steps after a failure
>can cause the TDX module to enter an unrecoverable state?

Yes. it is better.

>
>> 
>> One failure case must remain recoverable: update contention with an ongoing
>> TD build. The agreed kernel behavior for this case [1] is to fail the
>> update with -EBUSY so userspace can retry later.
>
>The link to the discussion is nice, but the explanation of just that there was
>an agreement is not saying much. But the reasoning around AVOID_COMPAT_SENSITIVE
>*is* handled in later patch. So can we say future changes will want to return
>errors to userspace for certain update failures? Then we can discuss the
>specifics when code is actual error is added?

yes. the main point is certain update failures should be recoverable so userspace
can retry.

>
>And why talk about EBUSY specifically? It is not in this patch. Stale log? 

Sure. there is no need to single out EBUSY.

>
>> 
>> Abort the update on any failure. This also makes the TD-build contention
>> case recoverable, because that failure occurs before any TDX module state
>> is changed. 
>> 
>
>Oh, maybe I didn't get what you meant above actually. The contention case is
>only recoverable because we detect it at the first step? Does "Continuing after
>a failure can leave the TDX module in an unrecoverable" really mean that any
>failure after the first step is unrecoverable?

You are right. Any failure after the initial module shutdown step is
unrecoverable.

> Or can we put it in some other
>more specific terms like that. Terms which are more specific but still not
>overly complex description of TDX module update flows?
>
>> Apply the same rule to all errors instead of special-casing
>> -EBUSY.
>
>It seems like actually it is not special cased...? The error returned is
>whatever is returned from the step.
>
>> 
>> Track per-step failures, stop the update loop once a failure is observed,
>> and do not advance the state machine to the next step.
>
>Hmm, so this is actually a bunch of generic handling for each step, that really
>only works for the first one? Is the generic handling really needed?

We could special-case the first step, but that would add step-specific
error handling to the update loop. I think the simpler rule is to abort the
update on the first observed failure, regardless of which step reports it.

how about:

A TDX module update is a multi-step process, and any step can fail.

The current update flow continues to later steps after an error.
Continuing after a failure can cause the TDX module to enter an
unrecoverable state.

But certain failures during the initial module shutdown step should
simply return an error to userspace, so the update can be retried cleanly.

To preserve that recoverability, one option would be to abort the update
only for those failures, since they occur before any TDX module state is
changed. But special-casing specific failures in specific steps would
complicate the do-while() update loop for no benefit.

Simply abort update on any failure, at any step.

Track failures for each step, stop the update loop once a failure is
observed, and do not advance the state machine to the next step.

>
>> 
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> Reviewed-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
>> Reviewed-by: Tony Lindgren <tony.lindgren@xxxxxxxxxxxxxxx>
>> Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>
>> Reviewed-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
>> Link: https://lore.kernel.org/linux-coco/aQFmOZCdw64z14cJ@xxxxxxxxxx/ # [1]
>> ---
>> v9:
>>   - Avoid nested if/else by deferring failure accounting to ack_state().
>>   - Reduce indentation of the main flow.
>>   - Convert the failed flag into a counter. This avoids a conditional
>>     update of the flag; the counter can simply accumulate failures.
>> ---
>>  arch/x86/virt/vmx/tdx/seamldr.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
>> index 7befe4a08f33..48fe71319fea 100644
>> --- a/arch/x86/virt/vmx/tdx/seamldr.c
>> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
>> @@ -170,6 +170,7 @@ enum module_update_state {
>>  static struct update_ctrl {
>>  	enum module_update_state state;
>>  	int num_ack;
>> +	int num_failed;
>
>Was there past discussion on why it keeps a failed count? All we need to know is
>if anything failed right? So a bool is fine too?

Kiryl suggested a boolean, and I used that in earlier versions.  In v9 I
moved the failure tracking next to the ack counting in ack_state(). A
boolean still works, but it needs an extra conditional to latch the failure
state.

static void ack_state(struct update_ctrl *ctrl, int result)
{
	raw_spin_lock(&ctrl->lock);

-	ctrl->num_failed += !!ret;
+	if (!ctrl->failed)
+		ctrl->failed = !!ret;
	ctrl->num_ack++;
	if (ctrl->num_ack == num_online_cpus())
	if (ctrl->num_ack == num_online_cpus() && !ctrl->num_failed)
		__set_target_state(ctrl, ctrl->state + 1);

Using an int mainly to keep the failure and ack tracking similar
and avoid the extra if. (I put a note under --- to explain this.)

If you prefer, I can switch it back to bool.