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

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

On Thu, Jul 09, 2026 at 08:33:40AM +0800, Edgecombe, Rick P wrote:
> Thanks for the review!
> 
> On Wed, 2026-07-08 at 14:46 +0800, Yan Zhao wrote:
> > On Mon, May 25, 2026 at 07:35:09PM -0700, Rick Edgecombe wrote:
> > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > 
> > > 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.

> I could say "When that scenario happens..."? But trying to keep the word count
> down.
... 

> > > +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> > How about
> > "Bump the refcount of the PAMT page pair for the given PFN and add the PAMT
> > page pair on the first reference." ?
> 
> I think I see what you are getting at. How about this instead:
> Bump PAMT refcount for the given pfn and allocate PAMT backing if needed
Hmm, in this patch, "allocate PAMT backing" is always performed.
Only "add PAMT backing" is "if needed".
That's my reasoning. :)

And I can understand that "PAMT refcount" refers to the refcount of the PAMT
page pair. I just thought it would be clear to call this out explicitly.
No strong opinion though.

> > >  static int tdx_pamt_get(kvm_pfn_t pfn)
> > >  {
> > >  	struct page *pamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
> > > +	atomic_t *pamt_refcount;
> > >  	u64 tdx_status;
> > >  	int ret;
> > >  
> > > @@ -2057,10 +2061,26 @@ static int tdx_pamt_get(kvm_pfn_t pfn)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	tdx_status = tdh_phymem_pamt_add(pfn, pamt_pages);
> > > -	if (tdx_status != TDX_SUCCESS) {
> > > -		ret = -EIO;
> > > -		goto out_free;
> > > +	pamt_refcount = tdx_find_pamt_refcount(pfn);
> > > +
> > > +	scoped_guard(spinlock, &pamt_lock) {
> > > +		/*
> > > +		 * If the pamt page is already added (i.e. refcount >= 1),
> > > +		 * then just increment the refcount.
> > > +		 */
> > > +		if (atomic_read(pamt_refcount)) {
> > > +			atomic_inc(pamt_refcount);
> > > +			goto out_free;
> > > +		}
> > > +
> > > +		/* Try to add the pamt page and take the refcount 0->1. */
> > > +		tdx_status = tdh_phymem_pamt_add(pfn, pamt_pages);
> > > +		if (WARN_ON_ONCE(tdx_status != TDX_SUCCESS)) {
> > > +			ret = -EIO;
> > > +			goto out_free;
> > > +		}
> > > +
> > > +		atomic_set(pamt_refcount, 1);
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -2069,26 +2089,46 @@ static int tdx_pamt_get(kvm_pfn_t pfn)
> > >  	return ret;
> > >  }
> > >  
> > > -/* Free PAMT memory for the given page */
> > > +/*
> > > + * Drop PAMT refcount for the given page and free PAMT memory if it is no
> > > + * longer needed.
> > How about:
> > "Drop the refcount of the PAMT page pair for the given PFN, and remove the
> > PAMT page pair if it is no longer needed." ?
> 
> Matching the above:
> 
> Drop PAMT refcount for the given pfn and free PAMT backing if needed
LGTM.
 
> > Another nit:
> > How about renaming the title from
> > "Handle concurrent callers in tdx_pamt_get/put()" to
> > "Handle concurrent calls to tdx_pamt_get/put()" ?
> 
> I'm not seeing a benefit. Just because it's shorter?
I previously thought "callers in" sounded odd.
Feel free to ignore it if it is not :)