diff mbox series

[v7,18/20] x86/virt/tdx: Initialize all TDMRs

Message ID 2337c8e9086a006aaa2c4b99caf478420d1fc640.1668988357.git.kai.huang@intel.com (mailing list archive)
State New
Headers show
Series TDX host kernel support | expand

Commit Message

Huang, Kai Nov. 21, 2022, 12:26 a.m. UTC 
Initialize TDMRs via TDH.SYS.TDMR.INIT as the last step to complete the
TDX initialization.

All TDMRs need to be initialized using TDH.SYS.TDMR.INIT SEAMCALL before
the memory pages can be used by the TDX module.  The time to initialize
TDMR is proportional to the size of the TDMR because TDH.SYS.TDMR.INIT
internally initializes the PAMT entries using the global KeyID.

To avoid long latency caused in one SEAMCALL, TDH.SYS.TDMR.INIT only
initializes an (implementation-specific) subset of PAMT entries of one
TDMR in one invocation.  The caller needs to call TDH.SYS.TDMR.INIT
iteratively until all PAMT entries of the given TDMR are initialized.

TDH.SYS.TDMR.INITs can run concurrently on multiple CPUs as long as they
are initializing different TDMRs.  To keep it simple, just initialize
all TDMRs one by one.  On a 2-socket machine with 2.2G CPUs and 64GB
memory, each TDH.SYS.TDMR.INIT roughly takes couple of microseconds on
average, and it takes roughly dozens of milliseconds to complete the
initialization of all TDMRs while system is idle.

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v6 -> v7:
 - Removed need_resched() check. -- Andi.

---
 arch/x86/virt/vmx/tdx/tdx.c | 69 ++++++++++++++++++++++++++++++++++---
 arch/x86/virt/vmx/tdx/tdx.h |  1 +
 2 files changed, 65 insertions(+), 5 deletions(-)

Comments

Dave Hansen Nov. 24, 2022, 12:42 a.m. UTC | #1 
On 11/20/22 16:26, Kai Huang wrote:
> Initialize TDMRs via TDH.SYS.TDMR.INIT as the last step to complete the
> TDX initialization.
> 
> All TDMRs need to be initialized using TDH.SYS.TDMR.INIT SEAMCALL before
> the memory pages can be used by the TDX module.  The time to initialize
> TDMR is proportional to the size of the TDMR because TDH.SYS.TDMR.INIT
> internally initializes the PAMT entries using the global KeyID.
> 
> To avoid long latency caused in one SEAMCALL, TDH.SYS.TDMR.INIT only
> initializes an (implementation-specific) subset of PAMT entries of one
> TDMR in one invocation.  The caller needs to call TDH.SYS.TDMR.INIT
> iteratively until all PAMT entries of the given TDMR are initialized.
> 
> TDH.SYS.TDMR.INITs can run concurrently on multiple CPUs as long as they
> are initializing different TDMRs.  To keep it simple, just initialize
> all TDMRs one by one.  On a 2-socket machine with 2.2G CPUs and 64GB
> memory, each TDH.SYS.TDMR.INIT roughly takes couple of microseconds on
> average, and it takes roughly dozens of milliseconds to complete the
> initialization of all TDMRs while system is idle.

Any chance you could say TDH.SYS.TDMR.INIT a few more times in there? ;)

Seriously, please try to trim that down.  If you talk about the
implementation in detail in the code comments, don't cover it in detail
in the changelog too.

Also, this is a momentous patch, right?  It's the last piece.  Maybe
worth calling that out.

> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 99d1be5941a7..9bcdb30b7a80 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1066,6 +1066,65 @@ static int config_global_keyid(void)
>  	return seamcall_on_each_package_serialized(&sc);
>  }
>  
> +/* Initialize one TDMR */

Does this comment add value?  Even if it does, it is better than naming
the dang function init_one_tdmr()?

