Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Tue, 2025-11-25 at 16:09 +0800, Binbin Wu wrote:
> 
> On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> [...]
> >   
> > +/* Number PAMT pages to be provided to TDX module per 2M region of PA */
>            ^                                             ^
>            of                                           2MB

Sounds good.

> > +static int tdx_dpamt_entry_pages(void)
> > +{
> > +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > +		return 0;
> > +
> > +	return tdx_sysinfo.tdmr.pamt_4k_entry_size * PTRS_PER_PTE / PAGE_SIZE;
> > +}
> > +
> > +/*
> > + * The TDX spec treats the registers like an array, as they are ordered
> > + * in the struct. The array size is limited by the number or registers,
> > + * so define the max size it could be for worst case allocations and sanity
> > + * checking.
> > + */
> > +#define MAX_TDX_ARG_SIZE(reg) (sizeof(struct tdx_module_args) - \
> > +			       offsetof(struct tdx_module_args, reg))
> 
> This should be the maximum number of registers could be used to pass the
> addresses of the pages (or other information), it needs to be divided by sizeof(u64).

Oops, right.

> 
> Also, "SIZE" in the name could be confusing.

Yes LENGTH is probably better.

> 
> > +#define TDX_ARG_INDEX(reg) (offsetof(struct tdx_module_args, reg) / \
> > +			    sizeof(u64))
> > +
> > +/*
> > + * Treat struct the registers like an array that starts at RDX, per
> > + * TDX spec. Do some sanitychecks, and return an indexable type.
> sanitychecks -> sanity checks
> 
> > + */
> [...]
> > +/* Serializes adding/removing PAMT memory */
> > +static DEFINE_SPINLOCK(pamt_lock);
> > +
> > +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> > +int tdx_pamt_get(struct page *page)
> > +{
> > +	u64 pamt_pa_array[MAX_TDX_ARG_SIZE(rdx)];
> > +	atomic_t *pamt_refcount;
> > +	u64 tdx_status;
> > +	int ret;
> > +
> > +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > +		return 0;
> > +
> > +	ret = alloc_pamt_array(pamt_pa_array);
> > +	if (ret)
> > +		goto out_free;
> > +
> > +	pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> > +
> > +	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);
> 
> So far, all atomic operations are inside the spinlock.
> May be better to add some info in the change log that atomic is needed due to
> the optimization in the later patch?

Yea, I really debated this. Changing to atomics is more churn, but doing part of
the optimizations early is weird too. I think I might go with the more churn
approach.

> 
> > +			goto out_free;
> > +		}
> > +
> > +		/* Try to add the pamt page and take the refcount 0->1. */
> > +
> > +		tdx_status = tdh_phymem_pamt_add(page, pamt_pa_array);
> > +		if (!IS_TDX_SUCCESS(tdx_status)) {
> > +			pr_err("TDH_PHYMEM_PAMT_ADD failed: %#llx\n", tdx_status);
> 
> Can use pr_tdx_error().

Not in arch/x86, plus it became TDX_BUG_ON() in the cleanup series.

> 
> Aslo, so for in this patch, when this SEAMCALL failed, does it indicate a bug?

Yes. This should set ret to an error value too. Yes SEAMCALL failure indicates a
bug, but it should be harmless to the host. tdx_pamt_get() returns a failure so
the calling code can handle it. The tdx_pamt_put() is a little weirder. We can
have the option to keep the refcount elevated if remove fails. This means DPAMT
stays addded for this 2MB range. I think it is ok for 4KB guest memory. It
basically means a DPAMT page will stay in place forever in the case of a bug,
which is the current non-dynamic PAMT situation. So I'm not sure if a full WARN
is needed.

But I'm just realizing that the page it is backing could reform it's way into a
2MB page. Then, after the TDX huge pages series, it could end up getting mapped
as 2MB private memory. In which case the PAMT.REMOVE needs to succeed before
adding the page as 2MB. Hmm, need to check TDX huge page series, because I think
this is not handled. So when TDX huge pages comes, a WARN might be more
appropriate.

We might need to make tdx_pamt_put() return an error for huge pages, and treat
failure like the other TDX remove failures, or maybe just a WARN. I'd lean
towards just the WARN. Do you have any better ideas?

> 
> > +			goto out_free;
> > +		}
> > +
> > +		atomic_inc(pamt_refcount);
> > +	}
> > +
> > +	return ret;
> 
> Maybe just return 0 here since all error paths must be directed to out_free.

Yes.

> 
> > +out_free:
> > +	/*
> > +	 * pamt_pa_array is populated or zeroed up to tdx_dpamt_entry_pages()
> > +	 * above. free_pamt_array() can handle either case.
> > +	 */
> > +	free_pamt_array(pamt_pa_array);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tdx_pamt_get);
> > +
> > 
> [...]