lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v7 18/20] x86/virt/tdx: Initialize all TDMRs
    From
    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
    >

    \
     
     \ /
      Last update: 2022-11-24 01:43    [W:4.065 / U:0.828 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site