[PATCH v2 0/2] Drop unneeded cache flushing

[PATCH v2 0/2] Drop unneeded cache flushing

[Rick Edgecombe]

Hi TDX developers,

Can I get another round of review before we ask Dave and Sean to look at
it? I'd like to pick up a few more RB tags.

-- Summary --

In the early days of TDX it was assumed the HW would need some cache 
flushing help around transitioning memory between KeyIDs, and so TDX arch 
was designed to specify how the VMM needed to help the module perform 
this. But it turned out the HW was able to handle these issues without SW 
help. For some reason the TDX arch around performing the flushes remained. 
Now the TDX arch is getting fixed and Linux can remove the code that was 
helping perform this unneeded cache flushing. The changes will appear in 
a future documentation update.

As some more context, while this is nice cleanup as is, merging these 
patches will let us drop 2 patches out of the TDX huge pages series. Then 
that series doesn't need to modify these flushes to work with TDX huge 
pages. It also addresses Kiryl's comment about the cache flushing in 
tdh_mem_page_aug() not respecting the 'level' arg, or warning that non-4KB 
sizes are not supported.[0]

-- Changes since v1 --

Yilun asked if we could also remove the WBINVD calls around kexec and TDX
setup error paths. It seems the answer is likely yes, but not confirmed
enough for me yet. So I left that change for the future. Besides, it
doesn't have the same TDX huge page intersection.

Yilun's comment about cleaning up the TDR reclaim after the changes in 
patch 2 was a bit bigger that I expected. I really debated whether to 
split that out. But it wasn't very feasible to do it as a prep patch, so I 
left it as one big bigger. Really debated doing that, or an follow up 
cleanup patch. Opinions requested.

Based on v7.3-rc7.

Thanks,

Rick


Rick Edgecombe (2):
  x86/virt/tdx: Drop tdx_clflush_*()
  x86/virt/tdx: Drop tdh_phymem_page_wbinvd_*()

 arch/x86/include/asm/tdx.h  |  2 --
 arch/x86/kvm/vmx/tdx.c      | 40 +++++-------------------------
 arch/x86/virt/vmx/tdx/tdx.c | 49 -------------------------------------
 arch/x86/virt/vmx/tdx/tdx.h |  1 -
 4 files changed, 6 insertions(+), 86 deletions(-)

-- 
2.55.0

[PATCH v2 1/2] x86/virt/tdx: Drop tdx_clflush_*()

[Rick Edgecombe]

The TDX module exposes a bit (CLFLUSH_BEFORE_ALLOC) to say whether it
requires certain types of memory to be flushed from the cache before being
handed to the TDX module.

During the initial TDX upstreaming, it was decided it would be simplest
to just always flush rather than checking for the bit and having
conditional logic. So based on the TDX docs:
  When allocating a memory page to be used as TD private memory or TD
  control structure page, the host VMM is required to ensure that none of
  the cache lines associated with the page is in a MODIFIED state.

...currently the kernel flushes before handing over private memory and TD
control structure pages.

The CLFLUSH_BEFORE_ALLOC bit has never been set by any TDX module, so this
flushing has yet to be needed in practice. Going forward, the TDX docs will
be updated to clarify that actually CLFLUSH_BEFORE_ALLOC will *never* be
set in any future TDX module either.

So make code even simpler by just deleting all the CLFLUSH_BEFORE_ALLOC
handling.

AI was used to review the change.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
---
v2:
 - Log verbiage (Yan)
 - Verbiage around "in practice" (Tony)
 - Clarify what flows require CLFLUSH_BEFORE_ALLOC (Nikolay)
---
 arch/x86/virt/vmx/tdx/tdx.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 1b9ff749dd8e0..8f9d3588f200f 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1619,22 +1619,6 @@ static inline u64 tdx_tdr_pa(struct tdx_td *td)
 	return page_to_phys(td->tdr_page);
 }
 
