Lisa Wang <[email protected]> writes:
> 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]/
>
Given that __vm_sev_ioctl and __tdx_vm_ioctl aren't likely to appear
next to each other, I think it's better to have the tdx prefix earlier
to keep the TDX code consistent. To move things along, I think we could
continue as-is and not swap to align with sev.
(The sev functions actually look kind of inconsistent in sev.h, but
that's a discussion for another series.)
>> > +({
>> > \
>> > + 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;
>
I didn't look in detail, perhaps Lisa could look into these:
+ What are the possible values of r? Is it always going to be 1 on
error?
+ Is r always positive or negative?
+ Is tdx_cmd.c.hw_error always positive or negative? It's probably not
one of the standard Linux errors, right?
Perhaps squashing hw_error together with a retval conflates the two.
In the lower-level __tdx_vm_ioctl() we could pass a hw_error and have
the macro set hw_error? That will allow the caller to print both.
>> > +})
>> > +
>> > +#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]/
>
Is tdx_vm_ioctl() the only place where TEST_ASSERT_TDX_VM_VCPU_IOCTL()
is going to be used though? If so, maybe we should defer introducing
TEST_ASSERT_TDX_VM_VCPU_IOCTL() till later.
I think the issue with if (ret) is just that TEST_ASSERT(!ret) already
does that same check, and so we can drop the if (ret) part.
>> <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.
>
