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

> 
> 1. tdx_pamt_get(huge_pfn, PG_LEVEL_2MB)
> 2. demote(huge_pfn)
> 3. tdx_pamt_put(huge_pfn, PG_LEVEL_4KB)
> 
> That's why 1 & 3 are not required and DPAMT pages management for the private
> guest page being demoted are handled in demote().
> 
> > So it is up to the caller to reason about what the mapping level is.
> Yes.
> 
> > Which makes me wonder what is the point of passing level? The caller has to
> > reason about what level the TDX module is using the memory at. The
> > pamt_get/put() have to take their best guess at the ambiguity. For example line
> > 3 above could actually free the PAMT, but it can't know to do that. Unless we
> > added some internal tracking, but it seems too much.
> > 
> > There are only three places where a non-4KB level is passed to get/put. We could
> > alternatively drop this patch and have KVM decide whether to call get/put or
> > not. Since it is actually the only thing that knows the right page size.
> > What do you think?
> Are you suggesting the implementation in v3 [1]?

Yea that was the path I was wandering down.

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

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.

> 
> [1] https://lore.kernel.org/all/20260106102304.25211-1-yan.y.zhao@intel.com
> [2] https://lore.kernel.org/all/20260129011517.3545883-30-seanjc@google.com