[PATCH v3 0/4] Add TDX Guest Attestation support

[PATCH v3 0/4] Add TDX Guest Attestation support

[Kuppuswamy Sathyanarayanan]

Hi All,

Intel's Trust Domain Extensions (TDX) protect guest VMs from malicious
hosts and some physical attacks. VM guest with TDX support is called
as TD Guest.

In TD Guest, the attestation process is used to verify the 
trustworthiness of TD guest to the 3rd party servers. Such attestation
process is required by 3rd party servers before sending sensitive
information to TD guests. One usage example is to get encryption keys
from the key server for mounting the encrypted rootfs or secondary drive.
    
Following patches add the attestation support to TDX guest which
includes attestation user interface driver, user agent example, and
related hypercall support.

Patch titled "platform/x86: intel_tdx_attest: Add TDX Guest attestation
interface driver" adds the attestation driver support. This is supposed
to be reviewed by platform-x86 maintainers.

Rest of the patches are intended for x86 maintainers review.


Dependencies:
--------------

This feature has dependency on TDX guest core patch set series.

https://lore.kernel.org/all/20220218161718.67148-1-kirill.shutemov@linux.intel.com/T/

Changes since v2:
 * As per Han's suggestion, modified the attestation driver to use
   platform device driver model.
 * Modified tdx_hcall_get_quote() and tdx_mcall_tdreport() APIs to
   return TDCALL error code instead of generic error info (like -EIO).
 * Removed attestation test app patch from this series to simplify
   the patchset and review process. Test app patches will be submitted
   once attestation support patches are merged.
 * Since patches titled "x86/tdx: Add SetupEventNotifyInterrupt TDX
   hypercall support" and "x86/tdx: Add TDX Guest event notify
   interrupt vector support" are related, combining them into a
   single patch.

Changes since v1:
 * Moved test driver from "tools/tdx/attest/tdx-attest-test.c" to
   "tools/arch/x86/tdx/attest/tdx-attest-test.c" as per Hans review
   suggestion.
 * Minor commit log and comment fixes in patches titled
   "x86/tdx: Add tdx_mcall_tdreport() API support" and "x86/tdx:
   Add tdx_hcall_get_quote() API support"
 * Extended tdx_hcall_get_quote() API to accept GPA length as argument
   to accomodate latest TDQUOTE TDVMCALL related specification update.
 * Added support for tdx_setup_ev_notify_handler() and
   tdx_remove_ev_notify_handler() in patch titled "x86/tdx: Add TDX
   Guest event notify interrupt vector support"


Kuppuswamy Sathyanarayanan (4):
  x86/tdx: Add tdx_mcall_tdreport() API support
  x86/tdx: Add tdx_hcall_get_quote() API support
  x86/tdx: Add TDX Guest event notify interrupt support
  platform/x86: intel_tdx_attest: Add TDX Guest attestation interface
    driver

 arch/x86/coco/tdx/tdx.c                       | 161 ++++++++++
 arch/x86/include/asm/hardirq.h                |   3 +
 arch/x86/include/asm/idtentry.h               |   4 +
 arch/x86/include/asm/irq_vectors.h            |   7 +-
 arch/x86/include/asm/tdx.h                    |   8 +
 arch/x86/kernel/irq.c                         |   7 +
 drivers/platform/x86/intel/Kconfig            |   2 +-
 drivers/platform/x86/intel/Makefile           |   1 +
 drivers/platform/x86/intel/tdx/Kconfig        |  13 +
 drivers/platform/x86/intel/tdx/Makefile       |   3 +
 .../platform/x86/intel/tdx/intel_tdx_attest.c | 302 ++++++++++++++++++
 include/uapi/misc/tdx.h                       |  42 +++
 12 files changed, 551 insertions(+), 2 deletions(-)
 create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
 create mode 100644 drivers/platform/x86/intel/tdx/Makefile
 create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
 create mode 100644 include/uapi/misc/tdx.h

-- 
2.25.1

[PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support

[Kuppuswamy Sathyanarayanan]

In TDX guest, attestation is mainly used to verify the trustworthiness
of a TD to the 3rd party key servers. First step in attestation process
is to get the TDREPORT data and the generated data is further used in
subsequent steps of the attestation process. TDREPORT data contains
details like TDX module version information, measurement of the TD,
along with a TD-specified nonce

Add a wrapper function (tdx_mcall_tdreport()) to get the TDREPORT from
the TDX Module.

More details about the TDREPORT TDCALL can be found in TDX Guest-Host
Communication Interface (GHCI) for Intel TDX 1.5, section titled
"TDCALL [MR.REPORT]".

Steps involved in attestation process can be found in TDX Guest-Host
Communication Interface (GHCI) for Intel TDX 1.5, section titled
"TD attestation"

This API will be mainly used by the attestation driver. Attestation
driver support will be added by upcoming 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    | 46 ++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/tdx.h |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 03deb4d6920d..3e409b618d3f 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -11,10 +11,12 @@
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
+#include <asm/io.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
 #define TDX_GET_VEINFO			3
+#define TDX_GET_REPORT			4
 #define TDX_ACCEPT_PAGE			6
 
 /* TDX hypercall Leaf IDs */
@@ -34,6 +36,10 @@
 #define VE_GET_PORT_NUM(e)	((e) >> 16)
 #define VE_IS_IO_STRING(e)	((e) & BIT(4))
 
+/* TDX Module call error codes */
+#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
+#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
+
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
  * return code.
@@ -98,6 +104,46 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
 }
 
+/*
+ * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
+ *
+ * @data        : Address of 1024B aligned data to store
+ *                TDREPORT_STRUCT.
+ * @reportdata  : Address of 64B aligned report data
+ *
+ * return 0 on success or failure error number.
+ */
+long tdx_mcall_tdreport(void *data, void *reportdata)
+{
+	u64 ret;
+
+	/*
+	 * Check for a valid TDX guest to ensure this API is only
+	 * used by TDX guest platform. Also make sure "data" and
+	 * "reportdata" pointers are valid.
+	 */
+	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EINVAL;
+
+	/*
+	 * Pass the physical address of user generated reportdata
+	 * and the physical address of out pointer to store the
+	 * TDREPORT data to the TDX module to generate the
+	 * TD report. Generated data contains measurements/configuration
+	 * data of the TD guest. More info about ABI can be found in TDX
+	 * Guest-Host-Communication Interface (GHCI), sec titled
+	 * "TDG.MR.REPORT".
+	 */
+	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
+				virt_to_phys(reportdata), 0, 0, NULL);
+
+	if (ret)
+		return TDCALL_RETURN_CODE(ret);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 020c81a7c729..a151f69dd6ef 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -67,6 +67,8 @@ void tdx_safe_halt(void);
 
 bool tdx_early_handle_ve(struct pt_regs *regs);
 
+long tdx_mcall_tdreport(void *data, void *reportdata);
+
 #else
 
 static inline void tdx_early_init(void) { };
-- 
2.25.1

Re: [PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support

[Kai Huang]

On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, attestation is mainly used to verify the trustworthiness
> of a TD to the 3rd party key servers. 
> 

"key servers" is only a use case of using the attestation service. This sentence
looks not accurate.

> First step in attestation process
> is to get the TDREPORT data and the generated data is further used in
> subsequent steps of the attestation process. TDREPORT data contains
> details like TDX module version information, measurement of the TD,
> along with a TD-specified nonce
> 
> Add a wrapper function (tdx_mcall_tdreport()) to get the TDREPORT from
> the TDX Module.
> 
> More details about the TDREPORT TDCALL can be found in TDX Guest-Host
> Communication Interface (GHCI) for Intel TDX 1.5, section titled
> "TDCALL [MR.REPORT]".

Attestation is a must for TDX architecture, so The TDCALL[MR.REPORT] is
available in TDX 1.0.  I don't think we should use TDX 1.5 here.  And this
TDCALL is defined in the TDX module spec 1.0.  You can find it in the public TDX
module 1.0 spec (22.3.3. TDG.MR.REPORT Leaf):

https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf

> 
> Steps involved in attestation process can be found in TDX Guest-Host
> Communication Interface (GHCI) for Intel TDX 1.5, section titled
> "TD attestation"

It's strange we need to use GHCI for TDX 1.5 to get some idea about the
attestation process.  Looking at the GHCI 1.0 spec, it seems it already has one
section to talk about attestation process (5.4 TD attestation).

> 
> This API will be mainly used by the attestation driver. Attestation
> driver support will be added by upcoming 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    | 46 ++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/tdx.h |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 03deb4d6920d..3e409b618d3f 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -11,10 +11,12 @@
>  #include <asm/insn.h>
>  #include <asm/insn-eval.h>
>  #include <asm/pgtable.h>
> +#include <asm/io.h>
>  
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
>  #define TDX_GET_VEINFO			3
> +#define TDX_GET_REPORT			4
>  #define TDX_ACCEPT_PAGE			6
>  
>  /* TDX hypercall Leaf IDs */
> @@ -34,6 +36,10 @@
>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>  
> +/* TDX Module call error codes */
> +#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
> +
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>   * return code.
> @@ -98,6 +104,46 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
>  		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
>  }
>  
> +/*
> + * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
> + *
> + * @data        : Address of 1024B aligned data to store
> + *                TDREPORT_STRUCT.
> + * @reportdata  : Address of 64B aligned report data
> + *
> + * return 0 on success or failure error number.
> + */
> +long tdx_mcall_tdreport(void *data, void *reportdata)
> +{
> +	u64 ret;
> +
> +	/*
> +	 * Check for a valid TDX guest to ensure this API is only
> +	 * used by TDX guest platform. Also make sure "data" and
> +	 * "reportdata" pointers are valid.
> +	 */
> +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EINVAL;

Do we need to manually check the alignment since it is mentioned in the comment
of this function?

> +
> +	/*
> +	 * Pass the physical address of user generated reportdata
> +	 * and the physical address of out pointer to store the
> +	 * TDREPORT data to the TDX module to generate the
> +	 * TD report. Generated data contains measurements/configuration
> +	 * data of the TD guest. More info about ABI can be found in TDX
> +	 * Guest-Host-Communication Interface (GHCI), sec titled
> +	 * "TDG.MR.REPORT".

If you agree with my above comments, then this comment should be updated too: 
TDG.MR.REPORT is defined in TDX module 1.0 spec.

> +	 */
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
> +				virt_to_phys(reportdata), 0, 0, NULL);
> +
> +	if (ret)
> +		return TDCALL_RETURN_CODE(ret);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
> +
>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 020c81a7c729..a151f69dd6ef 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -67,6 +67,8 @@ void tdx_safe_halt(void);
>  
>  bool tdx_early_handle_ve(struct pt_regs *regs);
>  
> +long tdx_mcall_tdreport(void *data, void *reportdata);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };

