| 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: <c736d2040f5452585e670819621d3bae5417fff4.camel@intel.com>
Date: Wed, 20 Aug 2025 11:22:12 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "pbonzini@...hat.com" <pbonzini@...hat.com>, "seanjc@...gle.com"
<seanjc@...gle.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "ashish.kalra@....com"
<ashish.kalra@....com>, "Hansen, Dave" <dave.hansen@...el.com>,
"thomas.lendacky@....com" <thomas.lendacky@....com>, "kas@...nel.org"
<kas@...nel.org>, "Chatre, Reinette" <reinette.chatre@...el.com>,
"dwmw@...zon.co.uk" <dwmw@...zon.co.uk>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "mingo@...hat.com" <mingo@...hat.com>,
"Yamahata, Isaku" <isaku.yamahata@...el.com>, "nik.borisov@...e.com"
<nik.borisov@...e.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
"hpa@...or.com" <hpa@...or.com>, "peterz@...radead.org"
<peterz@...radead.org>, "sagis@...gle.com" <sagis@...gle.com>, "Chen, Farrah"
<farrah.chen@...el.com>, "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
"bp@...en8.de" <bp@...en8.de>, "binbin.wu@...ux.intel.com"
<binbin.wu@...ux.intel.com>, "Gao, Chao" <chao.gao@...el.com>, "Williams, Dan
J" <dan.j.williams@...el.com>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v6 7/7] KVM: TDX: Explicitly do WBINVD when no more TDX
SEAMCALLs
On Wed, 2025-08-20 at 11:51 +0200, Paolo Bonzini wrote:
> On 8/19/25 23:53, Huang, Kai wrote:
> > On Tue, 2025-08-19 at 12:31 +0200, Paolo Bonzini wrote:
> > > 2) ... but anyway, KVM is the wrong place to do the test. If anything,
> > > since we need a v7 to change the unnecessary stub, you could move that
> > > stub under #ifndef CONFIG_KEXEC_CORE and rename the function to
> > > tdx_cpu_flush_cache_for_kexec().
> >
> > Agreed on renaming to tdx_cpu_flush_cache_for_kexec().
> >
> > But with the "for_kexec()" part in the function name, it already implies
> > it is related to kexec, and I kinda think there's no need to test
> > IS_ENABLED(CONFIG_KEXEC_CORE) anymore.
> >
> > One of the main purpose of this series is to unblock TDX_HOST and KEXEC in
> > the Kconfig, since otherwise I've been told distros will simply choose to
> > disable TDX_HOST in the Kconfig. So in reality, I suppose they will be on
> > together probably in like 95% cases, if not 100%.
> >
> > If we want to test CONFIG_KEXEC_CORE in tdx_cpu_flush_cache_for_kexec(),
> > then it would be a little bit weird that why we don't test it in other
> > places, e.g., when setting up the boolean. Testing it in all places would
> > make the code unnecessarily long and harder to read.
>
> No I don't mean testing it there, but just making
> tdx_cpu_flush_cache_for_kexec() a stub when CONFIG_KEXEC_CORE is
> undefined:
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index e9a213582f03..913199b1954b 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -217,7 +217,6 @@ u64 tdh_mem_page_remove(struct tdx_td *td, u64 gpa, u64 level, u64 *ext_err1, u6
> u64 tdh_phymem_cache_wb(bool resume);
> u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td);
> u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, struct page *page);
> -void tdx_cpu_flush_cache(void);
> #else
> static inline void tdx_init(void) { }
> static inline int tdx_cpu_enable(void) { return -ENODEV; }
> @@ -225,8 +224,13 @@ static inline int tdx_enable(void) { return -ENODEV; }
> static inline u32 tdx_get_nr_guest_keyids(void) { return 0; }
> static inline const char *tdx_dump_mce_info(struct mce *m) { return NULL; }
> static inline const struct tdx_sys_info *tdx_get_sysinfo(void) { return NULL; }
> -static inline void tdx_cpu_flush_cache(void) { }
> #endif /* CONFIG_INTEL_TDX_HOST */
>
> +#ifdef CONFIG_KEXEC_CORE
> +void tdx_cpu_flush_cache_for_kexec(void);
> +#else
> +static inline void tdx_cpu_flush_cache_for_kexec(void) { }
> +#endif
> +
I think one minor issue here is, when CONFIG_INTEL_TDX_HOST is off but
CONFIG_KEXEC_CORE is on, there will be no implementation of
tdx_cpu_flush_cache_for_kexec(). This won't result in build error,
though, because when TDX_HOST is off, KVM_INTEL_TDX will be off too, i.e.,
there won't be any caller of tdx_cpu_flush_cache_for_kexec().
But this still doesn't look nice?
Btw, the above will provide the stub function when both KEXEC_CORE and
TDX_HOST is off, which seems to be a step back too?
:-)
To me, it's more straightforward to just rename it to
tdx_cpu_flush_cache_for_kexec() and remove the stub:
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index e9a213582f03..5e37ae1a40e8 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -217,7 +217,7 @@ u64 tdh_mem_page_remove(struct tdx_td *td, u64 gpa,
u64 level, u64 *ext_err1, u6
u64 tdh_phymem_cache_wb(bool resume);
u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td);
u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, struct page *page);
-void tdx_cpu_flush_cache(void);
+void tdx_cpu_flush_cache_for_kexec(void);
#else
static inline void tdx_init(void) { }
static inline int tdx_cpu_enable(void) { return -ENODEV; }
@@ -225,7 +225,6 @@ static inline int tdx_enable(void) { return -ENODEV;
}
static inline u32 tdx_get_nr_guest_keyids(void) { return 0; }
static inline const char *tdx_dump_mce_info(struct mce *m) { return NULL;
}
static inline const struct tdx_sys_info *tdx_get_sysinfo(void) { return
NULL; }
-static inline void tdx_cpu_flush_cache(void) { }
#endif /* CONFIG_INTEL_TDX_HOST */
#endif /* !__ASSEMBLER__ */
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 1bc6f52e0cd7..382792e31f32 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -453,7 +453,7 @@ void tdx_disable_virtualization_cpu(void)
* remote CPUs to stop them. Doing WBINVD in stop_this_cpu()
* could potentially increase the possibility of the "race".
*/
- tdx_cpu_flush_cache();
+ tdx_cpu_flush_cache_for_kexec();
}
#define TDX_SEAMCALL_RETRIES 10000
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index c26e2e07ff6b..cd2a36dbbfc5 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1871,7 +1871,7 @@ u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, struct
page *page)
}
EXPORT_SYMBOL_GPL(tdh_phymem_page_wbinvd_hkid);
-void tdx_cpu_flush_cache(void)
+void tdx_cpu_flush_cache_for_kexec(void)
{
lockdep_assert_preemption_disabled();
@@ -1881,4 +1881,4 @@ void tdx_cpu_flush_cache(void)
wbinvd();
this_cpu_write(cache_state_incoherent, false);
}
-EXPORT_SYMBOL_GPL(tdx_cpu_flush_cache);
+EXPORT_SYMBOL_GPL(tdx_cpu_flush_cache_for_kexec);
Powered by blists - more mailing lists