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
>
