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

[Edgecombe, Rick P]

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.

> 
> 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.

> 
> 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?