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

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

On 9/9/2026 2:08 AM, Ackerley Tng wrote:
> Lisa Wang <wyihan@google.com> 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/489f3c7b-db03-43dc-bb64-910a0fcba31e@intel.com/
>>
> 
> 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.)

Yeah, either adjusting this patch to follow SEV or renaming existing SEV
MACROs is OK.

Keep the patch as-is and leave the SEV work to future are OK.

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

No. It is not correct because hw_error can be 0 when r != 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.

yeah. This idea can work.

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

Since we cannot simply use hw_error to replace ret, there will be not 32bit
vs 64bit issue. But ...

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

... if we want to print hw_error as well, we still need a new macro.

>> [2]: https://lore.kernel.org/all/a58e2941-77f9-43cf-a54d-023506dd7eb0@linux.intel.com/
>>
> 
> 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 tdx_vcpu_ioctl() will use it as well?

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