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

From: Dave Hansen <dave.hansen@intel.com>
To: Kai Huang <kai.huang@intel.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: linux-mm@kvack.org, seanjc@google.com, pbonzini@redhat.com,
	dan.j.williams@intel.com, rafael.j.wysocki@intel.com,
	kirill.shutemov@linux.intel.com, ying.huang@intel.com,
	reinette.chatre@intel.com, len.brown@intel.com,
	tony.luck@intel.com, peterz@infradead.org, ak@linux.intel.com,
	isaku.yamahata@intel.com, chao.gao@intel.com,
	sathyanarayanan.kuppuswamy@linux.intel.com, bagasdotme@gmail.com,
	sagis@google.com, imammedo@redhat.com
Subject: Re: [PATCH v7 18/20] x86/virt/tdx: Initialize all TDMRs
Date: Wed, 23 Nov 2022 16:42:17 -0800	[thread overview]
Message-ID: <4e19a7e1-9088-21d6-d521-a76c340d8179@intel.com> (raw)
In-Reply-To: <2337c8e9086a006aaa2c4b99caf478420d1fc640.1668988357.git.kai.huang@intel.com>

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
>  