Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version

[Chao Gao <chao.gao@intel.com>]

>> >Hm. Isn't it buggy?
>> >
>> >Caller expects to see field_id == -1 to exit loop, but you never set it
>> >in case of an error. It will result in endless loop if error happens not on
>> >the first iteration.
>> 
>> The caller checks the return value and bails out if there was an error.
>
>I misread it. Missed the break.
>
>> >
>> >Drop the branch and always return ret.
>> 
>> Setting field_id to -1 on error appears unnecessary since callers must check
>> the return value anyway. And, even if args.r8 were copied to field_id
>> on error, this wouldn't guarantee that field_id would be set to -1, as
>> SEAMCALLs may encounter #GP/#UD exceptions where r8 remains unchanged.
>> 
>> Given this, I prefer to leave field_id as an undefined value on error, and
>> callers should not read/use it when an error occurs.
>
>It is not undefined. TDX module sets R8 to -1 in case of error.

Yes, I saw it. That's TDX module ABI. It doesn't necessarily have to be the
semantics of this kernel API. So, we have two choices:

1. Follow the TDX module, i.e., set field_id to -1 on error. Then we should do:

	if (ret)
		*field_id = -1;
	else
		*field_id = args.r8;

 to cover #GP/#UD cases.

2. Leave field_id undefined on error, as in this patch.

I don't see the value of setting field_id to -1 on error if callers are
expected to check the return value.