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/[email protected]/ > > +({ \ > > + 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/[email protected]/ > <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.
