On 9/9/2026 2:08 AM, Ackerley Tng wrote:
> 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.)
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/[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 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.
>
