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

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

>I don't hate it. Seems more scalable than current approach.
>
>See some comments below.
>
>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
>> index 128e6ffba736..fa9bb6d47a87 100644
>> --- a/arch/x86/virt/vmx/tdx/tdx.c
>> +++ b/arch/x86/virt/vmx/tdx/tdx.c
>> @@ -226,22 +226,23 @@ static int build_tdx_memlist(struct list_head *tmb_list)
>> 	return ret;
>>  }
>>  
>> -static int read_sys_metadata_field(u64 field_id, u64 *data)
>> +static int read_sys_metadata_field(u64 *field_id, void *ptr)
>
>Keeping the same name for completely different functionality?

how about read_sys_metadata_fields() or read_sys_metadata_all()?

>
>>  {
>> 	struct tdx_module_args args = {};
>> 	int ret;
>>  
>> 	/*
>> -	 * TDH.SYS.RD -- reads one global metadata field
>> -	 *  - RDX (in): the field to read
>> -	 *  - R8 (out): the field data
>> +	 * TDH.SYS.RDALL -- reads all global metadata fields
>> +	 *  - RDX (in): the physical address of the buffer to store
>> +	 *  - R8 (in/out): the initial field ID to read (in) and
>> +	 *		   the next field ID to read (out).
>> 	 */
>> -	args.rdx = field_id;
>> -	ret = seamcall_prerr_ret(TDH_SYS_RD, &args);
>> +	args.rdx = __pa(ptr);
>
>Maybe take virtual address (unsigned long) of the buffer as an argument
>to the function. And use virt_to_phys() here.
>
>This way there's no need in cast on caller side.

Sure. Will do.

>
>> +	args.r8  = *field_id;
>> +	ret = seamcall_prerr_ret(TDH_SYS_RDALL, &args);
>> 	if (ret)
>> 		return ret;
>> -
>> -	*data = args.r8;
>> +	*field_id = args.r8;
>>  
>> 	return 0;
>
>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.

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

What do you think?