Re: [PATCH v8 14/16] x86/virt/tdx: Initialize all TDMRs

[Huang, Kai]

On Fri, 2023-01-06 at 16:17 -0800, Dave Hansen wrote:
> On 12/8/22 22:52, Kai Huang wrote:
> > After the global KeyID has been configured on all packages, initialize
> > all TDMRs to make all TDX-usable memory regions that are passed to the
> > TDX module become usable.
> > 
> > This is the last step of initializing the TDX module.
> > 
> > Initializing different TDMRs can be parallelized.  For now to keep it
> > simple, just initialize all TDMRs one by one.  It can be enhanced in the
> > future.
> 
> The changelog probably also needs a note about this being a long process
> and also at least touching on *why* it takes so long.

Will add.  How about:

	Initializing TDMRs can be time consuming on large memory systems as it 
	involves initializing all metadata entries for all pages that can be 
	used by TDX guests.

And put it before "Initializing different TDMRs can be parallelized ..."?

> 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 4c779e8412f1..8b7314f19df2 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -1006,6 +1006,55 @@ static int config_global_keyid(void)
> >  	return ret;
> >  }
> >  
> > +static int init_tdmr(struct tdmr_info *tdmr)
> > +{
> > +	u64 next;
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	do {
> > +		struct tdx_module_output out;
> > +		int ret;
> > +
> > +		/* All 0's are unused parameters, they mean nothing. */
> > +		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.
> 
> This reads strangely.  "Success" to me really is different from "partial
> success".  Sure, partial success is also not an error, *but* this can be
> explained better.  How about:
> 
> 		 * RDX contains 'next-to-initialize' address if
> 		 * TDH.SYS.TDMR.INT did not fully complete and should
> 		 * be retried.
> 

Will do.

[snip]