> +static int init_tdmr(struct tdmr_info *tdmr)
> +{
> +	u64 next;
> +
> +	/*
> +	 * Initializing PAMT entries might be time-consuming (in
> +	 * proportion to the size of the requested TDMR).  To avoid long
> +	 * latency in one SEAMCALL, TDH.SYS.TDMR.INIT only initializes
> +	 * an (implementation-defined) subset of PAMT entries in one
> +	 * invocation.
> +	 *
> +	 * Call TDH.SYS.TDMR.INIT iteratively until all PAMT entries
> +	 * of the requested TDMR are initialized (if next-to-initialize
> +	 * address matches the end address of the TDMR).
> +	 */

The PAMT discussion here is, IMNHO silly.  If this were about
initializing PAMT's then it should be renamed init_pamts() and the
SEAMCALL should be called PAMT_INIT or soemthing.  It's not, and the ABI
is built around the TDMR and *its* addresses.

Let's chop this comment down:

	/*
	 * Initializing a TDMR can be time consuming.  To avoid long
	 * SEAMCALLs, the TDX module may only initialize a part of the
	 * TDMR in each call.
	 */

Talk about the looping logic in the loop.

> +	do {
> +		struct tdx_module_output out;
> +		int ret;
> +
> +		ret = seamcall(TDH_SYS_TDMR_INIT, tdmr->base, 0, 0, 0, NULL,
> +				&out);
> +		if (ret)
> +			return ret;
> +		/*
> +		 * RDX contains 'next-to-initialize' address if
> +		 * TDH.SYS.TDMR.INT succeeded.
> +		 */
> +		next = out.rdx;
> +		/* Allow scheduling when needed */

That comment is a wee bit superfluous, don't you think?

> +		cond_resched();

		/* Keep making SEAMCALLs until the TDMR is done */

> +	} while (next < tdmr->base + tdmr->size);
> +
> +	return 0;
> +}
> +
> +/* Initialize all TDMRs */

Does this comment add value?

