Re: [PATCH v14 03/22] KVM: selftests: Initialize the TDX VM

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 7/23/2026 7:13 AM, Lisa Wang wrote:
> From: Sagi Shahar <sagis@google.com>
> 
> Add tdx_init_vm() to handle the mandatory VM-level initialization
> sequence required for Intel TDX.
> 
> For TDX, the guest's CPUID configuration must be "sealed" during
> KVM_TDX_INIT_VM before any vCPUs are created. This is necessary because
> the TDX hardware directly virtualizes CPUID and includes the
> configuration in the guest's initial security measurement.
> 
> The helper calculates the required CPUID values by filtering the host-
> supported bits (kvm_get_supported_cpuid) against the "directly
> configurable" bits reported by KVM_TDX_CAPABILITIES, ensuring
> compliance with the strict requirements of the TDH.MNG.INIT SEAMCALL.

<snip>

> +/*
> + * TDX ioctls
> + * Use underscores to avoid collisions with struct member names.
> + */
> +#define __tdx_vm_ioctl(vm, cmd, _flags, arg)				\

sev uses the name __vm_sev_ioctl, I think we need to keep them consistent.

> +({									\
> +	u64 r;								\
> +									\
> +	union {								\
> +		struct kvm_tdx_cmd c;					\
> +		unsigned long raw;					\
> +	} tdx_cmd = { .c = {						\
> +		.id = (cmd),						\
> +		.flags = (u32)(_flags),					\
> +		.data = (u64)(arg),					\
> +	} };								\
> +									\
> +	r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw);	\
> +	r ?: tdx_cmd.c.hw_error;					\

I know it takes the same handling from __vm_sev_ioctl(). But I think the 
handling for hw_error is not correct, at least for TDX (I didn't check 
for SEV).

the hw_error is the additional info, to tell the SEAMCALL return code, 
when the IOCTL fails. KVM requires hw_error to be in the input, and KVM 
puts the SEAMCALL return code into hw_error when the IOCTL fails due to 
SEAMCALL failure. That means, when r == 0, the hw_error is always 0.

I think we need to provide hw_error along with r to the caller so that 
caller can print them together.

> +})
> +
> +#define tdx_vm_ioctl(vm, cmd, flags, arg)				\
> +({									\
> +	u64 ret = __tdx_vm_ioctl(vm, cmd, flags, arg);			\
> +									\
> +	if (ret) {							\
> +		TEST_ASSERT(!ret,					\
> +			    "%s failed, rc: 0x%llx errno: %i (%s)",	\
> +			    #cmd, (unsigned long long)ret,		\
> +			    errno, strerror(errno));			\

The if() looks silly. Why add it? And why change it from 
__TEST_ASSERT_VM_VCPU_IOCTL() in the v13?

Considering the suggestion of hw_error above, I think we need to 
introduce the TEST_ASSERT_TDX_VM_VCPU_IOCTL() which accepts additional 
hw_error?

<snip>
> diff --git a/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c b/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> new file mode 100644
> index 000000000000..e1ffb67a106c
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "processor.h"
> +#include "tdx/tdx_util.h"
> +
> +static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)

make it const, is better.

<snip>

> +
> +void tdx_init_vm(struct kvm_vm *vm, u64 attributes)
> +{
> +	struct kvm_tdx_init_vm *init_vm;
> +	const struct kvm_cpuid2 *tmp;
> +	struct kvm_cpuid2 *cpuid;
> +
> +	tmp = kvm_get_supported_cpuid();
> +
> +	cpuid = allocate_kvm_cpuid2(tmp->nent);
> +	memcpy(cpuid, tmp, kvm_cpuid2_size(tmp->nent));
> +	tdx_filter_cpuid(vm, cpuid);
> +
> +	init_vm = calloc(1, sizeof(*init_vm) +
> +			 sizeof(init_vm->cpuid.entries[0]) * cpuid->nent);
> +	TEST_ASSERT(init_vm, "init_vm allocation failed");
> +
> +	memcpy(&init_vm->cpuid, cpuid, kvm_cpuid2_size(cpuid->nent));
> +	free(cpuid);
> +
> +	init_vm->attributes = attributes;

Besides CPUID, it only allows attributes to be configure but leave XFAM 
as 0. I think the changelog needs to explain why we need to configure 
attributes.

The rest of the patch looks good to me.

> +
> +	tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);
> +
> +	free(init_vm);
> +}
>