Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Huang, Kai]

On Thu, 2023-05-04 at 00:12 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Kai,
> 
> On 5/1/23 5:48 AM, Huang, Kai wrote:
> > On Sun, 2023-04-30 at 23:03 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > Hi Kai,
> > > 
> > > On 4/28/23 6:49 AM, Huang, Kai wrote:
> 
> [...]
> 
> > 
> > > 
> > > > 
> > > > > +	args.r10 = TDX_HYPERCALL_STANDARD;
> > > > > +	args.r11 = TDVMCALL_GET_QUOTE;
> > > > > +	args.r12 = cc_mkdec(virt_to_phys(tdquote));
> > 
> > Btw can we just use __pa()?  To be honest I am ignorant on the difference
> > between virt_to_phys() and __pa(), i.e. when should we use which.
> 
> The following link explains the difference. 
> 
> https://lkml.indiana.edu/hypermail/linux/kernel/0607.0/1607.html
> 
> In x86 ARCH, virt_to_phys() directly calls __pa(). So both are same.
> 
> But it looks like the recommendation is to use virt_to_phys().

Thanks!

[...]

> > > > 
> > > > > +
> > > > > +/**
> > > > > + * struct quote_entry - Quote request struct
> > > > > + * @valid: Flag to check validity of the GetQuote request.
> > > > > + * @buf: Kernel buffer to share data with VMM (size is page aligned).
> > > > > + * @buf_len: Size of the buf in bytes.
> > > > > + * @compl: Completion object to track completion of GetQuote request.
> > > > > + */
> > > > > +struct quote_entry {
> > > > > +	bool valid;
> > > > > +	void *buf;
> > > > > +	size_t buf_len;
> > > > > +	struct completion compl;
> > > > > +};
> > > > 
> > > > We have a static global @qentry below.
> > > > 
> > > > The buffer size is a fixed size (16K), why do we need @buf_len here?
> > > 
> > > I have added it to support buf length changes in future (like adding a
> > > command line option to allow user change the GET_QUOTE_MAX_SIZE).  Also,
> > > IMO, using buf_len is more readable than just using GET_QUOTE_MAX_SIZE
> > > macro in all places.
> > > 
> > > > 
> > > > And why do we need @valid?  It seems ...
> > > 
> > > As a precaution against spurious event notification. I also believe that in
> > > the future, event notification IRQs may be used for other purposes such as
> > > vTPM or other TDVMCALL services, and that this handler may be triggered
> > > without a valid GetQuote request. So, before we process the IRQ, I want to
> > > make sure we have a valid buffer.
> > 
> > OK.  This is an shared IRQ basically, so we need to track whether we have any
> > GetQuote request pending.
> > 
> > However I am wondering whether we need a dedicated @valid for this purpose.  If
> > I read correctly, we will make sure the buffer is zero-ed when there's no
> > request pending, thus can we just use some member in 'tdx_quote_hdr' to track?
> > 
> > For instance, per-GHCI the 'version' must be set to 1 for a valid request.  And
> > I think in a foreseeable future we can also assume @in_len being the size of
> > TDREPORT_STRUCT.  Can we use one of them (i.e. version) for this purpose?
> 
> IMO, it is better to track it from the guest end (with a dedicated @valid). Since
> quote request buffer is shared with the VMM, we should not just rely on its value
> to decide whether we have an active request. If needed, in addition to the "@valid"
> check we can also include check for @version. Also, I think we will not lose much
> using a "bool" value to track the quote buffer valid status.

But the header is never written by the VMM, right?

If the VMM is buggy then the guest is not guaranteed to work properly anyway.

So to me there's no need to have additional @valid.

[...]
> > > > 

> > > > > +/* struct tdx_quote_hdr: Format of Quote request buffer header.
> > > > > + * @version: Quote format version, filled by TD.
> > > > > + * @status: Status code of Quote request, filled by VMM.
> > > > > + * @in_len: Length of TDREPORT, filled by TD.
> > > > > + * @out_len: Length of Quote data, filled by VMM.
> > > > > + * @data: Quote data on output or TDREPORT on input.
> > > > > + *
> > > > > + * More details of Quote data header can be found in TDX
> > > > > + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> > > > > + * section titled "TDG.VP.VMCALL<GetQuote>"
> > > > > + */
> > > > > +struct tdx_quote_hdr {
> > > > > +	__u64 version;
> > > > > +	__u64 status;
> > > > > +	__u32 in_len;
> > > > > +	__u32 out_len;
> > > > > +	__u64 data[];
> > > > > +};
> > > > 
> > > > This structure is weird.  It's a header, but it contains the dynamic-size
> > > > buffer.  If you have __data[] in this structure, then it is already a buffer for
> > > > the entire Quote, no?  Then should we just call it 'struct tdx_quote'?
> > > > 
> > > > Or do you want to remove __data[]?
> > > 
> > > I can name it as struct tdx_quote_data
> > 
> > If go with route, why not just 'tdx_quote', or 'tdx_tdquote'?
> > 
> > Or, actually I think 'tdx_quote' (or 'tdx_tdquote') seems to be the format of
> > the _true_ Quote, so perhaps we want 'struct tdx_quote_req_buf'?
> > 
> 
> Since this buffer is used for both request/response, we can just use
> struct tdx_quote_buf.

Not my preference but will leave to others.