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

[Yan Zhao <yan.y.zhao@intel.com>]

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?

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