Messages in this thread Patch in this message | | | Date | Thu, 15 Jun 2023 11:12:07 +0300 | Subject | Re: [PATCH v11 15/20] x86/virt/tdx: Configure global KeyID on all packages | From | Nikolay Borisov <> |
| |
On 4.06.23 г. 17:27 ч., Kai Huang wrote: > After the list of TDMRs and the global KeyID are configured to the TDX > module, the kernel needs to configure the key of the global KeyID on all > packages using TDH.SYS.KEY.CONFIG. > > This SEAMCALL cannot run parallel on different cpus. Loop all online > cpus and use smp_call_on_cpu() to call this SEAMCALL on the first cpu of > each package. > > To keep things simple, this implementation takes no affirmative steps to > online cpus to make sure there's at least one cpu for each package. The > callers (aka. KVM) can ensure success by ensuring that.
The last sentence is a bit hard to read due to the multiple use of ensure/ensuring. OTOH I find the comment in the code somewhat more coherent:
> + * This code takes no affirmative steps to online CPUs. Callers (aka. > + * KVM) can ensure success by ensuring sufficient CPUs are online for > + * this to succeed. > + */
I'd suggest you just use those words. Or just saying "Callers (such as KVM) can ensure success by onlining at least 1 CPU per package."
<snip>
> static int init_tdx_module(void) > { > static DECLARE_PADDED_STRUCT(tdsysinfo_struct, tdsysinfo, > @@ -980,15 +1073,47 @@ static int init_tdx_module(void) > if (ret) > goto out_free_pamts; > > + /* > + * Hardware doesn't guarantee cache coherency across different > + * KeyIDs. The kernel needs to flush PAMT's dirty cachelines > + * (associated with KeyID 0) before the TDX module can use the > + * global KeyID to access the PAMT. Given PAMTs are potentially > + * large (~1/256th of system RAM), just use WBINVD on all cpus > + * to flush the cache. > + */ > + wbinvd_on_all_cpus(); > + > + /* Config the key of global KeyID on all packages */ > + ret = config_global_keyid(); > + if (ret) > + goto out_reset_pamts; > + > /* > * TODO: > * > - * - Configure the global KeyID on all packages. > * - Initialize all TDMRs. > * > * Return error before all steps are done. > */ > ret = -EINVAL; > +out_reset_pamts: > + if (ret) {
Again with those conditionals in the error paths. Just copy the put_mem_online(); ret 0. and this will decrease the indentation level and make the code linear. Here;s what the diff looks like:
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 4aa41352edfc..49fda2a28f24 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -1131,6 +1131,8 @@ static int init_tdx_module(void) if (ret) goto out_free_pamts;
+ pr_info("%lu KBs allocated for PAMT.\n", + tdmrs_count_pamt_pages(&tdx_tdmr_list) * 4); /* * Hardware doesn't guarantee cache coherency across different * KeyIDs. The kernel needs to flush PAMT's dirty cachelines @@ -1148,36 +1150,32 @@ static int init_tdx_module(void)
/* Initialize TDMRs to complete the TDX module initialization */ ret = init_tdmrs(&tdx_tdmr_list); + + put_online_mems(); + + return 0; out_reset_pamts: - if (ret) { - /* - * Part of PAMTs may already have been initialized by the - * TDX module. Flush cache before returning PAMTs back - * to the kernel. - */ - wbinvd_on_all_cpus(); - /* - * According to the TDX hardware spec, if the platform - * doesn't have the "partial write machine check" - * erratum, any kernel read/write will never cause #MC - * in kernel space, thus it's OK to not convert PAMTs - * back to normal. But do the conversion anyway here - * as suggested by the TDX spec. - */ - tdmrs_reset_pamt_all(&tdx_tdmr_list); - } + /* + * Part of PAMTs may already have been initialized by the + * TDX module. Flush cache before returning PAMTs back + * to the kernel. + */ + wbinvd_on_all_cpus(); + /* + * According to the TDX hardware spec, if the platform + * doesn't have the "partial write machine check" + * erratum, any kernel read/write will never cause #MC + * in kernel space, thus it's OK to not convert PAMTs + * back to normal. But do the conversion anyway here + * as suggested by the TDX spec. + */ + tdmrs_reset_pamt_all(&tdx_tdmr_list); out_free_pamts: - if (ret) - tdmrs_free_pamt_all(&tdx_tdmr_list); - else - pr_info("%lu KBs allocated for PAMT.\n", - tdmrs_count_pamt_pages(&tdx_tdmr_list) * 4); + tdmrs_free_pamt_all(&tdx_tdmr_list); out_free_tdmrs: - if (ret) - free_tdmr_list(&tdx_tdmr_list); + free_tdmr_list(&tdx_tdmr_list); out_free_tdx_mem: - if (ret) - free_tdx_memlist(&tdx_memlist); + free_tdx_memlist(&tdx_memlist); out: /* * @tdx_memlist is written here and read at memory hotplug time.
<snip>
| |