-/*
- * The TDX module exposes a CLFLUSH_BEFORE_ALLOC bit to specify whether
- * a CLFLUSH of pages is required before handing them to the TDX module.
- * Be conservative and make the code simpler by doing the CLFLUSH
- * unconditionally.
- */
-static void tdx_clflush_page(struct page *page)
-{
-	clflush_cache_range(page_to_virt(page), PAGE_SIZE);
-}
-
-static void tdx_clflush_pfn(kvm_pfn_t pfn)
-{
-	clflush_cache_range(__va(PFN_PHYS(pfn)), PAGE_SIZE);
-}
-
 static int pg_level_to_tdx_sept_level(enum pg_level level)
 {
 	WARN_ON_ONCE(level == PG_LEVEL_NONE);
@@ -1656,7 +1640,6 @@ u64 tdh_mng_addcx(struct tdx_td *td, struct page *tdcs_page)
 		.rdx = tdx_tdr_pa(td),
 	};
 
-	tdx_clflush_page(tdcs_page);
 	return seamcall(TDH_MNG_ADDCX, &args);
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_mng_addcx);
@@ -1672,7 +1655,6 @@ u64 tdh_mem_page_add(struct tdx_td *td, u64 gpa, kvm_pfn_t pfn, struct page *sou
 	};
 	u64 ret;
 
-	tdx_clflush_pfn(pfn);
 	ret = seamcall_ret(TDH_MEM_PAGE_ADD, &args);
 
 	*ext_err1 = args.rcx;
@@ -1692,7 +1674,6 @@ u64 tdh_mem_sept_add(struct tdx_td *td, u64 gpa, enum pg_level level,
 	};
 	u64 ret;
 
-	tdx_clflush_page(page);
 	ret = seamcall_ret(TDH_MEM_SEPT_ADD, &args);
 
 	*ext_err1 = args.rcx;
@@ -1709,7 +1690,6 @@ u64 tdh_vp_addcx(struct tdx_vp *vp, struct page *tdcx_page)
 		.rdx = vp->tdvpr_pa,
 	};
 
-	tdx_clflush_page(tdcx_page);
 	return seamcall(TDH_VP_ADDCX, &args);
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_vp_addcx);
@@ -1724,7 +1704,6 @@ u64 tdh_mem_page_aug(struct tdx_td *td, u64 gpa, enum pg_level level,
 	};
 	u64 ret;
 
-	tdx_clflush_pfn(pfn);
 	ret = seamcall_ret(TDH_MEM_PAGE_AUG, &args);
 
 	*ext_err1 = args.rcx;
@@ -1769,7 +1748,6 @@ u64 tdh_mng_create(struct tdx_td *td, u16 hkid)
 		.rdx = hkid,
 	};
 
-	tdx_clflush_page(td->tdr_page);
 	return seamcall(TDH_MNG_CREATE, &args);
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_mng_create);
@@ -1781,7 +1759,6 @@ u64 tdh_vp_create(struct tdx_td *td, struct tdx_vp *vp)
 		.rdx = tdx_tdr_pa(td),
 	};
 
-	tdx_clflush_page(vp->tdvpr_page);
 	return seamcall(TDH_VP_CREATE, &args);
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_vp_create);
-- 
2.55.0

[PATCH v2 2/2] x86/virt/tdx: Drop tdh_phymem_page_wbinvd_*()

[Rick Edgecombe]

In the early days of TDX it was expected that pages being reclaimed from
the TDX module would need any cacheline from their private KeyID alias
flushed. The TDX docs describe doing this in a couple narrow cases, for
example reclaiming the TDR memory or guest private memory.

Similar to how the CLFLUSH_BEFORE_ALLOC features0 bit was never actually
set in any TDX module, the flush of the private KeyID cachelines turned
out to not be needed by the final TDX HW either. However, the TDX docs
were never updated to reflect that.

