Re: [PATCH v4 07/17] KVM: TDX: Add core support for splitting/demoting 2MB S-EPT mappings to 4KB

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Mon, 2026-09-28 at 17:10 +0800, Yan Zhao wrote:
> Add support for splitting, a.k.a. demoting, a 2MB S-EPT leaf mapping to 512
> smaller 4KB leaf mappings.  As per the TDX module rules, first invoke
> MEM.RANGE.BLOCK to put the huge S-EPT leaf entry into a splittable state,
> then do MEM.TRACK and kick all vCPUs outside of guest mode to flush TLBs,
> and finally do MEM.PAGE.DEMOTE to demote/split the huge S-EPT leaf mapping.
> 
> Assert the mmu_lock is held for write, as the BLOCK => TRACK => DEMOTE
> sequence needs to be "atomic" to guarantee success (and because mmu_lock
> must be held for write to use tdh_do_no_vcpus()).
> 
> Note, even with kvm->mmu_lock held for write, tdh_mem_page_demote() may
> contend with tdh_vp_enter() and potentially with the guest's S-EPT entry
> operations.  Therefore, wrap the call with tdh_do_no_vcpus() to kick other
> vCPUs out of the guest and prevent tdh_vp_enter() to ensure success.
> 
> Invoke tdx_pamt_get() before invoking tdh_mem_page_demote() so that DPAMT
> pages for the new S-EPT page table page are installed before the DEMOTE
> SEAMCALL when DPAMT is enabled. DPAMT pages for the guest memory must be
> installed inside the DEMOTE SEAMCALL, since it is impossible to do so
> before a successful demotion.
> 
> Instead of allocating and freeing DPAMT pages for guest pages on the KVM
> side, pass pamt_cache to tdh_mem_page_demote() and let it draw DPAMT pages
> from pamt_cache before the DEMOTE SEAMCALL. This prevents KVM from having
> to manage DPAMT pages directly via alloc_pamt_array() and
> free_pamt_array(), or having knowledge of DPAMT-specific details such as
> TDX_DPAMT_ENTRY_PAGE_CNT.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>

The SOB seems wrong. You are the the listed author, but these don't have a Co-
developed-by tag.

