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>