Re: [PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support

[Sathyanarayanan Kuppuswamy]

On 4/18/22 7:29 PM, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is mainly used to verify the trustworthiness
>> of a TD to the 3rd party key servers.
>>
> 
> "key servers" is only a use case of using the attestation service. This sentence
> looks not accurate.

I thought it is mainly used for this use case. If it is not accurate,
how about following?

Attestation is used to verify the trustworthiness of a TD to the other
3rd party entities (like key servers) before exchanging sensitive
information.

> 
>> First step in attestation process
>> is to get the TDREPORT data and the generated data is further used in
>> subsequent steps of the attestation process. TDREPORT data contains
>> details like TDX module version information, measurement of the TD,
>> along with a TD-specified nonce
>>
>> Add a wrapper function (tdx_mcall_tdreport()) to get the TDREPORT from
>> the TDX Module.
>>
>> More details about the TDREPORT TDCALL can be found in TDX Guest-Host
>> Communication Interface (GHCI) for Intel TDX 1.5, section titled
>> "TDCALL [MR.REPORT]".
> 
> Attestation is a must for TDX architecture, so The TDCALL[MR.REPORT] is
> available in TDX 1.0.  I don't think we should use TDX 1.5 here.  And this

Yes. It is also part of v1.0. Since the feature is similar between v1.0
and v1.5, I have included one link. If v1.0 reference is preferred, I
will update it.

> TDCALL is defined in the TDX module spec 1.0.  You can find it in the public TDX
> module 1.0 spec (22.3.3. TDG.MR.REPORT Leaf):

It looks like in the latest update, they have moved this section from
ABI spec. I will update the specification reference.


> 
> https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
> 
>>
>> Steps involved in attestation process can be found in TDX Guest-Host
>> Communication Interface (GHCI) for Intel TDX 1.5, section titled
>> "TD attestation"
> 
> It's strange we need to use GHCI for TDX 1.5 to get some idea about the
> attestation process.  Looking at the GHCI 1.0 spec, it seems it already has one
> section to talk about attestation process (5.4 TD attestation).

Both are same. I will change it to 1.0 reference.

> 
>>
>> This API will be mainly used by the attestation driver. Attestation
>> driver support will be added by upcoming 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    | 46 ++++++++++++++++++++++++++++++++++++++
>>   arch/x86/include/asm/tdx.h |  2 ++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 03deb4d6920d..3e409b618d3f 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -11,10 +11,12 @@
>>   #include <asm/insn.h>
>>   #include <asm/insn-eval.h>
>>   #include <asm/pgtable.h>
>> +#include <asm/io.h>
>>   
>>   /* TDX module Call Leaf IDs */
>>   #define TDX_GET_INFO			1
>>   #define TDX_GET_VEINFO			3
>> +#define TDX_GET_REPORT			4
>>   #define TDX_ACCEPT_PAGE			6
>>   
>>   /* TDX hypercall Leaf IDs */
>> @@ -34,6 +36,10 @@
>>   #define VE_GET_PORT_NUM(e)	((e) >> 16)
>>   #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>>   
>> +/* TDX Module call error codes */
>> +#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
>> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
>> +
>>   /*
>>    * Wrapper for standard use of __tdx_hypercall with no output aside from
>>    * return code.
>> @@ -98,6 +104,46 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
>>   		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
>>   }
>>   
>> +/*
>> + * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
>> + *
>> + * @data        : Address of 1024B aligned data to store
>> + *                TDREPORT_STRUCT.
>> + * @reportdata  : Address of 64B aligned report data
>> + *
>> + * return 0 on success or failure error number.
>> + */
>> +long tdx_mcall_tdreport(void *data, void *reportdata)
>> +{
>> +	u64 ret;
>> +
>> +	/*
>> +	 * Check for a valid TDX guest to ensure this API is only
>> +	 * used by TDX guest platform. Also make sure "data" and
>> +	 * "reportdata" pointers are valid.
>> +	 */
>> +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EINVAL;
> 
> Do we need to manually check the alignment since it is mentioned in the comment
> of this function?

Users are responsible to allocate aligned data. I don't think we need
to add a check for it. If it is unaligned, TDCALL will return error.

> 
>> +
>> +	/*
>> +	 * Pass the physical address of user generated reportdata
>> +	 * and the physical address of out pointer to store the
>> +	 * TDREPORT data to the TDX module to generate the
>> +	 * TD report. Generated data contains measurements/configuration
>> +	 * data of the TD guest. More info about ABI can be found in TDX
>> +	 * Guest-Host-Communication Interface (GHCI), sec titled
>> +	 * "TDG.MR.REPORT".
> 
> If you agree with my above comments, then this comment should be updated too:
> TDG.MR.REPORT is defined in TDX module 1.0 spec.

Ok. will change it.



-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support

[Kai Huang]

On Mon, 2022-04-18 at 20:37 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/18/22 7:29 PM, Kai Huang wrote:
> > On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> > > In TDX guest, attestation is mainly used to verify the trustworthiness
> > > of a TD to the 3rd party key servers.
> > > 
> > 
> > "key servers" is only a use case of using the attestation service. This sentence
> > looks not accurate.
> 
> I thought it is mainly used for this use case. If it is not accurate,
> how about following?
> 
> Attestation is used to verify the trustworthiness of a TD to the other
> 3rd party entities (like key servers) before exchanging sensitive
> information.

Fine to me, although not sure whether you need to mention key servers.  We Intel
guys has some first impression of what does "key servers" mean mainly because we
defined some use cases around here using attestation.  However for other people
"key servers" can be very generic and may not be the case we defined.

> 
> > 
> > > First step in attestation process
> > > is to get the TDREPORT data and the generated data is further used in
> > > subsequent steps of the attestation process. TDREPORT data contains
> > > details like TDX module version information, measurement of the TD,
> > > along with a TD-specified nonce
> > > 
> > > Add a wrapper function (tdx_mcall_tdreport()) to get the TDREPORT from
> > > the TDX Module.
> > > 
> > > More details about the TDREPORT TDCALL can be found in TDX Guest-Host
> > > Communication Interface (GHCI) for Intel TDX 1.5, section titled
> > > "TDCALL [MR.REPORT]".
> > 
> > Attestation is a must for TDX architecture, so The TDCALL[MR.REPORT] is
> > available in TDX 1.0.  I don't think we should use TDX 1.5 here.  And this
> 
> Yes. It is also part of v1.0. Since the feature is similar between v1.0
> and v1.5, I have included one link. If v1.0 reference is preferred, I
> will update it.

I think we should use 1.0.  Attestation is a essential part for TDX, which means
it must be included in TDX1.0, therefore it doesn't make sense to use TDX1.5 to
reference it.

[...]

> > > +/*
> > > + * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
> > > + *
> > > + * @data        : Address of 1024B aligned data to store
> > > + *                TDREPORT_STRUCT.
> > > + * @reportdata  : Address of 64B aligned report data
> > > + *
> > > + * return 0 on success or failure error number.
> > > + */
> > > +long tdx_mcall_tdreport(void *data, void *reportdata)
> > > +{
> > > +	u64 ret;
> > > +
> > > +	/*
> > > +	 * Check for a valid TDX guest to ensure this API is only
> > > +	 * used by TDX guest platform. Also make sure "data" and
> > > +	 * "reportdata" pointers are valid.
> > > +	 */
> > > +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> > > +		return -EINVAL;
> > 
> > Do we need to manually check the alignment since it is mentioned in the comment
> > of this function?
> 
> Users are responsible to allocate aligned data. I don't think we need
> to add a check for it. If it is unaligned, TDCALL will return error.

Actually this is the kernel memory, but not user memory.  Otherwise
virt_to_phys() doesn't make sense.  You copied the user data to kernel memory in
the last patch.  So whether user memory is aligned doesn't matter, and in last
patch, you have guaranteed the alignment is met during kernel memory allocation.

Anyway like you said the TDCALL will fail if alignment doesn't meet, so I guess
is fine.

Re: [PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support

[Sathyanarayanan Kuppuswamy]

On 4/18/22 8:51 PM, Kai Huang wrote:
>> Users are responsible to allocate aligned data. I don't think we need
>> to add a check for it. If it is unaligned, TDCALL will return error.
> Actually this is the kernel memory, but not user memory.  Otherwise
> virt_to_phys() doesn't make sense.  You copied the user data to kernel memory in
> the last patch.  So whether user memory is aligned doesn't matter, and in last
> patch, you have guaranteed the alignment is met during kernel memory allocation.

I mean't user of this API (not user space). But I agree with your
comments.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Kuppuswamy Sathyanarayanan]

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);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index a151f69dd6ef..014cc6192dc5 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -69,6 +69,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
 
 long tdx_mcall_tdreport(void *data, void *reportdata);
 
+long tdx_hcall_get_quote(void *data, u64 len);
+
 #else
 
 static inline void tdx_early_init(void) { };
-- 
2.25.1

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

[Kai Huang]

On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> Attestation is the process used by two un-trusted entities to prove to
> each other that it can be trusted. 
> 

I don't think this is accurate.  TDX attestation is used to attest a TD is
genuine and runs on genuine Intel platforms to any challenger who wants to
verify this.  Theoretically, the TD guest doesn't necessarily need to verify the
trustworthiness of the challenger.

> In TDX guest, attestation is mainly
> used to verify the trustworthiness of a TD to the 3rd party key
> servers.

And "key servers" is only one potential use case of using the attestation
service.  I don't think it's right to say attestation is mainly used for this.

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

This is not accurate.  The VMM cannot generate and sign the Quote.  Only Quoting
enclave (QE) can do that.  The VMM is just a bridge which helps to send the
TDREPORT to the QE and then give the Quote back to TD guest when it receives it.

For instance, theoretically GetQuote TDVMCALL isn't absolutely necessarily for
attestation.  The TD attestation agent (runs in TD guest userspace) can choose
to connect to QE directly if feasible (i.e. via vsock, tcp/ip, ..) and then send
the TDREPORT to QE and receive the Quote directly.

> 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"

See my reply to previous patch.  It's mentioned in GHCI 1.0 spec (section 5.4 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.

It seems GetQuote definitions in public GHCI 1.0 and GHCI 1.5 are different.  In
GHCI 1.5, R13 is used to specify the shared memory size.

I think it is because the public GHCI 1.0 hasn't been updated yet?

> + *
> + * 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 think this function gives people impression that when this function is done,
the Quote is ready immediately in the shared buffer.  But actually GetQuote is
asynchronous.  It only means the VMM has accepted this request, but the Quote is
actually only ready when the guest receives the event notification (done in
later patch).  So I guess there should be a comment somewhere (or even in commit
message) to explain that?  

> +
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> +
>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index a151f69dd6ef..014cc6192dc5 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -69,6 +69,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>  
>  long tdx_mcall_tdreport(void *data, void *reportdata);
>  
> +long tdx_hcall_get_quote(void *data, u64 len);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };

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

[Sathyanarayanan Kuppuswamy]

On 4/18/22 7:59 PM, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
>> Attestation is the process used by two un-trusted entities to prove to
>> each other that it can be trusted.
>>
> 
> I don't think this is accurate.  TDX attestation is used to attest a TD is
> genuine and runs on genuine Intel platforms to any challenger who wants to
> verify this.  Theoretically, the TD guest doesn't necessarily need to verify the
> trustworthiness of the challenger.

Above is a generic explanation for attestation (not TDX specific).

> 
>> In TDX guest, attestation is mainly
>> used to verify the trustworthiness of a TD to the 3rd party key
>> servers.
> 
> And "key servers" is only one potential use case of using the attestation
> service.  I don't think it's right to say attestation is mainly used for this.

Agree. I will change it to,

Attestation is used to verify the trustworthiness of a TD to the other
3rd party entities before exchanging sensitive information.

> 
>>
>> 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.
>>
> 
> This is not accurate.  The VMM cannot generate and sign the Quote.  Only Quoting
> enclave (QE) can do that.  The VMM is just a bridge which helps to send the
> TDREPORT to the QE and then give the Quote back to TD guest when it receives it.
> 
> For instance, theoretically GetQuote TDVMCALL isn't absolutely necessarily for
> attestation.  The TD attestation agent (runs in TD guest userspace) can choose
> to connect to QE directly if feasible (i.e. via vsock, tcp/ip, ..) and then send
> the TDREPORT to QE and receive the Quote directly.

Yes, since guest does not get involved with how VMM facilities the
Quote Generation, I did not elaborate on Quoting Enclave part.

How about following change?

The second stage in the attestation process is for the guest to pass the
TDREPORT data (generated by TDREPORT TDCALL) to the VMM and
request it to trigger Quote generation via a Quoting enclave (QE).

Also note that GetQuote is an asynchronous request. So this API does not
block till Quote is generated. VMM will notify the TD guest about the
quote generation completion via interrupt (configured by
SetupEventNotifyInterrupt hypercall). This support will be added by
follow on patches in this series.


> 
>> 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"
> 
> See my reply to previous patch.  It's mentioned in GHCI 1.0 spec (section 5.4 TD
> attestation).

