Messages in this thread | | | Date | Wed, 7 Jun 2023 08:22:36 -0700 | Subject | Re: [PATCH v11 07/20] x86/virt/tdx: Add skeleton to enable TDX on demand | From | Dave Hansen <> |
| |
On 6/4/23 07:27, Kai Huang wrote: ... > +static int try_init_module_global(void) > +{ > + unsigned long flags; > + int ret; > + > + /* > + * The TDX module global initialization only needs to be done > + * once on any cpu. > + */ > + raw_spin_lock_irqsave(&tdx_global_init_lock, flags);
Why is this "raw_"?
There's zero mention of it anywhere.
> + if (tdx_global_init_status & TDX_GLOBAL_INIT_DONE) { > + ret = tdx_global_init_status & TDX_GLOBAL_INIT_FAILED ? > + -EINVAL : 0; > + goto out; > + } > + > + /* All '0's are just unused parameters. */ > + ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL); > + > + tdx_global_init_status = TDX_GLOBAL_INIT_DONE; > + if (ret) > + tdx_global_init_status |= TDX_GLOBAL_INIT_FAILED; > +out: > + raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags); > + > + return ret; > +} > + > +/** > + * tdx_cpu_enable - Enable TDX on local cpu > + * > + * Do one-time TDX module per-cpu initialization SEAMCALL (and TDX module > + * global initialization SEAMCALL if not done) on local cpu to make this > + * cpu be ready to run any other SEAMCALLs. > + * > + * Note this function must be called when preemption is not possible > + * (i.e. via SMP call or in per-cpu thread). It is not IRQ safe either > + * (i.e. cannot be called in per-cpu thread and via SMP call from remote > + * cpu simultaneously).
lockdep_assert_*() are your friends. Unlike comments, they will actually tell you if this goes wrong.
> +int tdx_cpu_enable(void) > +{ > + unsigned int lp_status; > + int ret; > + > + if (!platform_tdx_enabled()) > + return -EINVAL; > + > + lp_status = __this_cpu_read(tdx_lp_init_status); > + > + /* Already done */ > + if (lp_status & TDX_LP_INIT_DONE) > + return lp_status & TDX_LP_INIT_FAILED ? -EINVAL : 0; > + > + /* > + * The TDX module global initialization is the very first step > + * to enable TDX. Need to do it first (if hasn't been done) > + * before doing the per-cpu initialization. > + */ > + ret = try_init_module_global(); > + > + /* > + * If the module global initialization failed, there's no point > + * to do the per-cpu initialization. Just mark it as done but > + * failed. > + */ > + if (ret) > + goto update_status; > + > + /* All '0's are just unused parameters */ > + ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL); > + > +update_status: > + lp_status = TDX_LP_INIT_DONE; > + if (ret) > + lp_status |= TDX_LP_INIT_FAILED; > + > + this_cpu_write(tdx_lp_init_status, lp_status); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(tdx_cpu_enable);
You danced around it in the changelog, but the reason for the exports is not clear.
> +static int init_tdx_module(void) > +{ > + /* > + * TODO: > + * > + * - Get TDX module information and TDX-capable memory regions. > + * - Build the list of TDX-usable memory regions. > + * - Construct a list of "TD Memory Regions" (TDMRs) to cover > + * all TDX-usable memory regions. > + * - Configure the TDMRs and the global KeyID to the TDX module. > + * - Configure the global KeyID on all packages. > + * - Initialize all TDMRs. > + * > + * Return error before all steps are done. > + */ > + return -EINVAL; > +} > + > +static int __tdx_enable(void) > +{ > + int ret; > + > + ret = init_tdx_module(); > + if (ret) { > + pr_err("TDX module initialization failed (%d)\n", ret);
Have you actually gone any looked at how this pr_*()'s look?
Won't they say:
tdx: TDX module initialized
Isn't that a _bit_ silly? Why not just say:
pr_info("module initialized.\n");
| |