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.
[...]