CLFLUSH_BEFORE_ALLOC handles flushing when giving memory to the TDX
module, where the alias that needs flushing is the default, normal KeyID
0. This was done with a clflush of the kernel's KeyID 0 mapping of the
memory. For the reclaim path, instead it is the TDX KeyID aliases that
need to be flushed. Since virtual mappings for these aliases are
restricted to the TDX module, the module is the only thing that can map
the page to do the clflush. The TDH.PHYMEM.PAGE.WBINVD SEAMCALL leaf was
created to facilitate this.

The docs are being updated to clarify that calling TDH.PHYMEM.PAGE.WBINVD
is not needed. So drop the kernel code that calls this SEAMCALL and the
helpers that facilitate it. With the removal of
tdh_phymem_page_wbinvd_tdr(), tdx_global_keyid no longer needs to be kept
around as a global variable. It just needs to be selected and provided to
the TDX module during initial setup.

However, don't churn the code to remove the global variable because
it fits neatly with the other KeyID partitioning global data. It is also
foreseeable that this would be used in the future for other SEAMCALLs that
require passing the global KeyID.

KVM previously needed to call different types of tdh_phymem_page_wbinvd_*()
helpers for different types of reclaim. So it had a stack of helpers and
some open coded logic to use them in the rarer TDR reclaim case. Now that
the wbinvd is gone, the steps for each operation are logically equivalent.
So collapse the helpers and just call tdx_reclaim_control_page() from the
TDR reclaim path.

This unification brings TDR reclaim in line with the behavior of the other
reclaim paths, by handling TDH.PHYMEM.PAGE.RECLAIM failure by leaking the
page and setting the TDR page pointer to NULL. This results in reclaim not
being tried a second time, which is reasonable because the first reclaim
failing suggests a problem with the TDX module.

AI was used to review the change.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
v2:
 - Clean up open coding of TDR reclaim now that it doesn't need to be.
   (Yilun)
 - Logs stuff (Yilun, Yan, Chao)
 - [omit Yan RB due to code change]
---
 arch/x86/include/asm/tdx.h  |  2 --
 arch/x86/kvm/vmx/tdx.c      | 40 ++++++-------------------------------
 arch/x86/virt/vmx/tdx/tdx.c | 26 ------------------------
 arch/x86/virt/vmx/tdx/tdx.h |  1 -
 4 files changed, 6 insertions(+), 63 deletions(-)

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 89e97d5761d89..39e3c1f8a48e1 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -174,8 +174,6 @@ u64 tdh_phymem_page_reclaim(struct page *page, u64 *tdx_pt, u64 *tdx_owner, u64
 u64 tdh_mem_track(struct tdx_td *tdr);
 u64 tdh_mem_page_remove(struct tdx_td *td, u64 gpa, enum pg_level level, u64 *ext_err1, u64 *ext_err2);
 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, kvm_pfn_t pfn);
 #else
 static inline void tdx_init(void) { }
 static inline u32 tdx_get_nr_guest_keyids(void) { return 0; }
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 00850a3a77d66..ed1db8023c6ec 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -320,7 +320,7 @@ static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu)
 })
 
 /* TDH.PHYMEM.PAGE.RECLAIM is allowed only when destroying the TD. */
-static int __tdx_reclaim_page(struct page *page)
+static int tdx_reclaim_page(struct page *page)
 {
 	u64 err, rcx, rdx, r8;
 
@@ -334,24 +334,15 @@ static int __tdx_reclaim_page(struct page *page)
 	if (TDX_BUG_ON_3(err, TDH_PHYMEM_PAGE_RECLAIM, rcx, rdx, r8, NULL))
 		return -EIO;
 
+	tdx_quirk_reset_paddr(page_to_phys(page), PAGE_SIZE);
+
 	return 0;
 }
 
