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

[Dan Williams <dan.j.williams@intel.com>]

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.

[..]
> >  
> > +/*
> > + * 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.

[..]
> > +/*
> > + * 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.