> [sean: wire up via op set_external_spte(), merge in DPAMT-related code,
>  massage changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
> v4:
> - Hooked tdx_sept_split_leaf_spte() in x86 op set_external_spte() instead
>   of in x86 op split_external_spte() which was no longer introduced in v4.
>   (Sean).
> - Renamed tdx_sept_split_private_spte() --> tdx_sept_split_leaf_spte().
> - Merged in DPAMT-related code (i.e., passing to_tdx(vcpu)->pamt_cache to
>   tdh_mem_page_demote(). (Sean).
> - Assert new_spte is non-leaf. (Yan)
> 
> v3:
> - Rebased on top of Sean's cleanup series.
> - Call out UNBLOCK is not required after DEMOTE. (Kai)
> - tdx_sept_split_private_spt() --> tdx_sept_split_private_spte().
> 
> RFC v2:
> - Split out the code to handle the error TDX_INTERRUPTED_RESTARTABLE.
> - Rebased to 6.16.0-rc6 (the way of defining TDX hook changes).
> 
> RFC v1:
> - Split patch for exclusive mmu_lock only,
> - Invoke tdx_sept_zap_private_spte() and tdx_track() for splitting.
> - Handled busy error of tdh_mem_page_demote() by kicking off vCPUs.
> ---
>  arch/x86/kvm/vmx/tdx.c | 69 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 3dcddf1b48c5..3186c4808cae 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1873,11 +1873,74 @@ static int tdx_sept_remove_leaf_spte(struct kvm *kvm, gfn_t gfn,
>  	return 0;
>  }
>  
> +/*
> + * Split a huge mapping into smaller mappings at a lower level.  Currently only
> + * supports splitting 2MB mappings (KVM doesn't yet support 1GB mappings for TDX
> + * guests).
> + *
> + * Invoke "BLOCK + TRACK + kick off vCPUs (inside tdx_track())" since the TDX
> + * module does not yet support the NON-BLOCKING-RESIZE feature for DEMOTE.
> + *
> + * No UNBLOCK is needed after a successful DEMOTE.
> + *
> + * Under write mmu_lock, kick off all vCPUs and disallow vCPUs from entering to
> + * ensure DEMOTE will succeed on the second invocation if the first invocation
> + * returns BUSY.

How about putting these in the function instead of up here?

> + */
> +static int tdx_sept_split_leaf_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
> +				    u64 new_spte, enum pg_level level)
> +{
> +	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> +	gpa_t gpa = gfn_to_gpa(gfn);
> +	u64 err, entry, level_state;
> +	struct page *sept_pt;
> +	int r;
> +
> +	lockdep_assert_held_write(&kvm->mmu_lock);
> +
> +	if (KVM_BUG_ON(!is_last_spte(old_spte, level) || is_last_spte(new_spte, level), kvm))
> +		return -EIO;
> +
> +	sept_pt = tdx_spte_to_sept_pt(kvm, gfn, new_spte, level);
> +	if (!sept_pt)
> +		return -EIO;
> +
> +	if (KVM_BUG_ON(!vcpu || vcpu->kvm != kvm, kvm))
> +		return -EIO;
> +
> +	r = tdx_pamt_get(page_to_pfn(sept_pt), PG_LEVEL_4K, &to_tdx(vcpu)->pamt_cache);
> +	if (KVM_BUG_ON(r, kvm))
> +		return r;
> +
> +	err = tdh_do_no_vcpus(tdh_mem_range_block, kvm, &kvm_tdx->td, gpa,
> +			      level, &entry, &level_state);
> +	if (TDX_BUG_ON_2(err, TDH_MEM_RANGE_BLOCK, entry, level_state, kvm)) {
> +		r = -EIO;
> +		goto err;
> +	}
> +
> +	tdx_track(kvm);
> +	err = tdh_do_no_vcpus(tdh_mem_page_demote, kvm, &kvm_tdx->td, gpa,
> +			      level, spte_to_pfn(old_spte), sept_pt,
> +			      &to_tdx(vcpu)->pamt_cache, &entry, &level_state);
> +	if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_DEMOTE, entry, level_state, kvm)) {
> +		r = -EIO;

Not trying to rollback with an unlock in this case seems like the right call,
but can we have a comment justification?

> +		goto err;
> +	}
> +
> +	return 0;
> +err:
> +	tdx_pamt_put(page_to_pfn(sept_pt), PG_LEVEL_4K);
> +	return r;
> +}
> +
>  /*
>   * Handle changes for
>   * (1) leaf SPTEs from non-present to present
>   * (2) non-leaf SPTEs from non-present to present
>   * (3) leaf SPTEs from present to non-present
> + * (4) present leaf SPTEs to present non-leaf SPTEs (splitting)
>   *
>   * - (1) and (2) must be under shared mmu_lock. If (1) and (2) are under
>   *   exclusive mmu_lock (currently impossible), contention errors may lead to
> @@ -1888,13 +1951,17 @@ static int tdx_sept_remove_leaf_spte(struct kvm *kvm, gfn_t gfn,
>   *   (currently impossible), warnings will be generated due to
>   *   lockdep_assert_held_write() or TDX_BUG_ON() caused by concurrent BLOCK,
>   *   TRACK, REMOVE.
> - * - Promotion/demotion is not yet supported.
> + * - (4) must be under write mmu_lock currently.

Currently?

> + * - Promotion is not yet supported.

And we are not trying to support it, right?

Can we not make this imply some roadmap of enhancements that may never happen
for a long time if ever?

>   */
>  static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
>  				     u64 new_spte, enum pg_level level)
>  {
>  	lockdep_assert_held(&kvm->mmu_lock);
>  
> +	if (is_shadow_present_pte(old_spte) && is_shadow_present_pte(new_spte))
> +		return tdx_sept_split_leaf_spte(kvm, gfn, old_spte, new_spte, level);
> +
>  	if (is_shadow_present_pte(old_spte))
>  		return tdx_sept_remove_leaf_spte(kvm, gfn, level, old_spte);
>  

Except for those small comments, overall looks good to me.