Re: [PATCH v4 01/17] x86/virt/tdx: Enhance tdx_pamt_get/put() to support huge pages

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Thu, 2026-10-15 at 11:41 +0800, Yan Zhao wrote:
> On Tue, Oct 13, 2026 at 07:18:15AM +0800, Edgecombe, Rick P wrote:
> > On Sat, 2026-10-10 at 18:18 +0800, Yan Zhao wrote:
> > > On Fri, Oct 02, 2026 at 07:02:40AM +0800, Edgecombe, Rick P wrote:
> > > > On Mon, 2026-09-28 at 17:08 +0800, Yan Zhao wrote:
> > > > > The TDX module uses PAMT to track metadata for every physical page (within
> > > > > TDMR) across three levels: 1GB, 2MB, and 4KB. The VMM is responsible for
> > > > > providing backing pages for the PAMT. Without Dynamic PAMT (DPAMT), all
> > > > > PAMT backing pages are allocated statically at system boot time. With DPAMT
> > > > > enabled, PAMT backing pages tracking physical pages at the 4KB level
> > > > > (referred to as DPAMT pages) are dynamically installed and uninstalled at
> > > > > runtime.
> > > > > 
> > > > > Currently, the VMM invokes tdx_pamt_get()/tdx_pamt_put() to install and
> > > > > uninstall DPAMT pages under the assumption that all physical pages are
> > > > > tracked at 4KB level.  However, when a guest page is mapped as a huge page
> > > > > in the S-EPT, the TDX module tracks it directly at the huge page level,
> > > > > meaning installation and uninstallation of DPAMT pages for huge pages is
> > > > > neither needed nor applicable.
> > > > > 
> > > > > Add a "level" parameter to tdx_pamt_get()/tdx_pamt_put() so the helpers
> > > > > can skip DPAMT page installation and uninstallation for huge pages.
> > > > 
> > > > I guess this means we need to make sure to get/put at the right level that the
> > > > TDX module is using it as. For example in this implementation, if a huge page
> > > > gets demoted, we can't call them like this:
> > > > 1. tdx_pamt_get(huge_pfn, PG_LEVEL_2MB)
> > > > 2. demote(huge_pfn)
> > > > 3. tdx_pamt_put(huge_pfn, PG_LEVEL_2MB)
> > > Actually, calling in this way is fine, since steps 1 & 3 are effectively no-ops.
> > > However, we can't call them like below.
> > 
> > But step 2 incremented the refcounts. Then step 3 skips the decrement.
> > So don't you need?
> > 1. tdx_pamt_get(huge_pfn, PG_LEVEL_2MB)
> > 2. demote(huge_pfn)
> > 3. for (int i...) tdx_pamt_put(huge_pfn+i, PG_LEVEL_4KB);
> Oh, what I mean is there's no need for tdx_sept_split_leaf_spte() to invoke
> either 1 or 3. It only needs to invoke 2, though it doesn't break anything if
> tdx_sept_split_leaf_spte() incorrectly invokes
> tdx_pamt_{get/put}(huge_pfn, PG_LEVEL_2MB).
> 
> tdx_pamt_put(pfn, PG_LEVEL_4KB) for each 4K page is done in
> tdx_sept_remove_leaf_spte().
> 
> See more in the below contract.
> 
> > > In DPAMT combined v5, Sean moved the level checking from KVM to
> > > tdx_pamt_{get/put}() [2].
> > > However, Sean's version introduced __tdx_pamt_{get/put}() for invocations where
> > > 4KB size is enforced.
> > > 
> > > During our internal review, you said:
> > > "Can you also justify why to have two functions instead of page LEVEL_4K in a
> > > few places?"
> > > 
> > > So, compared to passing level to tdx_pamt_{get/put}() while introducing
> > > __tdx_pamt_{get/put}() for the always-4KB invocations, I updated the patch to
> > > drop __tdx_pamt_{get/put}(), having KVM pass 4KB directly in the always-4KB
> > > invocations.
> > > 
> > > I'm actually ok with any of these approaches. It seems they all require the
> > > caller to know about the right level size and whether to invoke PAMT get/put.
> > 
> > Yea. That is kind of the problem. The caller needs to understand the impact of
> > the mapping level. The level arg isn't able wrap everything within the helpers.
> > So how the get/put helpers handle it is internal details that caller needs to
> > know. So there is abstraction leakage I think.
> > 
> > So probably the most important thing is that it is clear how to use it. Then
> > whatever is the simplest code. It might be Kiryl's implementation. What do you
> > think?
> Hmm, I don't see checking level in KVM is better or simpler than passing the
> mapping level to tdx_pamt_{get/put}(). KVM still needs to know
> tdx_sept_split_leaf_spte() can't invoke tdx_pamt_get() for guest pages.
> 
> So, what about explaining the contract as a comment to tdx_pamt_{get/put}()?
> That is:
> 
> (1) KVM needs to invoke tdx_pamt_get() before it maps a new page (guest page or
>     page table page).
> 
>     So, KVM needs to invoke tdx_pamt_get() in tdx_sept_map_leaf_spte() for
>     guest pages, and tdx_sept_map_nonleaf_spte() for page table pages, and
>     tdx_sept_split_leaf_spte() needs to invoke tdx_pamt_get() for the page table
>     page.
>     However, tdx_sept_split_leaf_spte() doesn't need to invoke tdx_pamt_get()
>     for the guest page since the page has already been mapped.
> 
> (2) KVM needs to invoke tdx_pamt_put() after it unmaps a page (guest page or page
>     table page.
> 
>     So, KVM invokes tdx_pamt_put() in tdx_sept_remove_leaf_spte() and
>     tdx_free_control_page() in tdx_sept_free_private_spt().
>     It also needs to invoke tdx_pamt_put() when it fails to map a page.
> 
> With the contract, it seems to be simpler for KVM to pass the level to
> tdx_pamt_{get/put}() ?
> The dpamt refcount adjustment are invisible to KVM.
> 
> Do you think the contract is clear?

No, it doesn't seem clear. It's not clear which combination of args will update
the refcount or mapping level for different scenarios. The above seems to just
say what recipes will work, which only makes sense since you know all the
internal details. It doesn't describe a clear contact.

I think it is like - Pamt has a level, and three operations manipulate it. The
pamt level needs to match what TDX will map the page at.

The pamt get/put infra keeps refcounts for 4KB level page use. They may change
the mapping level, but they always update the refcount which is an explicit 4KB
level thing. So to me it makes much more sense if they stay 4KB level
operations.

Then the contract is simply:

   There are only two possible mapping levels: 2MB and 4KB. There is a refcount
   for each 2MB aligned range. The mapping level will be 4KB when there is any
   refcount for 2MB aligned range.
   
   tdx_pamt_get() increments the refcount
   tdx_pamt_put() decrements the refcount
   tdh_mem_page_demote() increments the refcount for each 4KB page
   
   The mapping level will be determined by the refcount after each operation is
   complete.

If you want to pass the level, somehow it has to be clear how the refcount and
mapping level will be adjusted for each combination of operations.


> 
> > Or, hmm. It would be easy to catch misuse on the put side by (dpamt_refcount !=
> > 0 && level > PG_LEVEL_4K)? Then some comments to help user use the existing
> > design correctly? Either of those seem ok. But my concern was that this part of
> > the design wasn't clear. So most important is actually to explain the why of the
> > design with an argument of the benefits.
> If the above contract looks good to you, maybe I can explain the contract and
> design of the API in both the code comment and the patch log.