Yes. I will change it to 1.0 reference.

> 
>>
>> 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.
> 
> It seems GetQuote definitions in public GHCI 1.0 and GHCI 1.5 are different.  In
> GHCI 1.5, R13 is used to specify the shared memory size.
> 
> I think it is because the public GHCI 1.0 hasn't been updated yet?

Please check the latest 1.0 specification (updated on Feb 2022). It has
details about R13 register.

> 
>> + *
>> + * 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 think this function gives people impression that when this function is done,
> the Quote is ready immediately in the shared buffer.  But actually GetQuote is
> asynchronous.  It only means the VMM has accepted this request, but the Quote is
> actually only ready when the guest receives the event notification (done in
> later patch).  So I guess there should be a comment somewhere (or even in commit
> message) to explain that?

I will include details about asynchronous request in commit log as
mentioned above.



-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Kai Huang]

On Mon, 2022-04-18 at 21:04 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/18/22 7:59 PM, Kai Huang wrote:
> > On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Attestation is the process used by two un-trusted entities to prove to
> > > each other that it can be trusted.
> > > 
> > 
> > I don't think this is accurate.  TDX attestation is used to attest a TD is
> > genuine and runs on genuine Intel platforms to any challenger who wants to
> > verify this.  Theoretically, the TD guest doesn't necessarily need to verify the
> > trustworthiness of the challenger.
> 
> Above is a generic explanation for attestation (not TDX specific).

Even for generic, it seems it's not accurate.  As I said it's not "two un-
trusted entities to prove to each other".

> 
> > 
> > > In TDX guest, attestation is mainly
> > > used to verify the trustworthiness of a TD to the 3rd party key
> > > servers.
> > 
> > And "key servers" is only one potential use case of using the attestation
> > service.  I don't think it's right to say attestation is mainly used for this.
> 
> Agree. I will change it to,
> 
> Attestation is used to verify the trustworthiness of a TD to the other
> 3rd party entities before exchanging sensitive information.

Fine to me.

> 
> > 
> > > 
> > > 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.
> > > 
> > 
> > This is not accurate.  The VMM cannot generate and sign the Quote.  Only Quoting
> > enclave (QE) can do that.  The VMM is just a bridge which helps to send the
> > TDREPORT to the QE and then give the Quote back to TD guest when it receives it.
> > 
> > For instance, theoretically GetQuote TDVMCALL isn't absolutely necessarily for
> > attestation.  The TD attestation agent (runs in TD guest userspace) can choose
> > to connect to QE directly if feasible (i.e. via vsock, tcp/ip, ..) and then send
> > the TDREPORT to QE and receive the Quote directly.
> 
> Yes, since guest does not get involved with how VMM facilities the
> Quote Generation, I did not elaborate on Quoting Enclave part.
> 
> How about following change?
> 
> The second stage in the attestation process is for the guest to pass the
> TDREPORT data (generated by TDREPORT TDCALL) to the VMM and
> request it to trigger Quote generation via a Quoting enclave (QE).
> 
> Also note that GetQuote is an asynchronous request. So this API does not
> block till Quote is generated. VMM will notify the TD guest about the
> quote generation completion via interrupt (configured by
> SetupEventNotifyInterrupt hypercall). This support will be added by
> follow on patches in this series.

Fine to me.

Btw, if I recall correctly, it seems we don't need to say "xxx will be done in
later patches", etc.  I can be wrong.  Will leave to others.

[...]


> > > +/*
> > > + * 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.
> > 
> > It seems GetQuote definitions in public GHCI 1.0 and GHCI 1.5 are different.  In
> > GHCI 1.5, R13 is used to specify the shared memory size.
> > 
> > I think it is because the public GHCI 1.0 hasn't been updated yet?
> 
> Please check the latest 1.0 specification (updated on Feb 2022). It has
> details about R13 register.

Thanks.  So it seems GHCI 1.0 has also been updated and it is consistent with
GHCI 1.5 now.  In this case, why do we still assume 8K shared memory?  Are you
going to update the driver?

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

[Sathyanarayanan Kuppuswamy]

On 4/18/22 9:40 PM, Kai Huang wrote:
>> Please check the latest 1.0 specification (updated on Feb 2022). It has
>> details about R13 register.
> Thanks.  So it seems GHCI 1.0 has also been updated and it is consistent with
> GHCI 1.5 now.  In this case, why do we still assume 8K shared memory?  Are you
> going to update the driver?
> 

Since the GetQuote spec only requires memory in 4K alignment, we just
went with 8k constant allocation. Since existing users does not
require more than 8k, it works. But I agree that this needs to be
changed.

In next version, I will change the driver to choose the allocation size
based on user space request. Since this change would require us to do
the memory allocation in IOCTL routine (instead of init code), it will
make it slower. But I think this is negligible compared to time it takes
for Quote request. So it should be fine.

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Kai Huang]

On Mon, 2022-04-18 at 22:28 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/18/22 9:40 PM, Kai Huang wrote:
> > > Please check the latest 1.0 specification (updated on Feb 2022). It has
> > > details about R13 register.
> > Thanks.  So it seems GHCI 1.0 has also been updated and it is consistent with
> > GHCI 1.5 now.  In this case, why do we still assume 8K shared memory?  Are you
> > going to update the driver?
> > 
> 
> Since the GetQuote spec only requires memory in 4K alignment, we just
> went with 8k constant allocation. Since existing users does not
> require more than 8k, it works. But I agree that this needs to be
> changed.

Quote format can be vendor specific, so there's no guarantee 3rd party won't
have a Quote larger than 8k.

> 
> In next version, I will change the driver to choose the allocation size
> based on user space request. Since this change would require us to do
> the memory allocation in IOCTL routine (instead of init code), it will
> make it slower. But I think this is negligible compared to time it takes
> for Quote request. So it should be fine.
> 
> > 

Yes attestation request should never be something that is very frequent, thus
should never be in a performance critical path.

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

[Aubrey Li]

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.

Thanks,
-Aubrey

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

[Sathyanarayanan Kuppuswamy]

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.

> 
> Thanks,
> -Aubrey

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Aubrey Li]

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

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

[Isaku Yamahata]

On Wed, Apr 20, 2022 at 12:16:04AM -0700,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> 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.

This is to conform to the guest-host communicate interface(GHCI) spec.
The input value is defined as "shared GPA as input".  Shared GPA is GPA with
shared bit set.

  table TDG.VP.VMCALL<GetQuote> - Input Operands
  R12 Shared GPA as input – the memory contains a TDREPORT_STRUCT.
      The same buffer is used as output – the memory contains a TD Quote.


Userspace VMM(qemu) can be implemented to accept GPA with shared bit set or not.
It's not a big issue.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

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

[Aubrey Li]

On 2022/4/23 上午1:24, Isaku Yamahata wrote:
> On Wed, Apr 20, 2022 at 12:16:04AM -0700,
> Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> 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.
> 
> This is to conform to the guest-host communicate interface(GHCI) spec.
> The input value is defined as "shared GPA as input".  Shared GPA is GPA with
> shared bit set.
> 
>   table TDG.VP.VMCALL<GetQuote> - Input Operands
>   R12 Shared GPA as input – the memory contains a TDREPORT_STRUCT.
>       The same buffer is used as output – the memory contains a TD Quote.
> 
> 
> Userspace VMM(qemu) can be implemented to accept GPA with shared bit set or not.
> It's not a big issue.
> 

OK, I checked other TDCALL like TDG.VP.VMCALL<MapGPA>, the GPA parameter also has
shared bit set. So the behavior is consistent.

Thanks for the explanation.
-Aubrey

[PATCH v3 3/4] x86/tdx: Add TDX Guest event notify interrupt support

[Kuppuswamy Sathyanarayanan]

Host-guest event notification via configured interrupt vector is
useful in cases where guest makes asynchronous request (like
attestation quote generate request) and need a callback from host to
indicate the completion or to let host notify the guest about events
like device removal

In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
guest to specify which interrupt vector to use as an event-notify
vector to the VMM. Details about the SetupEventNotifyInterrupt
hypercall can be found in TDX Guest-Host Communication Interface
(GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
Add a tdx_hcall_set_notify_intr() helper function to implement the
SetupEventNotifyInterrupt hypercall.

Reserve 0xec IRQ vector address for TDX guest to receive the event
completion notification from VMM. Also add related IDT handler to
process the notification event.

Add support to track the notification event status via
/proc/interrupts.

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            | 77 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/hardirq.h     |  3 ++
 arch/x86/include/asm/idtentry.h    |  4 ++
 arch/x86/include/asm/irq_vectors.h |  7 ++-
 arch/x86/include/asm/tdx.h         |  4 ++
 arch/x86/kernel/irq.c              |  7 +++
 6 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index c259d81a5d7f..3b6fdfb31e87 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -12,6 +12,10 @@
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
 #include <asm/io.h>
+#include <asm/apic.h>
+#include <asm/idtentry.h>
+#include <asm/irq_regs.h>
+#include <asm/desc.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
@@ -22,6 +26,7 @@
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
 #define TDVMCALL_GET_QUOTE		0x10002
+#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* MMIO direction */
 #define EPT_READ	0
@@ -41,6 +46,28 @@
 #define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
 #define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
 
