Re: [PATCH v2 05/21] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs

[Xu Yilun <yilun.xu@linux.intel.com>]

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 58d890fe2100..6b47383d2958 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1905,6 +1905,16 @@ config INTEL_TDX_HOST
>  
>  	  If unsure, say N.
>  
> +config INTEL_TDX_MODULE_UPDATE
> +	bool "Intel TDX module runtime update"
> +	depends on TDX_HOST_SERVICES
> +	help
> +	  This enables the kernel to support TDX module runtime update. This
> +	  allows the admin to update the TDX module to the same or any newer
> +	  version without the need to terminate running TDX guests.

I'm wondering if it is better to put this option in
drivers/virt/coco/tdx-host. Just as TDX Connect, the
functionalities/uAPIs are exposed in /sys/devices/faux/tdx_host. Better
the 2 features could have aligned config pattern. The TDX Connect
configuration is here:

  https://lore.kernel.org/all/20251117022311.2443900-4-yilun.xu@linux.intel.com/

> +
> +	  If unsure, say N.
> +
>  config EFI
>  	bool "EFI runtime service support"
>  	depends on ACPI
> diff --git a/arch/x86/virt/vmx/tdx/Makefile b/arch/x86/virt/vmx/tdx/Makefile
> index 90da47eb85ee..26aea3531c36 100644
> --- a/arch/x86/virt/vmx/tdx/Makefile
> +++ b/arch/x86/virt/vmx/tdx/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-y += seamcall.o tdx.o
> +obj-$(CONFIG_INTEL_TDX_MODULE_UPDATE) += seamldr.o

And I'm wondering if we must disable seamldr core helpers if Update
uAPIs are not selected. TDX core now are expected to expose various
helpers for different features and is it necessary we have to mask
in/out all helpers in such a fine granularity? For example we may not
disable tdh_mem_sept_xx() helpers if KVM_INTEL is not selected.

BTW: We may finally get rid of the dependency between KVM_INTEL & TDX_HOST

-----8<-----
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 80527299f859..e3e90d1fcad3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1898,7 +1898,6 @@ config INTEL_TDX_HOST
        bool "Intel Trust Domain Extensions (TDX) host support"
        depends on CPU_SUP_INTEL
        depends on X86_64
-       depends on KVM_INTEL
        depends on X86_X2APIC
        select ARCH_KEEP_MEMBLOCK
        depends on CONTIG_ALLOC

[...]

> +static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> +{
> +	unsigned long flags;
> +	u64 vmcs;
> +	int ret;
> +
> +	if (!is_seamldr_call(fn))
> +		return -EINVAL;
> +
> +	/*
> +	 * SEAMRET from P-SEAMLDR invalidates the current VMCS.  Save/restore
> +	 * the VMCS across P-SEAMLDR SEAMCALLs to avoid clobbering KVM state.
> +	 * Disable interrupts as KVM is allowed to do VMREAD/VMWRITE in IRQ
> +	 * context (but not NMI context).
> +	 */
> +	local_irq_save(flags);
> +
> +	asm goto("1: vmptrst %0\n\t"
> +		 _ASM_EXTABLE(1b, %l[error])
> +		 : "=m" (vmcs) : : "cc" : error);
> +
> +	ret = seamldr_prerr(fn, args);

As I mentioned, just use seamcall_prerr(). This perfectly illustrates the
main difference between normal seamcalls & seamldr_calls - the additional
VMCS handling.

> +
> +	/*
> +	 * Restore the current VMCS pointer.  VMPTSTR "returns" all ones if the
> +	 * current VMCS is invalid.
> +	 */
> +	if (vmcs != -1ULL) {
> +		asm goto("1: vmptrld %0\n\t"
> +			 "jna %l[error]\n\t"
> +			 _ASM_EXTABLE(1b, %l[error])
> +			 : : "m" (vmcs) : "cc" : error);
> +	}
> +
> +	local_irq_restore(flags);
> +	return ret;
> +
> +error:
> +	local_irq_restore(flags);
> +
> +	WARN_ONCE(1, "Failed to save/restore the current VMCS");
> +	return -EIO;
> +}
> -- 
> 2.47.3
>