-static int tdx_reclaim_page(struct page *page)
-{
-	int r;
-
-	r = __tdx_reclaim_page(page);
-	if (!r)
-		tdx_quirk_reset_paddr(page_to_phys(page), PAGE_SIZE);
-	return r;
-}
-
 
 /*
- * Reclaim the TD control page(s) which are crypto-protected by TDX guest's
- * private KeyID.  Assume the cache associated with the TDX private KeyID has
- * been flushed.
+ * Reclaim the TD control page(s) which are crypto-protected by private
+ * KeyIDs.
  */
 static void tdx_reclaim_control_page(struct page *ctrl_page)
 {
@@ -550,7 +541,6 @@ void tdx_mmu_release_hkid(struct kvm *kvm)
 static void tdx_reclaim_td_control_pages(struct kvm *kvm)
 {
 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
-	u64 err;
 	int i;
 
 	/*
@@ -575,21 +565,7 @@ static void tdx_reclaim_td_control_pages(struct kvm *kvm)
 	if (!kvm_tdx->td.tdr_page)
 		return;
 
-	if (__tdx_reclaim_page(kvm_tdx->td.tdr_page))
-		return;
-
-	/*
-	 * Use a SEAMCALL to ask the TDX module to flush the cache based on the
-	 * KeyID. TDX module may access TDR while operating on TD (Especially
-	 * when it is reclaiming TDCS).
-	 */
-	err = tdh_phymem_page_wbinvd_tdr(&kvm_tdx->td);
-	if (TDX_BUG_ON(err, TDH_PHYMEM_PAGE_WBINVD, kvm))
-		return;
-
-	tdx_quirk_reset_paddr(page_to_phys(kvm_tdx->td.tdr_page), PAGE_SIZE);
-
-	__free_page(kvm_tdx->td.tdr_page);
+	tdx_reclaim_control_page(kvm_tdx->td.tdr_page);
 	kvm_tdx->td.tdr_page = NULL;
 }
 
@@ -1814,10 +1790,6 @@ static int tdx_sept_remove_leaf_spte(struct kvm *kvm, gfn_t gfn,
 	if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_REMOVE, entry, level_state, kvm))
 		return -EIO;
 
-	err = tdh_phymem_page_wbinvd_hkid((u16)kvm_tdx->hkid, pfn);
-	if (TDX_BUG_ON(err, TDH_PHYMEM_PAGE_WBINVD, kvm))
-		return -EIO;
-
 	tdx_quirk_reset_paddr(PFN_PHYS(pfn), PAGE_SIZE);
 	return 0;
 }
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 8f9d3588f200f..21f7fce38aa46 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1956,32 +1956,6 @@ u64 tdh_phymem_cache_wb(bool resume)
 }
 EXPORT_SYMBOL_FOR_KVM(tdh_phymem_cache_wb);
 
-static inline u64 mk_keyed_paddr(u16 hkid, kvm_pfn_t pfn)
-{
-	/* KeyID bits are just above the physical address bits. */
-	return PFN_PHYS(pfn) | ((u64)hkid << boot_cpu_data.x86_phys_bits);
-}
-
-u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td)
-{
-	struct tdx_module_args args = {};
-
-	args.rcx = mk_keyed_paddr(tdx_global_keyid, page_to_pfn(td->tdr_page));
-
-	return seamcall(TDH_PHYMEM_PAGE_WBINVD, &args);
-}
-EXPORT_SYMBOL_FOR_KVM(tdh_phymem_page_wbinvd_tdr);
-
-u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, kvm_pfn_t pfn)
-{
-	struct tdx_module_args args = {};
-
-	args.rcx = mk_keyed_paddr(hkid, pfn);
-
-	return seamcall(TDH_PHYMEM_PAGE_WBINVD, &args);
-}
-EXPORT_SYMBOL_FOR_KVM(tdh_phymem_page_wbinvd_hkid);
-
 void tdx_sys_disable(void)
 {
 	struct tdx_module_args args = {};
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index bdfd0e1e337ac..3289be59d14fc 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -43,7 +43,6 @@
 #define TDH_SYS_TDMR_INIT		36
 #define TDH_MEM_TRACK			38
 #define TDH_PHYMEM_CACHE_WB		40
-#define TDH_PHYMEM_PAGE_WBINVD		41
 #define TDH_VP_WR			43
 #define TDH_SYS_CONFIG			45
 #define TDH_SYS_SHUTDOWN		52
-- 
2.55.0