+/*
+ * Handler used to report notifications about
+ * TDX_GUEST_EVENT_NOTIFY_VECTOR IRQ. Currently it will be
+ * used only by the attestation driver. So, race condition
+ * with read/write operation is not considered.
+ */
+static void (*tdx_event_notify_handler)(void);
+
+/* Helper function to register tdx_event_notify_handler */
+void tdx_setup_ev_notify_handler(void (*handler)(void))
+{
+        tdx_event_notify_handler = handler;
+}
+EXPORT_SYMBOL_GPL(tdx_setup_ev_notify_handler);
+
+/* Helper function to unregister tdx_event_notify_handler */
+void tdx_remove_ev_notify_handler(void)
+{
+        tdx_event_notify_handler = NULL;
+}
+EXPORT_SYMBOL_GPL(tdx_remove_ev_notify_handler);
+
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
  * return code.
@@ -105,6 +132,21 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
 }
 
+/* TDX guest event notification handler */
+DEFINE_IDTENTRY_SYSVEC(sysvec_tdx_event_notify)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	inc_irq_stat(irq_tdx_event_notify_count);
+
+	if (tdx_event_notify_handler)
+		tdx_event_notify_handler();
+
+	ack_APIC_irq();
+
+	set_irq_regs(old_regs);
+}
+
 /*
  * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
  *
@@ -182,6 +224,35 @@ long tdx_hcall_get_quote(void *data, u64 len)
 }
 EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
 
+/*
+ * tdx_hcall_set_notify_intr() - Setup Event Notify Interrupt Vector.
+ *
+ * @vector        : Vector address to be used for notification.
+ *
+ * return 0 on success or failure error number.
+ */
+static long tdx_hcall_set_notify_intr(u8 vector)
+{
+	u64 ret;
+
+	/* Minimum vector value allowed is 32 */
+	if (vector < 32)
+		return -EINVAL;
+
+	/*
+	 * Register callback vector address with VMM. More details
+	 * about the ABI can be found in TDX Guest-Host-Communication
+	 * Interface (GHCI), sec titled
+	 * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
+	 */
+	ret = _tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
@@ -772,5 +843,11 @@ void __init tdx_early_init(void)
 	x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;
 	x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
 
+	alloc_intr_gate(TDX_GUEST_EVENT_NOTIFY_VECTOR,
+			asm_sysvec_tdx_event_notify);
+
+	if (tdx_hcall_set_notify_intr(TDX_GUEST_EVENT_NOTIFY_VECTOR))
+		pr_warn("Setting event notification interrupt failed\n");
+
 	pr_info("Guest detected\n");
 }
diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index 275e7fd20310..582deff56210 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -44,6 +44,9 @@ typedef struct {
 	unsigned int irq_hv_reenlightenment_count;
 	unsigned int hyperv_stimer0_count;
 #endif
+#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
+	unsigned int irq_tdx_event_notify_count;
+#endif
 } ____cacheline_aligned irq_cpustat_t;
 
 DECLARE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 72184b0b2219..331d343e1d46 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -700,6 +700,10 @@ DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR,	sysvec_xen_hvm_callback);
 DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR,	sysvec_kvm_asyncpf_interrupt);
 #endif
 
+#ifdef CONFIG_INTEL_TDX_GUEST
+DECLARE_IDTENTRY_SYSVEC(TDX_GUEST_EVENT_NOTIFY_VECTOR,	sysvec_tdx_event_notify);
+#endif
+
 #undef X86_TRAP_OTHER
 
 #endif
diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
index 43dcb9284208..82ac0c0a34b1 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -104,7 +104,12 @@
 #define HYPERV_STIMER0_VECTOR		0xed
 #endif
 
-#define LOCAL_TIMER_VECTOR		0xec
+#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
+/* Vector on which TDX Guest event notification is delivered */
+#define TDX_GUEST_EVENT_NOTIFY_VECTOR	0xec
+#endif
+
+#define LOCAL_TIMER_VECTOR		0xeb
 
 #define NR_VECTORS			 256
 
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 014cc6192dc5..7b8479f46757 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -71,6 +71,10 @@ long tdx_mcall_tdreport(void *data, void *reportdata);
 
 long tdx_hcall_get_quote(void *data, u64 len);
 
+void tdx_setup_ev_notify_handler(void (*handler)(void));
+
+void tdx_remove_ev_notify_handler(void);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 766ffe3ba313..a96ecd866723 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -181,6 +181,13 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 		seq_printf(p, "%10u ",
 			   irq_stats(j)->kvm_posted_intr_wakeup_ipis);
 	seq_puts(p, "  Posted-interrupt wakeup event\n");
+#endif
+#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
+	seq_printf(p, "%*s: ", prec, "TGN");
+	for_each_online_cpu(j)
+		seq_printf(p, "%10u ",
+			   irq_stats(j)->irq_tdx_event_notify_count);
+	seq_puts(p, "  TDX Guest event notification\n");
 #endif
 	return 0;
 }
-- 
2.25.1

[PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy Sathyanarayanan]

TDX guest supports encrypted disk as root or secondary drives.
Decryption keys required to access such drives are usually maintained
by 3rd party key servers. Attestation is required by 3rd party key
servers to get the key for an encrypted disk volume, or possibly other
encrypted services. Attestation is used to prove to the key server that
the TD guest is running in a valid TD and the kernel and virtual BIOS
and other environment are secure.

During the boot process various components before the kernel accumulate
hashes in the TDX module, which can then combined into a report. This
would typically include a hash of the bios, bios configuration, boot
loader, command line, kernel, initrd.  After checking the hashes the
key server will securely release the keys.

The actual details of the attestation protocol depend on the particular
key server configuration, but some parts are common and need to
communicate with the TDX module.

This communication is implemented in the attestation driver.

The supported steps are:

  1. TD guest generates the TDREPORT that contains version information
     about the Intel TDX module, measurement of the TD, along with a
     TD-specified nonce.
  2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
     which is used by the host to generate a quote via quoting
     enclave (QE).
  3. Quote generation completion notification is sent to TD OS via
     callback interrupt vector configured by TD using
     SetupEventNotifyInterrupt hypercall.
  4. After receiving the generated TDQUOTE, a remote verifier can be
     used to verify the quote and confirm the trustworthiness of the
     TD.

Attestation agent uses IOCTLs implemented by the attestation driver to
complete the various steps of the attestation process.

Also note that, explicit access permissions are not enforced in this
driver because the quote and measurements are not a secret. However
the access permissions of the device node can be used to set any
desired access policy. The udev default is usually root access
only.

TDX_CMD_GEN_QUOTE IOCTL can be used to create an computation on the
host, but TDX assumes that the host is able to deal with malicious
guest flooding it anyways.

The interaction with the TDX module is like a RPM protocol here. There
are several operations (get tdreport, get quote) that need to input a
blob, and then output another blob. It was considered to use a sysfs
interface for this, but it doesn't fit well into the standard sysfs
model for configuring values. It would be possible to do read/write on
files, but it would need multiple file descriptors, which would be
somewhat messy. ioctls seems to be the best fitting and simplest model
here. There is one ioctl per operation, that takes the input blob and
returns the output blob, and as well as auxiliary ioctls to return the
blob lengths. The ioctls are documented in the header file. 

[Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer]
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>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v2:
 * Combined attestation related global variables into
   struct attest_dev to make the code clean.
 * Added support to pass TDREPORT and GetQuote TDCALLs
   error code back to user on failure.
 * Modified the driver to use platform device driver
   model and added check to ensure only one device is
   allowed.

 drivers/platform/x86/intel/Kconfig            |   2 +-
 drivers/platform/x86/intel/Makefile           |   1 +
 drivers/platform/x86/intel/tdx/Kconfig        |  13 +
 drivers/platform/x86/intel/tdx/Makefile       |   3 +
 .../platform/x86/intel/tdx/intel_tdx_attest.c | 302 ++++++++++++++++++
 include/uapi/misc/tdx.h                       |  42 +++
 6 files changed, 362 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
 create mode 100644 drivers/platform/x86/intel/tdx/Makefile
 create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
 create mode 100644 include/uapi/misc/tdx.h

diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 1f01a8a23c57..a2e2a5a29bde 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -12,7 +12,7 @@ source "drivers/platform/x86/intel/speed_select_if/Kconfig"
 source "drivers/platform/x86/intel/telemetry/Kconfig"
 source "drivers/platform/x86/intel/wmi/Kconfig"
 source "drivers/platform/x86/intel/uncore-frequency/Kconfig"
-
+source "drivers/platform/x86/intel/tdx/Kconfig"
 
 config INTEL_HID_EVENT
 	tristate "Intel HID Event"
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index c61bc3e97121..6b7c94051519 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472)		+= int3472/
 obj-$(CONFIG_INTEL_PMC_CORE)		+= pmc/
 obj-$(CONFIG_INTEL_PMT_CLASS)		+= pmt/
 obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/
+obj-$(CONFIG_INTEL_TDX_GUEST)		+= tdx/
 obj-$(CONFIG_INTEL_TELEMETRY)		+= telemetry/
 obj-$(CONFIG_INTEL_WMI)			+= wmi/
 obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL)	+= uncore-frequency/
diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig
new file mode 100644
index 000000000000..332a10313b49
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# X86 TDX Platform Specific Drivers
+#
+
+config INTEL_TDX_ATTESTATION
+	tristate "Intel TDX attestation driver"
+	depends on INTEL_TDX_GUEST
+	help
+	  The TDX attestation driver provides IOCTL interfaces to the user to
+	  request TDREPORT from the TDX module or request quote from the VMM
+	  or to get quote buffer size. It is mainly used to get secure disk
+	  decryption keys from the key server.
diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
new file mode 100644
index 000000000000..94eea6108fbd
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
new file mode 100644
index 000000000000..9124db800d4f
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel_tdx_attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process and
+ * read the TD Quote result.
+ *
+ * Copyright (C) 2021-2022 Intel Corporation
+ *
+ * Author:
+ *     Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
+ */
+
+#define pr_fmt(fmt) "x86/tdx: attest: " fmt
+
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/set_memory.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/jiffies.h>
+#include <linux/io.h>
+#include <asm/apic.h>
+#include <asm/tdx.h>
+#include <asm/irq_vectors.h>
+#include <uapi/misc/tdx.h>
+
+#define DRIVER_NAME "tdx-attest"
+
+/* Used in Quote memory allocation */
+#define QUOTE_SIZE			(2 * PAGE_SIZE)
+/* Used in Get Quote request memory allocation */
+#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
+/* Get Quote timeout in msec */
+#define GET_QUOTE_TIMEOUT		(5000)
+
+struct attest_dev {
+	/* Mutex to serialize attestation requests */
+	struct mutex lock;
+	/* Completion object to track GetQuote completion status */
+	struct completion req_compl;
+	/* Buffer used to copy report data in attestation handler */
+	u8 report_buf[TDX_REPORT_DATA_LEN] __aligned(64);
+	/* Data pointer used to get TD Quote data in attestation handler */
+	void *tdquote_buf;
+	/* Data pointer used to get TDREPORT data in attestation handler */
+	void *tdreport_buf;
+	/* DMA handle used to allocate and free tdquote DMA buffer */
+	dma_addr_t handle;
+	struct miscdevice miscdev;
+};
+
+static struct platform_device *pdev;
+
+static void attestation_callback_handler(void)
+{
+	struct attest_dev *adev = platform_get_drvdata(pdev);
+
+	complete(&adev->req_compl);
+}
+
+static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
+			     unsigned long arg)
+{
+	struct attest_dev *adev = platform_get_drvdata(pdev);
+	void __user *argp = (void __user *)arg;
+	struct tdx_gen_quote tdquote_req;
+	long ret = 0, err;
+
+	mutex_lock(&adev->lock);
+
+	switch (cmd) {
+	case TDX_CMD_GET_TDREPORT:
+		if (copy_from_user(adev->report_buf, argp,
+					TDX_REPORT_DATA_LEN)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		/* Generate TDREPORT_STRUCT */
+		err = tdx_mcall_tdreport(adev->tdreport_buf, adev->report_buf);
+		if (err) {
+			ret = put_user(err, (long __user *)argp);
+			ret = -EIO;
+			break;
+		}
+
+		if (copy_to_user(argp, adev->tdreport_buf, TDX_TDREPORT_LEN))
+			ret = -EFAULT;
+		break;
+	case TDX_CMD_GEN_QUOTE:
+		reinit_completion(&adev->req_compl);
+
+		/* Copy TDREPORT data from user buffer */
+		if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (copy_from_user(adev->tdquote_buf, (void __user *)tdquote_req.buf,
+					tdquote_req.len)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		/* Submit GetQuote Request */
+		err = tdx_hcall_get_quote(adev->tdquote_buf, GET_QUOTE_MAX_SIZE);
+		if (err) {
+			ret = put_user(err, (long __user *)argp);
+			ret = -EIO;
+			break;
+		}
+
+		/* Wait for attestation completion */
+		ret = wait_for_completion_interruptible_timeout(
+				&adev->req_compl,
+				msecs_to_jiffies(GET_QUOTE_TIMEOUT));
+		if (ret <= 0) {
+			ret = -EIO;
+			break;
+		}
+
+		/* ret will be positive if completed. */
+		ret = 0;
+
+		if (copy_to_user((void __user *)tdquote_req.buf, adev->tdquote_buf,
+					tdquote_req.len))
+			ret = -EFAULT;
+
+		break;
+	case TDX_CMD_GET_QUOTE_SIZE:
+		ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
+		break;
+	default:
+		pr_err("cmd %d not supported\n", cmd);
+		break;
+	}
+
+	mutex_unlock(&adev->lock);
+
+	return ret;
+}
+
+static const struct file_operations tdx_attest_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= tdx_attest_ioctl,
+	.llseek		= no_llseek,
+};
+
+/* Helper function to cleanup attestation related allocations */
+static void _tdx_attest_remove(struct attest_dev *adev)
+{
+	misc_deregister(&adev->miscdev);
+
+	tdx_remove_ev_notify_handler();
+
+	if (adev->tdquote_buf)
+		dma_free_coherent(&pdev->dev, GET_QUOTE_MAX_SIZE,
+				adev->tdquote_buf, adev->handle);
+
+	if (adev->tdreport_buf)
+		free_pages((unsigned long)adev->tdreport_buf, 0);
+
+	kfree(adev);
+}
+
+static int tdx_attest_probe(struct platform_device *attest_pdev)
+{
+	struct device *dev = &attest_pdev->dev;
+	struct attest_dev *adev;
+	long ret = 0;
+
+	/* Only single device is allowed */
+	if (pdev)
+		return -EBUSY;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	mutex_init(&adev->lock);
+	init_completion(&adev->req_compl);
+	pdev = attest_pdev;
+	platform_set_drvdata(pdev, adev);
+
+	/*
+	 * tdreport_data needs to be 64-byte aligned.
+	 * Full page alignment is more than enough.
+	 */
+	adev->tdreport_buf = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+						      0);
+	if (!adev->tdreport_buf) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+	if (ret) {
+		pr_err("dma set coherent mask failed\n");
+		goto failed;
+	}
+
+	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
+	adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE,
+						&adev->handle,
+						GFP_KERNEL | __GFP_ZERO);
+	if (!adev->tdquote_buf) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	/* Register attestation event notify handler */
+	tdx_setup_ev_notify_handler(attestation_callback_handler);
+
+	adev->miscdev.name = DRIVER_NAME;
+	adev->miscdev.minor = MISC_DYNAMIC_MINOR;
+	adev->miscdev.fops = &tdx_attest_fops;
+	adev->miscdev.parent = dev;
+
+	ret = misc_register(&adev->miscdev);
+	if (ret) {
+		pr_err("misc device registration failed\n");
+		goto failed;
+	}
+
+	pr_debug("module initialization success\n");
+
+	return 0;
+
+failed:
+	_tdx_attest_remove(adev);
+
+	pr_debug("module initialization failed\n");
+
+	return ret;
+}
+
+static int tdx_attest_remove(struct platform_device *attest_pdev)
+{
+	struct attest_dev *adev = platform_get_drvdata(attest_pdev);
+
+	mutex_lock(&adev->lock);
+	_tdx_attest_remove(adev);
+	mutex_unlock(&adev->lock);
+	pr_debug("module is successfully removed\n");
+	return 0;
+}
+
+static struct platform_driver tdx_attest_driver = {
+	.probe		= tdx_attest_probe,
+	.remove		= tdx_attest_remove,
+	.driver		= {
+		.name	= DRIVER_NAME,
+	},
+};
+
+static int __init tdx_attest_init(void)
+{
+	int ret;
+
+	/* Make sure we are in a valid TDX platform */
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EIO;
+
+	ret = platform_driver_register(&tdx_attest_driver);
+	if (ret) {
+		pr_err("failed to register driver, err=%d\n", ret);
+		return ret;
+	}
+
+	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
+		pr_err("failed to allocate device, err=%d\n", ret);
+		platform_driver_unregister(&tdx_attest_driver);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit tdx_attest_exit(void)
+{
+	platform_device_unregister(pdev);
+	platform_driver_unregister(&tdx_attest_driver);
+}
+
+module_init(tdx_attest_init);
+module_exit(tdx_attest_exit);
+
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
+MODULE_DESCRIPTION("TDX attestation driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
new file mode 100644
index 000000000000..9920f36c79fe
--- /dev/null
+++ b/include/uapi/misc/tdx.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MISC_TDX_H
+#define _UAPI_MISC_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
+#define TDX_REPORT_DATA_LEN		64
+
+/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
+#define TDX_TDREPORT_LEN		1024
+
+/*
+ * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
+ * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
+ * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
+ * TDREPORT data is copied to the user buffer.
+ */
+#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
+
+/*
+ * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
+ * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
+ * buffer of quote size. Once IOCTL is successful quote data is copied back to
+ * the user buffer.
+ */
+#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
+
+/*
+ * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
+ * This will be used for determining the input buffer allocation size when
+ * using TDX_CMD_GEN_QUOTE IOCTL.
+ */
+#define TDX_CMD_GET_QUOTE_SIZE		_IOR('T', 0x03, __u64)
+
+struct tdx_gen_quote {
+	__u64 buf;
+	__u64 len;
+};
+
+#endif /* _UAPI_MISC_TDX_H */
-- 
2.25.1

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# X86 TDX Platform Specific Drivers
> +#
> +
> +config INTEL_TDX_ATTESTATION
> +	tristate "Intel TDX attestation driver"
> +	depends on INTEL_TDX_GUEST
> +	help
> +	  The TDX attestation driver provides IOCTL interfaces to the user to
> +	  request TDREPORT from the TDX module or request quote from the VMM
> +	  or to get quote buffer size. It is mainly used to get secure disk
> +	  decryption keys from the key server.
> diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> new file mode 100644
> index 000000000000..94eea6108fbd
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..9124db800d4f
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c


From security's perspective, attestation is an essential part of TDX.  That
being said, w/o attestation support in TD guest, I guess nobody will seriously
use TD guest.

From this perspective, I am not sure what's the value of having a dedicated
INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
under arch/x86/coco/tdx/ I guess?

But I'll leave this to maintainers.

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Tue, Apr 19, 2022 at 07:47:33PM +1200, Kai Huang wrote:
> From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
> under arch/x86/coco/tdx/ I guess?
> 
> But I'll leave this to maintainers.

Similar story with the unaccepted memory gunk. If it is not going to
be used outside of encrypted guests, why are we polluting our already
insanely humongous Kconfig space with more symbols?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/19/22 1:13 AM, Borislav Petkov wrote:
> On Tue, Apr 19, 2022 at 07:47:33PM +1200, Kai Huang wrote:
>>  From this perspective, I am not sure what's the value of having a dedicated
>> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
>> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
>> under arch/x86/coco/tdx/ I guess?
>>
>> But I'll leave this to maintainers.
> 
> Similar story with the unaccepted memory gunk. If it is not going to
> be used outside of encrypted guests, why are we polluting our already
> insanely humongous Kconfig space with more symbols?
> 

Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION.

Boris, this is a simple platform driver which adds IOCTL interfaces to 
allow user space to get TDREPORT and TDQuote support.

So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Tue, Apr 19, 2022 at 05:48:57AM -0700, Sathyanarayanan Kuppuswamy wrote:
> Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION.

Sounds like you didn't read my mail. Kai was questioning the need for a
Kconfig symbol at all. And me too.

If this thing is not going to be used by anything else besides TDX
guests, why does it need a Kconfig option at all?!

> Boris, this is a simple platform driver which adds IOCTL interfaces to allow
> user space to get TDREPORT and TDQuote support.
> 
> So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?

See above.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Boris,

On 4/20/22 3:00 PM, Borislav Petkov wrote:
> On Tue, Apr 19, 2022 at 05:48:57AM -0700, Sathyanarayanan Kuppuswamy wrote:
>> Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION.
> 
> Sounds like you didn't read my mail. Kai was questioning the need for a
> Kconfig symbol at all. And me too.
> 
> If this thing is not going to be used by anything else besides TDX
> guests, why does it need a Kconfig option at all?!

Sorry, it is a typo. I meant we can just compile it with
TDX guest config (CONFIG_INTEL_TDX_GUEST).

> 
>> Boris, this is a simple platform driver which adds IOCTL interfaces to allow
>> user space to get TDREPORT and TDQuote support.
>>
>> So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?
> 
> See above.

I agree with dropping the new CONFIG option. But regarding driver
location, which is preferred (platform/x86/intel/tdx or arch/x86/coco/tdx/)?

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Wed, Apr 20, 2022 at 03:09:44PM -0700, Sathyanarayanan Kuppuswamy wrote:
> I agree with dropping the new CONFIG option. But regarding driver
> location, which is preferred (platform/x86/intel/tdx or arch/x86/coco/tdx/)?

Well, since this is not really a driver but the attestation part of TDX,
then arch/x86/coco/tdx/ sounds like the place.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/21/22 2:10 AM, Borislav Petkov wrote:
> Well, since this is not really a driver but the attestation part of TDX,
> then arch/x86/coco/tdx/ sounds like the place.

Ok. I will move it to arch/x86/coco/tdx/attest.c

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-04-19 at 19:47 +1200, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# X86 TDX Platform Specific Drivers
> > +#
> > +
> > +config INTEL_TDX_ATTESTATION
> > +	tristate "Intel TDX attestation driver"
> > +	depends on INTEL_TDX_GUEST
> > +	help
> > +	  The TDX attestation driver provides IOCTL interfaces to the user to
> > +	  request TDREPORT from the TDX module or request quote from the VMM
> > +	  or to get quote buffer size. It is mainly used to get secure disk
> > +	  decryption keys from the key server.
> > diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> > new file mode 100644
> > index 000000000000..94eea6108fbd
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
> > diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> > new file mode 100644
> > index 000000000000..9124db800d4f
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> 
> 
> From security's perspective, attestation is an essential part of TDX.  That
> being said, w/o attestation support in TD guest, I guess nobody will seriously
> use TD guest.
> 
> From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
> under arch/x86/coco/tdx/ I guess?
> 
> But I'll leave this to maintainers.

In fact after slightly thinking more, I think you can split TDREPORT TDCALL
support with GetQuote/SetupEventNotifyInterrupt support.  The reason is as I
said, GetQuote isn't mandatory to support attestation.  TD attestation agent can
use i.e. vsock, tcp/ip, to communicate to QE directly.  Whether kernel needs to
support GetQuote is actually arguable.

So IMHO you can split this attestation driver into two parts:

1) A "basic" driver which supports reporting TDREPORT to userspace
2) Additional support of GetQuote/SetupEventNotifyInterrupt.

