Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS

[Huang, Kai]

On Fri, 2023-09-08 at 09:19 -0700, Dan Williams wrote:
> Huang, Kai wrote:
> > 
> > > 
> > > Changes since previous version:
> > > * Used ConfigFS interface instead of IOCTL interface.
> > > * Used polling model for Quote generation and dropped the event notification IRQ support.
> > 
> > Can you elaborate why the notification IRQ is dropped?
> 
> Because it was a pile of hacks and non-idiomatic complexity. It can come
> back when / if driver code can treat it like a typical interrupt.

Thanks.  Agreed.

> 
> [..]
> > >  
> > > +/*
> > > + * Intel's SGX QE implementation generally uses Quote size less
> > > + * than 8K (2K Quote data + ~5K of ceritificate blob).
> > > + */
> > > +#define GET_QUOTE_BUF_SIZE		SZ_8K
> > 
> > SZ_8K is defined in <linux/sizes.h>.  It seems it's not explicitly included. 
> > It's better to explicitly include it.
> > 
> > Btw, although the size of the certificate blob shouldn't change dramatically,
> > the Quote can also include the "QE Authentication Data", which can vary a lot
> > depending on different QE implementation (e.g., containing geography
> > information, etc).
> > 
> > I wish eventually there's some /sysfs entry to configure the size of Quote
> > buffer, but I guess it can be done in the future.
> 
> How would userspace have any idea of how big the quote buffer is to be
> able to set it? The output format at least needs to standardized within
> a given vendor's implementation, and future variation should be
> de-emphasized relative to getting to a common report format across
> vendors.

The Quoting enclave implements what to be included into the Quote.  The admin
who deploys the Quoting enclave knows the Quote size.

> 
> [..]
> > > +/*
> > > + * wait_for_quote_completion() - Wait for Quote request completion
> > > + * @quote_buf: Address of Quote buffer.
> > > + * @timeout: Timeout in seconds to wait for the Quote generation.
> > > + *
> > > + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> > > + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> > > + * while VMM processes the GetQuote request, and will change it to success
> > > + * or error code after processing is complete. So wait till the status
> > > + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> > > + */
> > > +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> > > +{
> > > +	int i = 0;
> > > +
> > > +	/*
> > > +	 * Quote requests usually take a few seconds to complete, so waking up
> > > +	 * once per second to recheck the status is fine for this use case.
> > > +	 */
> > > +	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> > > +		ssleep(1);
> > > +
> > > +	return (i == timeout) ? -ETIMEDOUT : 0;
> > > +}
> > 
> > Why can't we use wait_for_completion_timeout() provided by the kernel?
> > 
> > Btw, can we use _killable() variant?  Supporting timeout brings extra
> > complication, and I think supporting timeout isn't mandatory for the first
> > version.
> 
> It definitely is required even if interrupts were supported. The kernel
> needs to give up on stalled operations in a reasonable amount of time.

Thanks for explaining.