> +static int init_tdmrs(struct tdmr_info *tdmr_array, int tdmr_num)
> +{
> +	int i;
> +
> +	/*
> +	 * Initialize TDMRs one-by-one for simplicity, though the TDX
> +	 * architecture does allow different TDMRs to be initialized in
> +	 * parallel on multiple CPUs.  Parallel initialization could
> +	 * be added later when the time spent in the serialized scheme
> +	 * becomes a real concern.
> +	 */

Trim down the comment:

	/*
	 * This operation is costly.  It can be parallelized,
	 * but keep it simple for now.
	 */

> +	for (i = 0; i < tdmr_num; i++) {
> +		int ret;
> +
> +		ret = init_tdmr(tdmr_array_entry(tdmr_array, i));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Detect and initialize the TDX module.
>   *
> @@ -1172,11 +1231,11 @@ static int init_tdx_module(void)
>  	if (ret)
>  		goto out_free_pamts;
>  
> -	/*
> -	 * Return -EINVAL until all steps of TDX module initialization
> -	 * process are done.
> -	 */
> -	ret = -EINVAL;
> +	/* Initialize TDMRs to complete the TDX module initialization */
> +	ret = init_tdmrs(tdmr_array, tdmr_num);
> +	if (ret)
> +		goto out_free_pamts;
> +
>  out_free_pamts:
>  	if (ret) {
>  		/*
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 768d097412ab..891691b1ea50 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -19,6 +19,7 @@
>  #define TDH_SYS_INFO		32
>  #define TDH_SYS_INIT		33
>  #define TDH_SYS_LP_INIT		35
> +#define TDH_SYS_TDMR_INIT	36
>  #define TDH_SYS_LP_SHUTDOWN	44
>  #define TDH_SYS_CONFIG		45
>
Huang, Kai Nov. 25, 2022, 2:27 a.m. UTC | #2 
On Wed, 2022-11-23 at 16:42 -0800, Dave Hansen wrote:
> On 11/20/22 16:26, Kai Huang wrote:
> > Initialize TDMRs via TDH.SYS.TDMR.INIT as the last step to complete the
> > TDX initialization.
> > 
> > All TDMRs need to be initialized using TDH.SYS.TDMR.INIT SEAMCALL before
> > the memory pages can be used by the TDX module.  The time to initialize
> > TDMR is proportional to the size of the TDMR because TDH.SYS.TDMR.INIT
> > internally initializes the PAMT entries using the global KeyID.
> > 
> > To avoid long latency caused in one SEAMCALL, TDH.SYS.TDMR.INIT only
> > initializes an (implementation-specific) subset of PAMT entries of one
> > TDMR in one invocation.  The caller needs to call TDH.SYS.TDMR.INIT
> > iteratively until all PAMT entries of the given TDMR are initialized.
> > 
> > TDH.SYS.TDMR.INITs can run concurrently on multiple CPUs as long as they
> > are initializing different TDMRs.  To keep it simple, just initialize
> > all TDMRs one by one.  On a 2-socket machine with 2.2G CPUs and 64GB
> > memory, each TDH.SYS.TDMR.INIT roughly takes couple of microseconds on
> > average, and it takes roughly dozens of milliseconds to complete the
> > initialization of all TDMRs while system is idle.
> 
> Any chance you could say TDH.SYS.TDMR.INIT a few more times in there? ;)

I am a bad changelog writer.

> 
> Seriously, please try to trim that down.  If you talk about the
> implementation in detail in the code comments, don't cover it in detail
> in the changelog too.

Yes will use this tip to trim down.  Thanks for the tip.

> 
> Also, this is a momentous patch, right?  It's the last piece.  Maybe
> worth calling that out.

Yes this is the last step of initializing the TDX module.  It is sort of
mentioned in the first sentence of this changelong:

	Initialize TDMRs via TDH.SYS.TDMR.INIT as the last step to complete the
	TDX initialization.

But perhaps it can be more explicitly.

> 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 99d1be5941a7..9bcdb30b7a80 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -1066,6 +1066,65 @@ static int config_global_keyid(void)
> >  	return seamcall_on_each_package_serialized(&sc);
> >  }
> >  
> > +/* Initialize one TDMR */
> 
> Does this comment add value?  Even if it does, it is better than naming
> the dang function init_one_tdmr()?

Sorry will try best to avoid such type of comments in the future.

> 
> > +static int init_tdmr(struct tdmr_info *tdmr)
> > +{
> > +	u64 next;
> > +
> > +	/*
> > +	 * Initializing PAMT entries might be time-consuming (in
> > +	 * proportion to the size of the requested TDMR).  To avoid long
> > +	 * latency in one SEAMCALL, TDH.SYS.TDMR.INIT only initializes
> > +	 * an (implementation-defined) subset of PAMT entries in one
> > +	 * invocation.
> > +	 *
> > +	 * Call TDH.SYS.TDMR.INIT iteratively until all PAMT entries
> > +	 * of the requested TDMR are initialized (if next-to-initialize
> > +	 * address matches the end address of the TDMR).
> > +	 */
> 
> The PAMT discussion here is, IMNHO silly.  If this were about
> initializing PAMT's then it should be renamed init_pamts() and the
> SEAMCALL should be called PAMT_INIT or soemthing.  It's not, and the ABI
> is built around the TDMR and *its* addresses.

Agreed.

> 
> Let's chop this comment down:
> 
> 	/*
> 	 * Initializing a TDMR can be time consuming.  To avoid long
> 	 * SEAMCALLs, the TDX module may only initialize a part of the
> 	 * TDMR in each call.
> 	 */
> 
> Talk about the looping logic in the loop.

Agreed. Thanks.

> 
> > +	do {
> > +		struct tdx_module_output out;
> > +		int ret;
> > +
> > +		ret = seamcall(TDH_SYS_TDMR_INIT, tdmr->base, 0, 0, 0, NULL,
> > +				&out);
> > +		if (ret)
> > +			return ret;
> > +		/*
> > +		 * RDX contains 'next-to-initialize' address if
> > +		 * TDH.SYS.TDMR.INT succeeded.
> > +		 */
> > +		next = out.rdx;
> > +		/* Allow scheduling when needed */
> 
> That comment is a wee bit superfluous, don't you think?

Indeed.

> 
> > +		cond_resched();
> 
> 		/* Keep making SEAMCALLs until the TDMR is done */
> 
> > +	} while (next < tdmr->base + tdmr->size);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Initialize all TDMRs */
> 
> Does this comment add value?

No.  Will remove.

> 
> > +static int init_tdmrs(struct tdmr_info *tdmr_array, int tdmr_num)
> > +{
> > +	int i;
> > +
> > +	/*
> > +	 * Initialize TDMRs one-by-one for simplicity, though the TDX
> > +	 * architecture does allow different TDMRs to be initialized in
> > +	 * parallel on multiple CPUs.  Parallel initialization could
> > +	 * be added later when the time spent in the serialized scheme
> > +	 * becomes a real concern.
> > +	 */
> 
> Trim down the comment:
> 
> 	/*
> 	 * This operation is costly.  It can be parallelized,
> 	 * but keep it simple for now.
> 	 */

Thanks.

[...]
diff mbox series

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 99d1be5941a7..9bcdb30b7a80 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1066,6 +1066,65 @@  static int config_global_keyid(void)
 	return seamcall_on_each_package_serialized(&sc);
 }
 
+/* Initialize one TDMR */
+static int init_tdmr(struct tdmr_info *tdmr)
+{
+	u64 next;
+
+	/*
+	 * Initializing PAMT entries might be time-consuming (in
+	 * proportion to the size of the requested TDMR).  To avoid long
+	 * latency in one SEAMCALL, TDH.SYS.TDMR.INIT only initializes
+	 * an (implementation-defined) subset of PAMT entries in one
+	 * invocation.
+	 *
+	 * Call TDH.SYS.TDMR.INIT iteratively until all PAMT entries
+	 * of the requested TDMR are initialized (if next-to-initialize
+	 * address matches the end address of the TDMR).
+	 */
+	do {
+		struct tdx_module_output out;
+		int ret;
+
+		ret = seamcall(TDH_SYS_TDMR_INIT, tdmr->base, 0, 0, 0, NULL,
+				&out);
+		if (ret)
+			return ret;
+		/*
+		 * RDX contains 'next-to-initialize' address if
+		 * TDH.SYS.TDMR.INT succeeded.
+		 */
+		next = out.rdx;
+		/* Allow scheduling when needed */
+		cond_resched();
+	} while (next < tdmr->base + tdmr->size);
+
+	return 0;
+}
+
+/* Initialize all TDMRs */
+static int init_tdmrs(struct tdmr_info *tdmr_array, int tdmr_num)
+{
+	int i;
+
+	/*
+	 * Initialize TDMRs one-by-one for simplicity, though the TDX
+	 * architecture does allow different TDMRs to be initialized in
+	 * parallel on multiple CPUs.  Parallel initialization could
+	 * be added later when the time spent in the serialized scheme
+	 * becomes a real concern.
+	 */
+	for (i = 0; i < tdmr_num; i++) {
+		int ret;
+
+		ret = init_tdmr(tdmr_array_entry(tdmr_array, i));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * Detect and initialize the TDX module.
  *
@@ -1172,11 +1231,11 @@  static int init_tdx_module(void)
 	if (ret)
 		goto out_free_pamts;
 
-	/*
-	 * Return -EINVAL until all steps of TDX module initialization
-	 * process are done.
-	 */
-	ret = -EINVAL;
+	/* Initialize TDMRs to complete the TDX module initialization */
+	ret = init_tdmrs(tdmr_array, tdmr_num);
+	if (ret)
+		goto out_free_pamts;
+
 out_free_pamts:
 	if (ret) {
 		/*
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 768d097412ab..891691b1ea50 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -19,6 +19,7 @@ 
 #define TDH_SYS_INFO		32
 #define TDH_SYS_INIT		33
 #define TDH_SYS_LP_INIT		35
+#define TDH_SYS_TDMR_INIT	36
 #define TDH_SYS_LP_SHUTDOWN	44
 #define TDH_SYS_CONFIG		45