The 1) can even be in a single patch (I guess it won't be complicated).  It is
easy to review (and i.e. can be merged separately), and with it, you will
immediately have one way to support attestation.

2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE).  I think this part has most of the
complexity things in terms of review.

-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/19/22 1:16 AM, Kai Huang wrote:
> In fact after slightly thinking more, I think you can split TDREPORT TDCALL
> support with GetQuote/SetupEventNotifyInterrupt support.  The reason is as I
> said, GetQuote isn't mandatory to support attestation.  TD attestation agent can
> use i.e. vsock, tcp/ip, to communicate to QE directly.  Whether kernel needs to
> support GetQuote is actually arguable.

IMO, we should not use a usage model to categorize "GetQuote" support
as a mandatory or non-mandatory requirement.

For customers who use VSOCK, they can get away without GetQuote
TDVMCALL support. But for customers who do not want to use
VSOCK model, this is a required support. AFAIK, our current customer
requirement is to use TDVMCALL approach for attestation support.

If your suggestion is to split GetQuote support as separate
patch to make it easier for review, I am fine with such
suggestion.

Maintainers, any opinion? Would you prefer to split the
driver into two patches?


> 
> So IMHO you can split this attestation driver into two parts:
> 
> 1) A "basic" driver which supports reporting TDREPORT to userspace
> 2) Additional support of GetQuote/SetupEventNotifyInterrupt.
> 
> The 1) can even be in a single patch (I guess it won't be complicated).  It is
> easy to review (and i.e. can be merged separately), and with it, you will
> immediately have one way to support attestation.
> 
> 2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
> CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE).  I think this part has most of the


GetQuote IOCTL support is a very simple feature support, so, IMO, we
don't need to complicate it with additional config.

> complexity things in terms of review.


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-04-19 at 07:00 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/19/22 1:16 AM, Kai Huang wrote:
> > In fact after slightly thinking more, I think you can split TDREPORT TDCALL
> > support with GetQuote/SetupEventNotifyInterrupt support.  The reason is as I
> > said, GetQuote isn't mandatory to support attestation.  TD attestation agent can
> > use i.e. vsock, tcp/ip, to communicate to QE directly.  Whether kernel needs to
> > support GetQuote is actually arguable.
> 
> IMO, we should not use a usage model to categorize "GetQuote" support
> as a mandatory or non-mandatory requirement.
> 
> For customers who use VSOCK, they can get away without GetQuote
> TDVMCALL support. But for customers who do not want to use
> VSOCK model, this is a required support. AFAIK, our current customer
> requirement is to use TDVMCALL approach for attestation support.
> 
> If your suggestion is to split GetQuote support as separate
> patch to make it easier for review, I am fine with such
> suggestion.
> 

I am not saying we should get rid of GetQuote support.  If there's customer
wants this with a good reason, we can certainly support it.  I understand that
some customer wants to deploy QE in host and don't want additional communication
channel (i.e. vsock) between guest and host, which may add additional attack
window and/or customer's validation resource.

My point is regardless whether we need to support GetQuote, logically this
driver can be split to two parts as I said: 1) basic TDREPORT support to
userspace; 2) additional GetQuote support.  And I think there are many benefits
if you do in this way as I commented below.


> > 
> > So IMHO you can split this attestation driver into two parts:
> > 
> > 1) A "basic" driver which supports reporting TDREPORT to userspace
> > 2) Additional support of GetQuote/SetupEventNotifyInterrupt.
> > 
> > The 1) can even be in a single patch (I guess it won't be complicated).  It is
> > easy to review (and i.e. can be merged separately), and with it, you will
> > immediately have one way to support attestation.
> > 
> > 2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
> > CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE).  I think this part has most of the
> 
> 
> GetQuote IOCTL support is a very simple feature support, so, IMO, we
> don't need to complicate it with additional config.
> 
> > 

Additional Kconfig can reduce attack window by turning it off for people don't
need it.  Anyway no strong opinion here.

-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dave Hansen]

On 4/19/22 00:47, Kai Huang wrote:
>>From security's perspective, attestation is an essential part of TDX.  That
> being said, w/o attestation support in TD guest, I guess nobody will seriously
> use TD guest.

Are you saying you can't think of a single threat model where there's a
benefit to running a TDX guest without attestation?  Will TDX only be
used in environments where secrets are provisioned to guests on the
basis of attestation?

>>From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
> under arch/x86/coco/tdx/ I guess?

How much code are we talking about?  What's the difference in the size
of the binaries with this compiled in?

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/19/22 7:13 AM, Dave Hansen wrote:
>> >From this perspective, I am not sure what's the value of having a dedicated
>> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
>> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
>> under arch/x86/coco/tdx/ I guess?
> How much code are we talking about?  What's the difference in the size
> of the binaries with this compiled in?

Current driver size is ~300 lines. It adds ~500 bytes to the kernel
binary if it is built-in.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dave Hansen]

On 4/19/22 07:19, Sathyanarayanan Kuppuswamy wrote:
> On 4/19/22 7:13 AM, Dave Hansen wrote:
>>> >From this perspective, I am not sure what's the value of having a
>>> dedicated
>>> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should
>>> be turned on
>>> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also
>>> be just
>>> under arch/x86/coco/tdx/ I guess?
>> How much code are we talking about?  What's the difference in the size
>> of the binaries with this compiled in?
> 
> Current driver size is ~300 lines. It adds ~500 bytes to the kernel
> binary if it is built-in.

That doesn't sound like good use of a Kconfig option to me.  Just
explain in the cover letter:

	Any distribution enabling TDX is also expected to need
	attestation.  The compiled size is quite small (500 bytes).

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/19/22 7:24 AM, Dave Hansen wrote:
>> Current driver size is ~300 lines. It adds ~500 bytes to the kernel
>> binary if it is built-in.
> That doesn't sound like good use of a Kconfig option to me.  Just
> explain in the cover letter:
> 
> 	Any distribution enabling TDX is also expected to need
> 	attestation.  The compiled size is quite small (500 bytes).

Ok. I will add it.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote:
> On 4/19/22 00:47, Kai Huang wrote:
> > > From security's perspective, attestation is an essential part of TDX.  That
> > being said, w/o attestation support in TD guest, I guess nobody will seriously
> > use TD guest.
> 
> Are you saying you can't think of a single threat model where there's a
> benefit to running a TDX guest without attestation?  Will TDX only be
> used in environments where secrets are provisioned to guests on the
> basis of attestation?
> 
> > 

I don't think anyone should provision secret to a TD before it get attested that
it is a genuine TD that he/she expected.  If someone does that, he/she takes the
risk of losing the secret.  Of course if someone just want to try a TD then w/o
attestation is totally fine.

-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dave Hansen]

On 4/19/22 15:21, Kai Huang wrote:
> On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote:
>> On 4/19/22 00:47, Kai Huang wrote:
>>>> From security's perspective, attestation is an essential part of TDX.  That
>>> being said, w/o attestation support in TD guest, I guess nobody will seriously
>>> use TD guest.
>> Are you saying you can't think of a single threat model where there's a
>> benefit to running a TDX guest without attestation?  Will TDX only be
>> used in environments where secrets are provisioned to guests on the
>> basis of attestation?
>>
> I don't think anyone should provision secret to a TD before it get attested that
> it is a genuine TD that he/she expected.  If someone does that, he/she takes the
> risk of losing the secret.  Of course if someone just want to try a TD then w/o
> attestation is totally fine.

Yeah, but you said:

	w/o attestation support in TD guest, I guess nobody will
	seriously use TD guest.

I'm trying to get to the bottom of that.  That's a much more broad
statement than something about when it's safe to deploy secrets.

There are lots of secrets deployed in (serious) VMs today.  There are
lots of secrets deployed in (serious) SEV VMs that don't have
attestation.  Yet, the world somehow hasn't come crashing down.

I think it's crazy to say that nobody will deploy secrets to TDX VMs
without attestation.  I think it's a step father into crazy land to say
that no one will "seriously" use TDX guests without attestation.

Let's be honest about this and not live in some fantasy world, please.

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-04-19 at 15:49 -0700, Dave Hansen wrote:
> On 4/19/22 15:21, Kai Huang wrote:
> > On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote:
> > > On 4/19/22 00:47, Kai Huang wrote:
> > > > > From security's perspective, attestation is an essential part of TDX.  That
> > > > being said, w/o attestation support in TD guest, I guess nobody will seriously
> > > > use TD guest.
> > > Are you saying you can't think of a single threat model where there's a
> > > benefit to running a TDX guest without attestation?  Will TDX only be
> > > used in environments where secrets are provisioned to guests on the
> > > basis of attestation?
> > > 
> > I don't think anyone should provision secret to a TD before it get attested that
> > it is a genuine TD that he/she expected.  If someone does that, he/she takes the
> > risk of losing the secret.  Of course if someone just want to try a TD then w/o
> > attestation is totally fine.
> 
> Yeah, but you said:
> 
> 	w/o attestation support in TD guest, I guess nobody will
> 	seriously use TD guest.
> 
> I'm trying to get to the bottom of that.  That's a much more broad
> statement than something about when it's safe to deploy secrets.
> 
> There are lots of secrets deployed in (serious) VMs today.  There are
> lots of secrets deployed in (serious) SEV VMs that don't have
> attestation.  Yet, the world somehow hasn't come crashing down.
> 
> I think it's crazy to say that nobody will deploy secrets to TDX VMs
> without attestation.  I think it's a step father into crazy land to say
> that no one will "seriously" use TDX guests without attestation.
> 
> Let's be honest about this and not live in some fantasy world, please.

