Re: [PATCH v6 7/7] KVM: TDX: Explicitly do WBINVD when no more TDX SEAMCALLs

[Huang, Kai]

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