[PATCH v2 2/2] KVM: x86/tdp_mmu: Trigger the callback only when an interesting change

[Isaku Yamahata]

Call the set_external_spte() hook only when the present bit is changed from
non-present to present.

Observation of the issue
------------------------
An issue was reported on an out-of-tree kernel with an older version of TDX
support, but that uses similar code to the current existing mirror/external
MMU support.  It can't be triggered without future patches.  In the out of
tree kernel it appears as a KVM_BUG_ON() getting hit in TDX code when
set_external_spte() was called on a PTE that was already present.
Investigation turned up that:

 1. It was due to the AD state machine trying to update the dirty bit
 2. A problem in the currently queued mirror/external PTE, but that can't be
    hit until more TDX support is added.

In more detail, the spotted issue was caused by two vcpus simultaneously
faulting for same GFN:

vcpu0                                   vcpu1
-----                                   -----
ept violation for GFN                   ept violation for GFN
encounter !present mirror PTE
set_external_spte()
re-enter TD, and accept GFN
                                        encounter present mirror PTE
                                        trigger AD state machine transition
                                        see old/new PTEs are different
                                        call set_external_spte() again

The TDX external TDP backend couldn't handle the second call to
set_external_spte().  In recent TDX development branches, the
TDH.MEM.PAGE.AUG() SEAMCALL() hook would fail unexpectedly with
TDX_EPT_ENTRY_STATE_INCORRECT + SPTE PENDING or PRESENT.  However, the
callback assumes that it's called only when non-present => present leaf
SPTE change.  It shows as a triggering a KVM_BUG_ON() with a stacktrace
like:

  WARNING: CPU: 4 PID: 3700 at tdx_mem_page_aug+0x16d/0x560 [kvm_intel]
  IP: 0010:tdx_mem_page_aug+0x16d/0x560 [kvm_intel]

  Call Trace:
  set_external_spte_present+0x244/0x7e0
  tdp_mmu_map_handle_target_level+0x460/0xd60
  kvm_tdp_mmu_map+0x9c6/0xdc0
  kvm_tdp_mmu_page_fault+0x32b/0x3e0
  kvm_mmu_do_page_fault+0x4e5/0x6a0
  kvm_mmu_page_fault+0x1a0/0x5e0
  vcpu_enter_guest+0x1f5f/0x3900
  vcpu_run+0x133/0xb60
  kvm_arch_vcpu_ioctl_run+0x8ef/0x1650
  kvm_vcpu_ioctl+0x4f9/0xb70
  __x64_sys_ioctl+0x132/0x1a0
  do_syscall_64+0xc1/0x1d0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f

Design of MMU mirror/external
-----------------------------
In current mirror/external MMU support, the set_external_spte() allows
external PTEs to be set as present, but doesn't allow configuration of any
other PTE bits.  remove_external_spte() allows external PTEs to be zapped.

Based on the fact that external PTE callbacks are exposed to essentially
configure two PTE states (present or not present), and that mirror PTEs
record whether the external PTE is present or not, future patches were
planned to introduce a TDX backend for mirror/external page tables to not
expect set_external_spte() calls on already present external PTEs.  To
accommodate this future code, steps were taken in the mirror/external
support in the core MMU to prevent permission bits changing on such PTEs.

However, the AD state machine scenario was missed.  When the multiple vCPUs
fault in the same GFN simultaneously, the hook is called multiple times
with some bits changed while both old/new SPTEs have the present bits.  The
leaf SPTE is a complex state machine in general, and the value can change
with software available bits and hardware bits.

Solution
--------
There are several options to address this:
- Tame the SPTE state machine so that SPTE change won't happen for mirrored
  page table.
  PRO: It's conceptually natural to disable D bit support because mirrored
       page table doesn't support AD bits.
  CON: Not only D bit but also other bits can be modified.
       setting strict kvm_page_table.role.ad_disabled = true doesn't work.
       It requires to change the SPTE more deeply.

- Add a check to the TDP MMU so that it won't call the hook unnecessarily
  PRO: Direct fix that shares set_external_spte() expectations in core
       MMU code.
  CON: Hard code in the TDP MMU when the hook is needed.

- Pass old and new SPTE values to the hooks, add a check to the backend
  PRO: The backend can determine whether the callback is needed.
  CON: The KVM MMU implementation details leak to the backend because
       The SPTE encoding is specific to the KVM MMU implementation.
       And it's subject to change in the future.
       For example, is_shadow_present_pte() needs to be exported from the
       KVM MMU to the backend.

The first choice is out because it doesn't easily handle the problem.
Choose the second option over the third choice because the current
interesting change is only non-present => present because we don't support
dirty page tracking by write protect and large page yet by TDX KVM for now.
(TDX supports them.) They're future work for TDX KVM.

Care only about the leaf SPTE because non-leaf doesn't have a state
machine.

Reported-by: Sagi Shahar <sagis@xxxxxxxxxx>
Fixes: b6bcd88ad43a ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
[log heavily edited by Rick]
Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
---
v2:
- Removed KVM_BUG_ON() into another patch (Sean)
- Removed pr_debug() (Sean)

v1:
https://lore.kernel.org/kvm/6eecc450d0326c9bedfbb34096a0279410923c8d.1726182754.git.isaku.yamahata@xxxxxxxxx/
---
 arch/x86/kvm/mmu/tdp_mmu.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 1da3df517522..c7f251549973 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -503,8 +503,6 @@ static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
 	kvm_pfn_t new_pfn = spte_to_pfn(new_spte);
 	int ret = 0;
 
-	KVM_BUG_ON(was_present, kvm);
-
 	lockdep_assert_held(&kvm->mmu_lock);
 	/*
 	 * We need to lock out other updates to the SPTE until the external
@@ -519,10 +517,16 @@ static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
 	 * external page table, or leaf.
 	 */
 	if (is_leaf) {
-		ret = static_call(kvm_x86_set_external_spte)(kvm, gfn, level, new_pfn);
+		/*
+		 * SPTE is state machine with software available bits used.
+		 * Check if the change is interesting to the backend.
+		 */
+		if (!was_present)
+			ret = static_call(kvm_x86_set_external_spte)(kvm, gfn, level, new_pfn);
 	} else {
 		void *external_spt = get_external_spt(gfn, new_spte, level);
 
+		KVM_BUG_ON(was_present, kvm);
 		KVM_BUG_ON(!external_spt, kvm);
 		ret = static_call(kvm_x86_link_external_spt)(kvm, gfn, level, external_spt);
 	}
-- 
2.45.2