OK agree.  No argument about this.


-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Isaku Yamahata]

On Fri, Apr 15, 2022 at 03:01:09PM -0700,
Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
...
> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..9124db800d4f
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
...
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	struct attest_dev *adev = platform_get_drvdata(pdev);
> +	void __user *argp = (void __user *)arg;
> +	struct tdx_gen_quote tdquote_req;
> +	long ret = 0, err;
> +
> +	mutex_lock(&adev->lock);
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		if (copy_from_user(adev->report_buf, argp,
> +					TDX_REPORT_DATA_LEN)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Generate TDREPORT_STRUCT */
> +		err = tdx_mcall_tdreport(adev->tdreport_buf, adev->report_buf);
> +		if (err) {
> +			ret = put_user(err, (long __user *)argp);
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		if (copy_to_user(argp, adev->tdreport_buf, TDX_TDREPORT_LEN))
> +			ret = -EFAULT;
> +		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		reinit_completion(&adev->req_compl);
> +
> +		/* Copy TDREPORT data from user buffer */
> +		if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (copy_from_user(adev->tdquote_buf, (void __user *)tdquote_req.buf,
> +					tdquote_req.len)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Submit GetQuote Request */
> +		err = tdx_hcall_get_quote(adev->tdquote_buf, GET_QUOTE_MAX_SIZE);
> +		if (err) {
> +			ret = put_user(err, (long __user *)argp);
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		/* Wait for attestation completion */
> +		ret = wait_for_completion_interruptible_timeout(
> +				&adev->req_compl,
> +				msecs_to_jiffies(GET_QUOTE_TIMEOUT));

If timeout occurs, the state of adev->tdquote_buf is unknown.  It's not safe
to continue to using adev->tdquote_buf.  VMM would continue to processing
getquote request with this buffer.  What if TDX_CMD_GEN_QUOTE is issued again,
and tdquote_buf is re-used?
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/19/22 6:20 PM, Isaku Yamahata wrote:
> If timeout occurs, the state of adev->tdquote_buf is unknown.  It's not safe
> to continue to using adev->tdquote_buf.  VMM would continue to processing
> getquote request with this buffer.  What if TDX_CMD_GEN_QUOTE is issued again,
> and tdquote_buf is re-used?

This part is not clearly discussed in the specification. May be spec
should define some reasonable timeout and teardown details.

Regarding not using this buffer again, what happens if we de-allocate
it on timeout and the host still updates it?

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Isaku Yamahata]

On Tue, Apr 19, 2022 at 06:26:43PM -0700,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> On 4/19/22 6:20 PM, Isaku Yamahata wrote:
> > If timeout occurs, the state of adev->tdquote_buf is unknown.  It's not safe
> > to continue to using adev->tdquote_buf.  VMM would continue to processing
> > getquote request with this buffer.  What if TDX_CMD_GEN_QUOTE is issued again,
> > and tdquote_buf is re-used?
> 
> This part is not clearly discussed in the specification. May be spec
> should define some reasonable timeout and teardown details.
> 
> Regarding not using this buffer again, what happens if we de-allocate
> it on timeout and the host still updates it?

Until GET_QUOTE_IN_FLIGHT is cleared, the shared page is owned by VMM, TD
attestation driver shouldn't reuse/free the pages.

In the case of this driver, I think of two options
- don't timeout. wait for interrupt to arrive and check the shared GPA state.
- allow timeout. When the next request comes, check the shared GPA state.
  If it's still GET_QUOTE_IN_FLIGHT, return EBUSY.

It's possible for VMM to keep the shared GPA forever maliciously(DoS) or
unintentionally due to bug.  TD can't do much about it.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/21/22 12:04 AM, Isaku Yamahata wrote:
> On Tue, Apr 19, 2022 at 06:26:43PM -0700,
> Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>> On 4/19/22 6:20 PM, Isaku Yamahata wrote:
>>> If timeout occurs, the state of adev->tdquote_buf is unknown.  It's not safe
>>> to continue to using adev->tdquote_buf.  VMM would continue to processing
>>> getquote request with this buffer.  What if TDX_CMD_GEN_QUOTE is issued again,
>>> and tdquote_buf is re-used?
>>
>> This part is not clearly discussed in the specification. May be spec
>> should define some reasonable timeout and teardown details.
>>
>> Regarding not using this buffer again, what happens if we de-allocate
>> it on timeout and the host still updates it?
> 
> Until GET_QUOTE_IN_FLIGHT is cleared, the shared page is owned by VMM, TD
> attestation driver shouldn't reuse/free the pages.
> 
> In the case of this driver, I think of two options
> - don't timeout. wait for interrupt to arrive and check the shared GPA state.
> - allow timeout. When the next request comes, check the shared GPA state.
>    If it's still GET_QUOTE_IN_FLIGHT, return EBUSY.

Out of the above two options, I think option 1 is better. It is simpler
to serialize them using mutex compared to checking the shared buffer of
previous request.

> 
> It's possible for VMM to keep the shared GPA forever maliciously(DoS) or
> unintentionally due to bug.  TD can't do much about it.

I will add a note about it in commit log.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> TDX guest supports encrypted disk as root or secondary drives.
> Decryption keys required to access such drives are usually maintained
> by 3rd party key servers. Attestation is required by 3rd party key
> servers to get the key for an encrypted disk volume, or possibly other
> encrypted services. Attestation is used to prove to the key server that
> the TD guest is running in a valid TD and the kernel and virtual BIOS
> and other environment are secure.
> 
> During the boot process various components before the kernel accumulate
> hashes in the TDX module, which can then combined into a report. This
> would typically include a hash of the bios, bios configuration, boot
> loader, command line, kernel, initrd.  After checking the hashes the
> key server will securely release the keys.
> 
> The actual details of the attestation protocol depend on the particular
> key server configuration, but some parts are common and need to
> communicate with the TDX module.

As we discussed "key provisioning from key server to support disk decryption" is
only one use case of attestation, so I don't think you need 3 paragraphs to talk
about details of this use case here.  The attestation flow is documented in GHCI
so it's clear.  The attestation flow (and what this patch does) does not have
any direct relation to the "disk decryption" details above.  I think you can
discard above entirely or using one or two simple sentences to explain.

Also, as you agreed you will remove the 8K shared memory assumption:

https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/

and if you agree with my approach (again, I recommend) to split the driver to
two parts (reorganize your patches essentially):

https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m9e3c5115df0be0b53d41f987e1eda1715255d1d8

I'll review again once you finish updating the driver.

Btw some minor comments below.


[...]

> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c

Perhaps attest.c is enough, no matter where the file will reside.

> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel_tdx_attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2021-2022 Intel Corporation

For upstream I guess just need 2022.

[...]

> +struct attest_dev {
> +	/* Mutex to serialize attestation requests */
> +	struct mutex lock;

I think we need a comment to explain why the driver doesn't support multiple
GetQuote requests in parallel.  In fact the updated GHCI spec doesn't explicitly
say GetQuote cannot be done in parallel.  It has a new "GET_QUOTE_IN_FLIGHT"
flag introduced, which can be used to determine which Quote is finished I think.

I am fine with only supporting GetQuote in serialized way, but perhaps we need
to call out the reason somewhere.

[...]

> +
> +	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
> +	adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE,
> +						&adev->handle,
> +						GFP_KERNEL | __GFP_ZERO);
> +	if (!adev->tdquote_buf) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}

The buffer needs to be shared.  Guest must have called MapGPA to convert the
buffer to shared.  Is this guaranteed by calling dma_alloc_coherent(), since
seems I didn't see any MapGPA in the driver?  Anyway this deserves a comment I
think.


-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi,

On 4/20/22 4:18 PM, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
>> TDX guest supports encrypted disk as root or secondary drives.
>> Decryption keys required to access such drives are usually maintained
>> by 3rd party key servers. Attestation is required by 3rd party key
>> servers to get the key for an encrypted disk volume, or possibly other
>> encrypted services. Attestation is used to prove to the key server that
>> the TD guest is running in a valid TD and the kernel and virtual BIOS
>> and other environment are secure.
>>
>> During the boot process various components before the kernel accumulate
>> hashes in the TDX module, which can then combined into a report. This
>> would typically include a hash of the bios, bios configuration, boot
>> loader, command line, kernel, initrd.  After checking the hashes the
>> key server will securely release the keys.
>>
>> The actual details of the attestation protocol depend on the particular
>> key server configuration, but some parts are common and need to
>> communicate with the TDX module.
> 
> As we discussed "key provisioning from key server to support disk decryption" is
> only one use case of attestation, so I don't think you need 3 paragraphs to talk
> about details of this use case here.  The attestation flow is documented in GHCI

Not everyone understands the attestation context and usage. So I have 
attempted to explain one of the use-case in detail.


> so it's clear.  The attestation flow (and what this patch does) does not have
> any direct relation to the "disk decryption" details above.  I think you can
> discard above entirely or using one or two simple sentences to explain.

Ok. I will try to summarize the details in few lines

> 
> Also, as you agreed you will remove the 8K shared memory assumption:
> 
> https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/

Yes. I have already removed this restriction. This will be part of my
next submission.

> 
> and if you agree with my approach (again, I recommend) to split the driver to
> two parts (reorganize your patches essentially):

Yes. I have moved the GetQuote support to a new patch (but without new
config).

> 
> https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m9e3c5115df0be0b53d41f987e1eda1715255d1d8
> 
> I'll review again once you finish updating the driver.

Thanks.

> 
> Btw some minor comments below.
> 
> 
> [...]
> 
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> 
> Perhaps attest.c is enough, no matter where the file will reside.

Noted. I will change it.

> 
>> @@ -0,0 +1,302 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * intel_tdx_attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process and
>> + * read the TD Quote result.
>> + *
>> + * Copyright (C) 2021-2022 Intel Corporation
> 
> For upstream I guess just need 2022.
> 
> [...]

Noted. I will change it.

> 
>> +struct attest_dev {
>> +	/* Mutex to serialize attestation requests */
>> +	struct mutex lock;
> 
> I think we need a comment to explain why the driver doesn't support multiple
> GetQuote requests in parallel.  In fact the updated GHCI spec doesn't explicitly
> say GetQuote cannot be done in parallel.  It has a new "GET_QUOTE_IN_FLIGHT"
> flag introduced, which can be used to determine which Quote is finished I think.
> 
> I am fine with only supporting GetQuote in serialized way, but perhaps we need
> to call out the reason somewhere.

If we want to support multiple GetQuote requests in parallel, then we
need some way to uniquely identify the GetQuote requests. So that when
we get completion notification, we can understand which request is
completed. This part is not mentioned/discussed in ABI spec. So we want 
to serialize the requests for now.

I will include the details about it in the commit log.

> 
> [...]
> 
>> +
>> +	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
>> +	adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE,
>> +						&adev->handle,
>> +						GFP_KERNEL | __GFP_ZERO);
>> +	if (!adev->tdquote_buf) {
>> +		ret = -ENOMEM;
>> +		goto failed;
>> +	}
> 
> The buffer needs to be shared.  Guest must have called MapGPA to convert the
> buffer to shared.  Is this guaranteed by calling dma_alloc_coherent(), since
> seems I didn't see any MapGPA in the driver?  Anyway this deserves a comment I
> think.

Yes. It is taken care by dma_alloc_*() API. DMA memory is marked by
default as shared in TDX guest. I will add comment about it.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> If we want to support multiple GetQuote requests in parallel, then we
> need some way to uniquely identify the GetQuote requests. So that when
> we get completion notification, we can understand which request is
> completed. This part is not mentioned/discussed in ABI spec. So we want 
> to serialize the requests for now.
> 

Yes it's unfortunate that this part (whether concurrent GetQuote requests are
supported by TDX architecture) is not explicitly mentioned in GHCI spec.  I am
fine with only supporting GetQuote requests one by one.  AFAICT there's no
request to support concurrent GetQuote requests anyway.  What concerns me is
exactly how explain this.

As I said, we have GET_QUOTE_IN_FLIGHT flag now.  Theoretically, you can queue
multiple GetQuote requests, and when you receive the interrupt, you check which
buffer has GET_QUOTE_IN_FLIGHT cleared.  That buffer is the one with Quote
ready.  However I am not 100% sure whether above will always work.  Interrupt
can get lost when there are multiple Quotes ready in multiple buffer in very
short time period, etc?  Perhaps Isaku can provide more input here.

Anyway, how about explaining in this way:

"The GHCI spec doesn't clearly say whether TDX can support or how to support
multiple GetQuote requests in parallel.  Attestation request is not supposed to
be frequent and should not be in performance critical path.  Only support
GetQuote requests in serialized way for now." 


-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/20/22 5:11 PM, Kai Huang wrote:
> On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
>> If we want to support multiple GetQuote requests in parallel, then we
>> need some way to uniquely identify the GetQuote requests. So that when
>> we get completion notification, we can understand which request is
>> completed. This part is not mentioned/discussed in ABI spec. So we want
>> to serialize the requests for now.
>>
> 
> Yes it's unfortunate that this part (whether concurrent GetQuote requests are
> supported by TDX architecture) is not explicitly mentioned in GHCI spec.  I am
> fine with only supporting GetQuote requests one by one.  AFAICT there's no
> request to support concurrent GetQuote requests anyway.  What concerns me is
> exactly how explain this.
> 
> As I said, we have GET_QUOTE_IN_FLIGHT flag now.  Theoretically, you can queue
> multiple GetQuote requests, and when you receive the interrupt, you check which
> buffer has GET_QUOTE_IN_FLIGHT cleared.  That buffer is the one with Quote
> ready.  However I am not 100% sure whether above will always work.  Interrupt
> can get lost when there are multiple Quotes ready in multiple buffer in very
> short time period, etc?  Perhaps Isaku can provide more input here.

