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

[Chao Gao]

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

I realize there's a potential issue with this update sequence:

old module (no compat detection) -> newer module (has compat detection) -> latest module

The problem arises during the second update. Userspace checks the currently
loaded module version and sees it supports compatibility detection, so it
expects the kernel to perform these checks. However, the kernel still thinks
the module lacks this capability because it never refreshes the module's
features after the first update.

Regarding disabling updates, I was thinking of an approach like the one below.
Do you think this is a workaround/hack?

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 2cf3a01d0b9c..50fe6373984d 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1192,9 +1192,7 @@ int tdx_module_shutdown(void)
	 * modules as new modules likely have higher handoff version.
	 */
	args.rcx = tdx_sysinfo.handoff.module_hv;
-
-	if (tdx_supports_update_compatibility(&tdx_sysinfo))
-		args.rcx |= TDX_SYS_SHUTDOWN_AVOID_COMPAT_SENSITIVE;
+	args.rcx |= TDX_SYS_SHUTDOWN_AVOID_COMPAT_SENSITIVE;
 
	ret = seamcall(TDH_SYS_SHUTDOWN, &args);
 
diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
index 9ade3028a5bd..c7f0853e8ce5 100644
--- a/drivers/virt/coco/tdx-host/tdx-host.c
+++ b/drivers/virt/coco/tdx-host/tdx-host.c
@@ -181,6 +181,11 @@ static void seamldr_init(struct device *dev)
		return;
	}
 
+	if (!tdx_supports_update_compatibility(tdx_sysinfo)) {
+		pr_info("Current TDX Module does not support update compatibility\n");
+		return;
+	}
+
	tdx_fwl = firmware_upload_register(THIS_MODULE, dev, "tdx_module",
					   &tdx_fw_ops, NULL);
	ret = PTR_ERR_OR_ZERO(tdx_fwl);