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

[<dan.j.williams@intel.com>]

Chao Gao wrote:
> >>  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.

Doing nothing in the kernel is fine. This is a tooling problem.

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

Do not make Linux carry short lived one-off complexity. Make userspace
do a "if $module_version < $min_module_version_for_compat_detect" and
tell the user to update at their own risk if that minimum version is not
met. Linux should be encouraging the module to be better, not
accommodate every early generation miss like this with permanent hacks.