Either supported or not, it should be mentioned in the GHCI spec. 
Currently, there are no details related to it. If it is supported, the 
specification should include the protocol to use.

I will check with Isaku about it.

> 
> Anyway, how about explaining in this way:
> 
> "The GHCI spec doesn't clearly say whether TDX can support or how to support
> multiple GetQuote requests in parallel.  Attestation request is not supposed to
> be frequent and should not be in performance critical path.  Only support
> GetQuote requests in serialized way for now."

It seems good enough. I will use it.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Isaku Yamahata]

On Wed, Apr 20, 2022 at 07:42:06PM -0700,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> 
> 
> On 4/20/22 5:11 PM, Kai Huang wrote:
> > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > If we want to support multiple GetQuote requests in parallel, then we
> > > need some way to uniquely identify the GetQuote requests. So that when
> > > we get completion notification, we can understand which request is
> > > completed. This part is not mentioned/discussed in ABI spec. So we want
> > > to serialize the requests for now.
> > > 
> > 
> > Yes it's unfortunate that this part (whether concurrent GetQuote requests are
> > supported by TDX architecture) is not explicitly mentioned in GHCI spec.  I am
> > fine with only supporting GetQuote requests one by one.  AFAICT there's no
> > request to support concurrent GetQuote requests anyway.  What concerns me is
> > exactly how explain this.
> > 
> > As I said, we have GET_QUOTE_IN_FLIGHT flag now.  Theoretically, you can queue
> > multiple GetQuote requests, and when you receive the interrupt, you check which
> > buffer has GET_QUOTE_IN_FLIGHT cleared.  That buffer is the one with Quote
> > ready.  However I am not 100% sure whether above will always work.  Interrupt
> > can get lost when there are multiple Quotes ready in multiple buffer in very
> > short time period, etc?  Perhaps Isaku can provide more input here.
> 
> Either supported or not, it should be mentioned in the GHCI spec. Currently,
> there are no details related to it. If it is supported, the specification
> should include the protocol to use.
> 
> I will check with Isaku about it.

The spec says that TD can call multiple GetQuote requests in parallel.

  TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
  implementation specific that how many concurrent requests are allowed. The TD
  should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
  requests simultaneously

As Kai said, there is no requirement for multiple GetQuote in parallel, it's
okay to support only single request at the same time.

While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA.  The
attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before
sending next request.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kai Huang]

On Wed, 2022-04-20 at 23:57 -0700, Isaku Yamahata wrote:
> On Wed, Apr 20, 2022 at 07:42:06PM -0700,
> Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
> > 
> > 
> > On 4/20/22 5:11 PM, Kai Huang wrote:
> > > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > > If we want to support multiple GetQuote requests in parallel, then we
> > > > need some way to uniquely identify the GetQuote requests. So that when
> > > > we get completion notification, we can understand which request is
> > > > completed. This part is not mentioned/discussed in ABI spec. So we want
> > > > to serialize the requests for now.
> > > > 
> > > 
> > > Yes it's unfortunate that this part (whether concurrent GetQuote requests are
> > > supported by TDX architecture) is not explicitly mentioned in GHCI spec.  I am
> > > fine with only supporting GetQuote requests one by one.  AFAICT there's no
> > > request to support concurrent GetQuote requests anyway.  What concerns me is
> > > exactly how explain this.
> > > 
> > > As I said, we have GET_QUOTE_IN_FLIGHT flag now.  Theoretically, you can queue
> > > multiple GetQuote requests, and when you receive the interrupt, you check which
> > > buffer has GET_QUOTE_IN_FLIGHT cleared.  That buffer is the one with Quote
> > > ready.  However I am not 100% sure whether above will always work.  Interrupt
> > > can get lost when there are multiple Quotes ready in multiple buffer in very
> > > short time period, etc?  Perhaps Isaku can provide more input here.
> > 
> > Either supported or not, it should be mentioned in the GHCI spec. Currently,
> > there are no details related to it. If it is supported, the specification
> > should include the protocol to use.
> > 
> > I will check with Isaku about it.
> 
> The spec says that TD can call multiple GetQuote requests in parallel.
> 
>   TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
>   implementation specific that how many concurrent requests are allowed. The TD
>   should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
>   requests simultaneously
> 
> As Kai said, there is no requirement for multiple GetQuote in parallel, it's
> okay to support only single request at the same time.
> 
> While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA.  The
> attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before
> sending next request.

Sorry I missed this in the spec.  Then as I mentioned above, TD should check
which buffer has GET_QUOTE_IN_FLIGHT bit cleared to determine which GetQuote
request is done?  I guess this is the only way.

Anyway, supporting single request only is fine to me.  Just needs some
explanation in comments or commit message.

-- 
Thanks,
-Kai

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/20/22 11:57 PM, Isaku Yamahata wrote:
> On Wed, Apr 20, 2022 at 07:42:06PM -0700,
> Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>>
>>
>> On 4/20/22 5:11 PM, Kai Huang wrote:
>>> On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>> If we want to support multiple GetQuote requests in parallel, then we
>>>> need some way to uniquely identify the GetQuote requests. So that when
>>>> we get completion notification, we can understand which request is
>>>> completed. This part is not mentioned/discussed in ABI spec. So we want
>>>> to serialize the requests for now.
>>>>
>>>
>>> Yes it's unfortunate that this part (whether concurrent GetQuote requests are
>>> supported by TDX architecture) is not explicitly mentioned in GHCI spec.  I am
>>> fine with only supporting GetQuote requests one by one.  AFAICT there's no
>>> request to support concurrent GetQuote requests anyway.  What concerns me is
>>> exactly how explain this.
>>>
>>> As I said, we have GET_QUOTE_IN_FLIGHT flag now.  Theoretically, you can queue
>>> multiple GetQuote requests, and when you receive the interrupt, you check which
>>> buffer has GET_QUOTE_IN_FLIGHT cleared.  That buffer is the one with Quote
>>> ready.  However I am not 100% sure whether above will always work.  Interrupt
>>> can get lost when there are multiple Quotes ready in multiple buffer in very
>>> short time period, etc?  Perhaps Isaku can provide more input here.
>>
>> Either supported or not, it should be mentioned in the GHCI spec. Currently,
>> there are no details related to it. If it is supported, the specification
>> should include the protocol to use.
>>
>> I will check with Isaku about it.
> 
> The spec says that TD can call multiple GetQuote requests in parallel.

Sorry, I missed the above content. Thanks for pointing out.

> 
>    TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
>    implementation specific that how many concurrent requests are allowed. The TD
>    should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
>    requests simultaneously

Do you know why we should handle VMCALL_RETRY case? IIUC, as per
above spec, if each request we send uses different GPA buffer, then we
should not even worry about checking for IN_FLIGHT status. right?

> 
> As Kai said, there is no requirement for multiple GetQuote in parallel, it's
> okay to support only single request at the same time.

For now I will leave it as single request at a time.

> 
> While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA.  The
> attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before
> sending next request.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Isaku Yamahata]

On Thu, Apr 21, 2022 at 07:53:39AM -0700,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> On 4/20/22 11:57 PM, Isaku Yamahata wrote:
> > On Wed, Apr 20, 2022 at 07:42:06PM -0700,
> > Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> > 
> >    TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
> >    implementation specific that how many concurrent requests are allowed. The TD
> >    should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
> >    requests simultaneously
> 
> Do you know why we should handle VMCALL_RETRY case? IIUC, as per
> above spec, if each request we send uses different GPA buffer, then we
> should not even worry about checking for IN_FLIGHT status. right?

Not correct.  User space  VMM, e.g. qemu, may return RETRY error for various
reasons even if different GPAs are used or even if only single request is issued
at the same time.  Other user space VMM (there are severals alternatives to qemu)
would support TDX in future. They would choose different way to implement.


Attestation driver should check IN_FLIGHT always before processing shared GPA.
Interrupt notifies only that it needs attention from attestation driver.  It
doesn't guarantee that IN_FLIGHT is cleared. Interrupt indicates only that the
state may be changed.  It may not be changed.  VMM inject the iterrupt
spuriously.
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>