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

[Yan Zhao]

On Sat, Oct 10, 2026 at 06:12:19AM +0800, Edgecombe, Rick P wrote:
> On Fri, 2026-10-09 at 17:08 +0800, Yan Zhao wrote:
> > On Thu, Oct 08, 2026 at 09:16:12AM +0800, Edgecombe, Rick P wrote:
> > > On Wed, 2026-10-07 at 15:47 -0700, Rick Edgecombe wrote:
> > > > Another idea, change the op to be:
> > > > int topup_external_cache(struct kvm_vcpu *vcpu, bool root, bool private_page,
> > > > int min_nr_spts);
> > > > 
> > > > Normal topup can set:
> > > > root=true
> > > > private_page=true
> > > > min_nr_spts = PT64_ROOT_MAX_LEVEL - 1
> > > > 
> > > > Then we can calculate exactly what we need. And even better, the existing code
> > > > won't nee a comment to explain the weirdness.
> > > 
> > > Actually since this gets a struct kvm arg later in the series, maybe we could
> > > just have two ops. One for topping up from a fault and another for a split.
> > Hmm, no? That would make the min_nr_spts arg meaningless.
> > Reusing the topping up op for the fault also scales better, e.g., it could
> > support splitting under the fault path in the future.
> 
> I don't see how min_nr_spts is meaningless. What do you mean?
> 
> Let me spell it out a bit more. For a fault it would have math like this:
> 
> int topup(bool root, bool private_page, int nr_extra_spts)
> {
> 	add_to_cache(private_page + nr_extra_spts)
> }
> 
> 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.

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);
}

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?