Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers

[Dave Hansen]

On 1/28/26 17:14, Sean Christopherson wrote:
> +static void tdx_pamt_put(struct page *page)
> +{
> +	u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];
> +	atomic_t *pamt_refcount;
> +	u64 tdx_status;
> +
> +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> +		return;
> +
> +	pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> +
> +	scoped_guard(spinlock, &pamt_lock) {
> +		/*
> +		 * If the there are more than 1 references on the pamt page,
> +		 * don't remove it yet. Just decrement the refcount.
> +		 */
> +		if (atomic_read(pamt_refcount) > 1) {
> +			atomic_dec(pamt_refcount);
> +			return;
> +		}
> +
> +		/* Try to remove the pamt page and take the refcount 1->0. */
> +		tdx_status = tdh_phymem_pamt_remove(page, pamt_pa_array);
> +
> +		/*
> +		 * Don't free pamt_pa_array as it could hold garbage when
> +		 * tdh_phymem_pamt_remove() fails.  Don't panic/BUG_ON(), as
> +		 * there is no risk of data corruption, but do yell loudly as
> +		 * failure indicates a kernel bug, memory is being leaked, and
> +		 * the dangling PAMT entry may cause future operations to fail.
> +		 */
> +		if (WARN_ON_ONCE(!IS_TDX_SUCCESS(tdx_status)))
> +			return;
> +
> +		atomic_dec(pamt_refcount);
> +	}
> +
> +	/*
> +	 * pamt_pa_array is populated up to tdx_dpamt_entry_pages() by the TDX
> +	 * module with pages, or remains zero inited. free_pamt_array() can
> +	 * handle either case. Just pass it unconditionally.
> +	 */
> +	free_pamt_array(pamt_pa_array);
> +}

This looks funky.

Right now, this is:

	spin_lock(pamt_lock)
	atomic_inc/dec(fine-grained-refcount)
	tdcall_blah_blah()
	spin_unlock(pamt_lock)

Where it *always* acquires the global lock when DPAMT is supported.
Couldn't we optimize it so that it only acquires it when it has to keep
the refcount stable at zero?

Roughly:

	slow_path = atomic_dec_and_lock(fine-grained-refcount,
					pamt_lock)
	if (!slow_path)
		goto out;

	// fine-grained-refcount==0 and must stay that way with
	// pamt_lock held. Remove the DPAMT pages:
	tdh_phymem_pamt_remove(page, pamt_pa_array)
out:	
	spin_unlock(pamt_lock)

On the acquire side, you do:

	fast_path = atomic_inc_not_zero(fine-grained-refcount)
	if (fast_path)
		return;

	// slow path:
	spin_lock(pamt_lock)

	// Was the race lost with another 0=>1 increment?
	if (atomic_read(fine-grained-refcount) > 0)
		goto out_inc

	tdh_phymem_pamt_add(page, pamt_pa_array)
	// Inc after the TDCALL so another thread won't race ahead of us
	// and try to use a non-existent PAMT entry
out_inc:
	atomic_inc(fine-grained-refcount)
	spin_unlock(pamt_lock)

Then, at least only the 0=>1 and 1=>0 transitions need the global lock.
The fast paths only touch the refcount which isn't shared nearly as much
as the global lock.

BTW, this probably still needs to be spin_lock_irq(), not what I wrote
above, but that's not a big deal to add.

I've stared at this for a bit and don't see any holes. Does anyone else
see any?