Re: [PATCH v3 2/4] x86/tdx: Add tdx_hcall_get_quote() API support

[Aubrey Li <aubrey.li@linux.intel.com>]

On 2022/4/20 下午3:16, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 4/19/22 8:39 PM, Aubrey Li wrote:
>> On 2022/4/16 上午6:01, Kuppuswamy Sathyanarayanan wrote:
>>> Attestation is the process used by two un-trusted entities to prove to
>>> each other that it can be trusted. In TDX guest, attestation is mainly
>>> used to verify the trustworthiness of a TD to the 3rd party key
>>> servers.
>>>
>>> First step in the attestation process is to generate the TDREPORT data.
>>> This support is added using tdx_mcall_tdreport() API. The second stage
>>> in the attestation process is for the guest to request the VMM generate
>>> and sign a quote based on the TDREPORT acquired earlier. More details
>>> about the steps involved in attestation process can be found in TDX
>>> Guest-Host Communication Interface (GHCI) for Intel TDX 1.5, section
>>> titled "TD attestation"
>>>
>>> Add tdx_hcall_get_quote() helper function to implement the GetQuote
>>> hypercall.
>>>
>>> More details about the GetQuote TDVMCALL are in the Guest-Host
>>> Communication Interface (GHCI) Specification, sec 3.3, titled
>>> "VP.VMCALL<GetQuote>".
>>>
>>> This will be used by the TD attestation driver in follow-on patches.
>>>
>>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>   arch/x86/coco/tdx/tdx.c    | 38 ++++++++++++++++++++++++++++++++++++++
>>>   arch/x86/include/asm/tdx.h |  2 ++
>>>   2 files changed, 40 insertions(+)
>>>
>>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>>> index 3e409b618d3f..c259d81a5d7f 100644
>>> --- a/arch/x86/coco/tdx/tdx.c
>>> +++ b/arch/x86/coco/tdx/tdx.c
>>> @@ -21,6 +21,7 @@
>>>     /* TDX hypercall Leaf IDs */
>>>   #define TDVMCALL_MAP_GPA        0x10001
>>> +#define TDVMCALL_GET_QUOTE        0x10002
>>>     /* MMIO direction */
>>>   #define EPT_READ    0
>>> @@ -144,6 +145,43 @@ long tdx_mcall_tdreport(void *data, void *reportdata)
>>>   }
>>>   EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
>>>   +/*
>>> + * tdx_hcall_get_quote() - Generate TDQUOTE using TDREPORT_STRUCT.
>>> + *
>>> + * @data        : Address of 8KB GPA memory which contains
>>> + *                TDREPORT_STRUCT.
>>> + * @len        : Length of the GPA in bytes.
>>> + *
>>> + * return 0 on success or failure error number.
>>> + */
>>> +long tdx_hcall_get_quote(void *data, u64 len)
>>> +{
>>> +    u64 ret;
>>> +
>>> +    /*
>>> +     * Use confidential guest TDX check to ensure this API is only
>>> +     * used by TDX guest platforms.
>>> +     */
>>> +    if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>>> +        return -EINVAL;
>>> +
>>> +    /*
>>> +     * Pass the physical address of tdreport data to the VMM
>>> +     * and trigger the tdquote generation. Quote data will be
>>> +     * stored back in the same physical address space. More info
>>> +     * about ABI can be found in TDX Guest-Host-Communication
>>> +     * Interface (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
>>> +     */
>>> +    ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
>>> +                 len, 0, 0);
>>> +
>>
>> I commented here in v2 but no response, so let me try again.
>>
>> IIUC, virt_to_phys(data) (GPA) will be stored in the register when
>> TDCALL brings the context back to the VMX root mode, and hypervisor(QEMU)
>> will find the mapped host virtual address(HVA) with the GPA in the register,
>> and the subsequent ops will be HVA<->HVA in hypervisor, EPT will not be
>> involved so no need to cc_mkdec() this GPA.
>>
>> Please help to correct me if I was wrong.
> 
> It was done to meet the expectation from VMM. For shared GPA address,
> VMM expects shared bit set. All cc_mkdec() does is to set this bit.
> 

It seems not a good idea to make the guest aware of the shared bit IMHO.
I didn't see it specified in GHCI, there should be at least a comment here
to explain this behavior.

Thanks,
-Aubrey