Re: [PATCH v4 21/24] x86/virt/tdx: Avoid updates during update-sensitive operations

[Chao Gao]

>>  int tdx_module_shutdown(void)
>>  {
>>  	struct tdx_module_args args = {};
>> -	int ret, cpu;
>> +	u64 ret;
>> +	int cpu;
>>  
>>  	/*
>>  	 * Shut down the TDX Module and prepare handoff data for the next
>> @@ -1189,9 +1192,21 @@ int tdx_module_shutdown(void)
>>  	 * modules as new modules likely have higher handoff version.
>>  	 */
>>  	args.rcx = tdx_sysinfo.handoff.module_hv;
>> -	ret = seamcall_prerr(TDH_SYS_SHUTDOWN, &args);
>> -	if (ret)
>> -		return ret;
>> +
>> +	if (tdx_supports_update_compatibility(&tdx_sysinfo))
>> +		args.rcx |= TDX_SYS_SHUTDOWN_AVOID_COMPAT_SENSITIVE;
>> +
>> +	ret = seamcall(TDH_SYS_SHUTDOWN, &args);
>> +
>> +	/*
>> +	 * Return -EBUSY to signal that there is one or more ongoing flows
>> +	 * which may not be compatible with an updated TDX module, so that
>> +	 * userspace can retry on this error.
>> +	 */
>> +	if ((ret & TDX_SEAMCALL_STATUS_MASK) == TDX_UPDATE_COMPAT_SENSITIVE)
>> +		return -EBUSY;
>> +	else if (ret)
>> +		return -EIO;
>> 
>
>The changelog says "doing nothing" isn't an option, and we need to depend on
>TDH.SYS.SHUTDOWN to catch such incompatibilities.
>
>To me this means we cannot support module update if TDH.SYS.SHUTDOWN doesn't
>support this "AVOID_COMPAT_SENSITIVE" feature, because w/o it we cannot tell
>whether the update is happening during any sensitive operation.
>

Good point.

I'm fine with disabling updates in this case. The only concern is that it would
block even perfectly compatible updates, but this only impacts a few older
modules, so it shouldn't be a big problem. And the value of supporting old
modules will also diminish over time.

But IMO, the kernel's incompatibility check is intentionally best effort, not a
guarantee. For example, the kernel doesn't verify if the module update is
compatible with the CPU or P-SEAMLDR. So non-compatible updates may slip through
anyway, and the expectation for users is "run non-compatible updates at their
own risk". Given this, allowing updates when one incompatibility check is
not supported (i.e., AVOID_COMPAT_SENSITIVE) is also acceptable. At minimum,
users can choose not to perform updates if the module lacks
AVOID_COMPAT_SENSITIVE support.

I'm fine with either approach, but slightly prefer disabling updates in
this case. Let's see if anyone has strong opinions on this.