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

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

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().


> 
> Also, you _may_ want to add a comment why "cc_mkdec()" is used.  By the nature
> of this TDVMCALL, it's obvious the buffer needs to be shared, and the VMM must
> check whether the buffer is actually shared, no matter whether the "shared-bit"
> is set here or not.
> 
> So to me it's just requested by the GHCI spec that we need to include the
> "shared-bit", but it _seems_ the GHCI spec doesn't explicitly say we need to do
> that because it only says "Shared buffer as input".  So looks a comment can help
> to clarify a little bit.

I will add it.

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



>>>> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
>>>> index a6a2098c08ff..500cdfa025ad 100644
>>>> --- a/include/uapi/linux/tdx-guest.h
>>>> +++ b/include/uapi/linux/tdx-guest.h
>>>> @@ -17,6 +17,12 @@
>>>>  /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>>>>  #define TDX_REPORT_LEN                  1024
>>>>  
>>>> +/* TD Quote status codes */
>>>> +#define GET_QUOTE_SUCCESS               0
>>>> +#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
>>>> +#define GET_QUOTE_ERROR                 0x8000000000000000
>>>> +#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
>>>> +
>>>>  /**
>>>>   * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
>>>>   *
>>>> @@ -30,6 +36,35 @@ struct tdx_report_req {
>>>>  	__u8 tdreport[TDX_REPORT_LEN];
>>>>  };
>>>>  
>>>> +/* 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.

>>
>>>
>>>> +
>>>> +/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
>>>> + * @buf: Address of user buffer that includes TDREPORT. Upon successful
>>>> + *	 completion of IOCTL, output is copied back to the same buffer.
>>>
>>> This description isn't precise.  "user buffer that includes TDREPORT" doesn't
>>> tell application writer where to put the TDREPORT at all.  We need to explicitly
>>> call out the buffer starts with 'tdx_quote_hdr' followed by TDREPORT
>>> immediately.
>>
>> I have specified it in struct tdx_quote_hdr.data help content.
> 
> Perhaps I missed something but I didn't say at any place this is clearly
> documented.  The comment around @data above certainly doesn't.
> 
> Just say something like:
> 
> 	@buf: The userspace pointer which points to the
> 	      'struct tdx_quote_req_buf' (whatever the final name)
> 
>>
>>>  
>>>> + * @len: Length of the Quote buffer.
>>>> + */
>>>> +struct tdx_quote_req {
>>>> +	__u64 buf;
>>>> +	__u64 len;
>>>> +};
>>>> +
>>>>  /*
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer