Re: [PATCH v4 08/17] KVM: TDX: Adjust the topup count of DPAMT page pairs for splitting S-EPT

[Yan Zhao]

On Tue, Oct 13, 2026 at 12:36:13AM +0800, Edgecombe, Rick P wrote:
> On Sat, 2026-10-10 at 13:13 +0800, Yan Zhao wrote:
> > > For fault:
> > > topup(true, true, PT64_ROOT_MAX_LEVEL - 1)
> > > 
> > > For split:
> > > topup(false, false, 1)
> > Hmm, having the caller perform the -1 calculation gives me some hesitation.
> > The meaning of nr_extra_spts is not straightforward, though it seems to mean
> > the number of spts excluding the root.
> 
> It means "extra" beyond the root. How about:
> 
> int topup(bool root_spt, int nr_extra_spts, bool private_page)
> 
> The idea is to tell external infra what the fault handler might do. It might add
> a root, it might add a private leaf and it may need extra spts beyond the root.
> These are all concepts known to mirror EPT and also has all the info needed for
> the calculation on the TDX side, and also doesn't assume anything about TDX
> implementation from the mirror side. There is extra info passed, that is not
> needed. But the gain is that the code on both sides and calculation are self
> documenting.
It seems strange to me to have nr_extra_spts if root_spt is false, in which case
nr_extra_spts are all the spts the caller wants to topup for, so why call them
extra?

Also, is there a case where "private_page" is false? I can't find one...
Specifically for the "split" case (patch 6,
https://lore.kernel.org/all/20260928091005.15567-1-yan.y.zhao@xxxxxxxxx/) where
.topup_external_cache() is invoked in tdp_mmu_alloc_sp_for_split(), how does the
caller (tdp_mmu_alloc_sp_for_split()) know whether private_page should be true
or false without TDX's specific knowledge?

So, I propose dropping the private_page argument and renaming root_spt to
has_root_spt. i.e., either:

(1) int topup(bool has_root_spt, int nr_spts);

or

(2) int topup(bool has_root_spt, int nonroot_spts);

I prefer (1), as it gives the caller the freedom to always pass
PT64_ROOT_MAX_LEVEL even for 4-level VMs. Additionally, has_root_spt in topup()
in (2) would be useless.

int topup(bool has_root_spt, int nonroot_spts)
{
 	add_to_cache(nonroot_spts + 1);
}

> > private_page arg should be unnecessary, since the split path also needs to
> > allocate DPAMT pages for the guest private page.
> > 
> > > Why have root as a separate arg even though it is never used? Because it
> > > makes
> > > the logic make sense from code KVM.
> > What about:
> > 
> > int topup(bool has_root, int nr_spts)
> > {
> >  	add_to_cache(nr_spts + (has_root ? -1 : 0) + 1);
> > }
> 
> "(has_root ? -1 : 0) + 1)" is going to need a comment. So I was suggesting just
> to make it obvious in the code.
like this?

int topup(bool has_root, int nr_spts)
{
	int pairs = nr_spts;

	/* the one for root has been allocated */
	if (has_root)
		pairs -= 1;

	/* for guest private page */
	pairs += 1;

 	add_to_cache(pairs);
}

> > For fault:
> > topup(true, PT64_ROOT_MAX_LEVEL)
> > 
> > For split:
> > topup(false, 1)
> > 
> > > > Besides, I actually introduced a separate cache and op for split in v3,
> > > > https://lore.kernel.org/all/20260106102345.25261-1-yan.y.zhao@xxxxxxxxx.
> > > > Sean merged the cache/op for split with the cache/op for fault in
> > > > DPAMT+Hugepage v5.
> > > 
> > > Yea, but he didn't have any big assumptions in it like this. It was way
> > > simpler.
> > Since the re-insertion solution looks good to you, it should be not be too
> > complex now :)
> > 
> > Given that the fault path theoretically also needs to address the root-only -1
> > calculation issue, having split and fault share the same op seems reasonable?
>