Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs

[Dave Hansen <dave.hansen@intel.com>]

On 1/23/26 06:55, Chao Gao wrote:
...
> +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;

Why is this here? We shouldn't be silently papering over kernel bugs.
This is a WARN_ON() at *best*, but it also begs the question of how a
non-SEAMLDR call even got here.

> +	/*
> +	 * 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).
> +	 */

I think you mean:

	WARN_ON(in_nmi());

> +	local_irq_save(flags);
> +
> +	asm goto("1: vmptrst %0\n\t"
> +		 _ASM_EXTABLE(1b, %l[error])
> +		 : "=m" (vmcs) : : "cc" : error);

I'd much rather this be wrapped up in a helper function. We shouldn't
have to look at the horrors of inline assembly like this.

But this *REALLY* wants the KVM folks to look at it. One argument is
that with the inline assembly this is nice and self-contained. The other
argument is that this completely ignores all existing KVM infrastructure
and is parallel VMCS management.

I'd be shocked if this is the one and only place in the whole kernel
that can unceremoniously zap VMX state.

I'd *bet* that you don't really need to do the vmptrld and that KVM can
figure it out because it can vmptrld on demand anyway. Something along
the lines of:

	local_irq_disable();
	list_for_each(handwaving...)
		vmcs_clear();
	ret = seamldr_prerr(fn, args);
	local_irq_enable();	

Basically, zap this CPU's vmcs state and then make KVM reload it at some
later time.

I'm sure Sean and Paolo will tell me if I'm crazy.

> diff --git a/drivers/virt/coco/tdx-host/Kconfig b/drivers/virt/coco/tdx-host/Kconfig
> index e58bad148a35..6a9199e6c2c6 100644
> --- a/drivers/virt/coco/tdx-host/Kconfig
> +++ b/drivers/virt/coco/tdx-host/Kconfig
> @@ -8,3 +8,13 @@ config TDX_HOST_SERVICES
>  
>  	  Say y or m if enabling support for confidential virtual machine
>  	  support (CONFIG_INTEL_TDX_HOST). The module is called tdx_host.ko
> +
> +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 another compatible
> +	  version without the need to terminate running TDX guests.

... as opposed to the method that the kernel has to update the module
without terminating guests? ;)

> +	  If unsure, say N.

Let's call this:

 config
INTEL_TDX_ONLY_DISABLE_THIS_IF_YOU_HATE_SECURITY_AND_IF_YOU_DO_WHY_ARE_YOU_RUNNING_TDX?

Can we have question marks in config symbol names? ;)

But, seriously, what the heck? Who would disable security updates for
their confidential computing infrastructure? Is this some kind of
intelligence test for our users so that if someone disables it we can
just laugh at them?