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

[Lisa Wang <wyihan@google.com>]

On Thu, Jul 23, 2026 at 04:44:08PM +0800, Xiaoyao Li wrote:
> > + */
> > +#define __tdx_vm_ioctl(vm, cmd, _flags, arg)				\
> 
> sev uses the name __vm_sev_ioctl, I think we need to keep them consistent.

While __vm_tdx_ioctl matches SEV, the TDX selftest follows a tdx_<scope>_*
naming convention (1. TDX prefix, 2. Scope: VM or vCPU). I named it
__tdx_vm_ioctl to keep the TDX codebase internally consistent.[1]

Do you think we should align with SEV's naming convention instead of
sticking with the internal TDX pattern?

[1]: https://lore.kernel.org/kvm/489f3c7b-db03-43dc-bb64-910a0fcba31e@intel.com/

> > +({									\
> > +	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.

I think the value of r is not important, because the ioctl failure
is already captured in errno.

We only need to fix the return values for SEV and TDX and have
TEST_ASSERT_* print formatted error logs with errno and hw_error.

- r ?: {tdx, sev}_cmd.c.hw_error;
+ r ? {tdx, sev}_cmd.c.hw_error : 0;

> > +})
> > +
> > +#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?

The reason we could not use __TEST_ASSERT_VM_VCPU_IOCTL() directly[2] is
because it formats ther return value as %i (32-bit), whereas
__tdx_vm_ioctl might return a u64 hardware error code.

I agree with your suggestion to introduce a new
TEST_ASSERT_TDX_VM_VCPU_IOCTL() macro to print out u64 hardware error
code properly.

[2]: https://lore.kernel.org/all/a58e2941-77f9-43cf-a54d-023506dd7eb0@linux.intel.com/

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

Thanks, noted.

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

Thanks, noted.

> The rest of the patch looks good to me.