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

[Yan Zhao <yan.y.zhao@intel.com>]

On Thu, Oct 08, 2026 at 05:52:05AM +0800, Edgecombe, Rick P wrote:
> 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.
The current implementation has been substantially reworked from the original one
https://lore.kernel.org/all/b387dcfc499d2f17e448cd4574115d1fd2437921.1708933624.git.isaku.yamahata@intel.com/

Should I use Originally-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?
Sounds good.

> > + */
> > +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?
Makes sense.

What about:
"Something severe went wrong. The VM is going to die.
 No need to unblock, which could potentially fail as well." 

> > +		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?
Ok. Let me correct them to:

- (4) must be under write mmu_lock.
- Promotion is not supported

> >   */
> >  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.
Thanks for the detailed review!