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

[Sathyanarayanan Kuppuswamy]

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