Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 5/22/22 7:52 PM, Kai Huang wrote:
> On Tue, 2022-05-17 at 07:54 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>> +struct tdx_report_req {
>>>> +	union {
>>>> +		__u8 reportdata[TDX_REPORTDATA_LEN];
>>>> +		__u8 tdreport[TDX_REPORT_LEN];
>>>> +	};
>>>> +};
>>>
>>> As a userspace ABI, one concern is this doesn't provide any space for future
>>> extension.  But probably it's OK since I don't see any possible additional
>>> input
>>> for now.  And although TDREPORT may have additional information in future
>>> generation of TDX but the spec says the size is 1024 so perhaps this won't
>>> change even in the future.
>>>
>>> Anyway will leave to others.
>>
>> IMO, if the spec changes in future we can revisit it.
> 
> I don't think the problem is how to revisit _this_ ABI.  The problem is, once it
> is introduced, you cannot break the ABI for the compatibility of supporting the
> userspace software written for old platforms.  So basically you cannot just
> increase the TDX_REPORT_LEN to a larger value.  This means if we have a larger
> than 1024B TDREPORT in future, the old userspace TD attestation software which
> uses this ABI will not work anymore on the new platforms.
> 
> If we need to make sure this ABI work for _ANY_ TDX platforms, I think we either
> need to make sure TDREPORT will always be 1024B for _ANY_ TDX platforms, or we
> need to have a flexible ABI which doesn't assume TDREPORT size.
> 
> For instance, we might need another IOCTL (or other interfaces such as /sysfs)
> to query the TDREPORT size, and make this IOCTL like below:
> 
> 	struct tdx_report_req {
> 		__u8 reportdata[TDX_REPORTDATA_LEN];
> 		__u8 reserved[...];
> 		__u8 tdreport[0];
> 	};
> 
> The actual TDREPORT buffer size is allocated by userspace after it queries the
> TDREPORT size.

I don't want to over design it just based on the assumption that length
will change in the future. I don't see any statement in spec supporting
the possibility of length changes. IMO, since the possibility is very
small, we don't need to overthink about it.

@maintainers, please let me know if you think otherwise.
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer