Re: [PATCH v3 00/24] KVM: TDX huge page support for private memory

[Sean Christopherson <seanjc@google.com>]

On Fri, Jan 16, 2026, Dave Hansen wrote:
> On 1/14/26 16:19, Sean Christopherson wrote:
> >> 'struct page' gives us two things: One is the type safety, but I'm
> >> pretty flexible on how that's implemented as long as it's not a raw u64
> >> getting passed around everywhere.
> > I don't necessarily disagree on the type safety front, but for the specific code
> > in question, any type safety is a facade.  Everything leading up to the TDX code
> > is dealing with raw PFNs and/or PTEs.  Then the TDX code assumes that the PFN
> > being mapped into the guest is backed by a struct page, and that the folio size
> > is consistent with @level, without _any_ checks whatsover.  This is providing
> > the exact opposite of safety.
> > 
> >   static int tdx_mem_page_aug(struct kvm *kvm, gfn_t gfn,
> > 			    enum pg_level level, kvm_pfn_t pfn)
> >   {
> > 	int tdx_level = pg_level_to_tdx_sept_level(level);
> > 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > 	struct page *page = pfn_to_page(pfn);    <==================
> 
> I of course agree that this is fundamentally unsafe, it's just not
> necessarily bad code.
> 
> I hope we both agree that this could be made _more_ safe by, for
> instance, making sure the page is in a zone, pfn_valid(), and a few more
> things.
>
> In a perfect world, these conversions would happen at a well-defined
> layer (KVM=>TDX) and in relatively few places. That layer transition is
> where the sanity checks happen. It's super useful to have:
> 
> struct page *kvm_pfn_to_tdx_private_page(kvm_pfn_t pfn)
> {
> 	struct page *page = pfn_to_page(pfn);
> #ifdef DEBUG
> 	WARN_ON_ONCE(pfn_valid(pfn));
> 	// page must be from a "file"???
> 	WARN_ON_ONCE(!page_mapping(page));
> 	WARN_ON_ONCE(...);
> #endif
> 	return page;
> }
> 
> *EVEN* if the pfn_to_page() itself is unsafe, and even if the WARN()s
> are compiled out, this explicitly lays out the assumptions and it means
> someone reading TDX code has an easier idea comprehending it.

I object to the existence of those assumptions.  Why the blazes does TDX care
how KVM and guest_memfd manages memory?  If you want to assert that the pfn is
compatible with TDX, then by all means.  But I am NOT accepting any more KVM code
that assumes TDX memory is backed by refcounted struct page.  If I had been paying
more attention when the initial TDX series landed, I would have NAK'd that too.

tdh_mem_page_aug() is just an absurdly slow way of writing a PTE.  It doesn't
_need_ the pfn to be backed a struct page, at all.  IMO, what you're asking for
is akin to adding a pile of unnecessary assumptions to e.g. __set_spte() and
__kvm_tdp_mmu_write_spte().  No thanks.

> It's also not a crime to do the *same* checking on kvm_pfn_t and not
> have a type transition. I just like the idea of changing the type so
> that the transition line is clear and the concept is carried (forced,
> even) through the layers of helpers.