Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()

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

On Fri, Jul 10, 2026 at 07:09:04AM +0800, Edgecombe, Rick P wrote:
> On Thu, 2026-07-09 at 10:50 +0800, Yan Zhao wrote:
> > > > > tdx_pamt_get()/tdx_pamt_put() unconditionally add or remove Dynamic PAMT
> > > > > backing for the 2MB region covering the passed pfn. However, multiple
> > > > > callers can concurrently operate on 4KB pages that fall within the same
> > > > > 2MB region. When this happens only one Dynamic PAMT page pair needs to
> > > > > be
> > > > What "this" stands for is not clear and a comma is missing after
> > > > "happens".
> > > 
> > > Really this is not clear? The previous sentence is all about a senario being
> > > possible "multiple callers can concurrently operate on 4KB pages that fall
> > > within the same 2MB region". So I thought this would be clear.
> > Hmm, sorry for nitpicking.
> > But the previous sentence says "multiple callers can concurrently operate on
> > 4KB pages", where "operate" doesn't necessarily imply allocating pages.
> > However, the latter sentence assumes PAMT pages need to be installed. That's
> > why I think "it" is not clear.
> 
> Not sure what you mean by "it", but I guess you are saying that "this" could
Oh, yes, "this"! :D

> refer to the first sentence and not the one that immediately preceded the "this"
> reference? That part still reads ok to me, but if you still feel it's unclear
> I'll change it to.
> 
> Actually, another problem with this paragraph is focuses too much on concurrent
> callers, when the main thing this patch does is address get-ing multiple pages.
> The concurrency is added to make the refcounts work. I'd like to address that
> too. How is this to you?
This version looks great!
Thanks!
 
> x86/virt/tdx: Handle multiple callers in tdx_pamt_get/put()
> 
> tdx_pamt_get()/tdx_pamt_put() unconditionally add or remove Dynamic PAMT 
> backing for the 2MB region covering the passed page. However, multiple 
> callers can add or remove 4KB pages that fall within the same 2MB region
> and in that scenario only a single PAMT entry is required.
> 
> Make the helpers handle only adding/removing Dynamic PAMT backing when 
> required, by refcounting each 2MB range. Gate the actual Dynamic PAMT add
> and remove on refcount transitions (0->1 and 1->0). Serialize the refcount
> check and SEAMCALL with a global spinlock so the read-decide-act sequence
> is atomic. This also avoids TDX module BUSY errors, as the Dynamic PAMT add
> and remove SEAMCALLs take internal TDX module locks for the 2MB ranges of
> the specified PFN and the PAMT page pair PFNs. So simultaneous attempts on
> the same 2MB ranges of the PFNs would otherwise encounter and error, which
> would not be handleable in the put case.
> 
> The lock is global and heavyweight. Use simple conditional logic to keep
> correctness obvious. This will be optimized in a later change.
> 
> The pamt_refcount[]s are atomic_t's. They do not strictly need to be
Nit: Not sure if it's correct.
Just in case it's a typo: pamt_refcounts[].

> because all access is protected by pamt_lock. The overhead of an atomic_t
> in this situation is minuscule compared to the global lock. Leave the
> atomic_t in place to enable future optimization with minimal churn.