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

[Huang, Kai]

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.

[...]