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);
