| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <49bd9f7f-9a2b-8b8c-f408-49b9b5982eb7@suse.com>
Date: Thu, 15 Jun 2023 11:12:07 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: linux-mm@...ck.org, dave.hansen@...el.com,
kirill.shutemov@...ux.intel.com, tony.luck@...el.com,
peterz@...radead.org, tglx@...utronix.de, seanjc@...gle.com,
pbonzini@...hat.com, david@...hat.com, dan.j.williams@...el.com,
rafael.j.wysocki@...el.com, ying.huang@...el.com,
reinette.chatre@...el.com, len.brown@...el.com, ak@...ux.intel.com,
isaku.yamahata@...el.com, chao.gao@...el.com,
sathyanarayanan.kuppuswamy@...ux.intel.com, bagasdotme@...il.com,
sagis@...gle.com, imammedo@...hat.com
Subject: Re: [PATCH v11 15/20] x86/virt/tdx: Configure global KeyID on all
packages
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>
Powered by blists - more mailing lists