Re: [PATCH v3 6/6] x86/virt/tdx: Support DPAMT when adding memory for the extensions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tdx: allocator thrashing due to unconditional DPAMT allocations
--

commit 02adad93d9f4243664b0cea8f1ad9c0b433e79a8
Author: Xu Yilun <yilun.xu@linux.intel.com>

x86/virt/tdx: Support DPAMT when adding memory for the extensions

This commit updates the TDX module extension memory setup to install a 4K
Dynamic PAMT (DPAMT) for each page before adding it. It calls tdx_pamt_get()
for each page within the memory chunk.

> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 5d5f9da1ab028..27a7039ee443c 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1330,8 +1330,15 @@ static __init int tdx_ext_mem_setup(void)
>  		struct page *chunk = page + added_pages;
>  		unsigned int i;
>  
> -		for (i = 0; i < chunk_pages; i++)
> +		for (i = 0; i < chunk_pages; i++) {
> +			ret = tdx_pamt_get(page_to_pfn(chunk + i), NULL);

[Severity: Medium]
Will calling tdx_pamt_get() sequentially without a DPAMT cache cause severe
allocator thrashing during boot?

Looking at tdx_pamt_get(), providing a NULL cache forces an unconditional 
allocation of DPAMT backing pages before checking the reference count:

arch/x86/virt/vmx/tdx/tdx.c:tdx_pamt_get() {
    ...
	ret = alloc_pamt_array(pamt_pages, cache);
	if (ret)
		return ret;

	dpamt_refcount = tdx_find_dpamt_refcount(pfn);

	spin_lock(&dpamt_lock);

	/*
	 * If the DPAMT entry is already added (i.e. refcount >= 1),
	 * then just increment the refcount.
	 */
	if (atomic_inc_not_zero(dpamt_refcount))
		goto out_free;
    ...
out_free:
	spin_unlock(&dpamt_lock);
	free_pamt_array(pamt_pages);
    ...
}

Since 511 out of 512 pages in each 2MB block will find the entry already
present, the newly allocated pages are immediately freed. For tens of
megabytes of extensions, this results in thousands of unnecessary allocations
and frees. Furthermore, free_pamt_array() performs expensive memory resets,
leading to unnecessary boot delays.

Could a struct tdx_pamt_cache be allocated locally in tdx_ext_mem_setup() and
passed into tdx_pamt_get() to avoid this thrashing?

> +			if (ret) {
> +				WARN(1, "DPAMT setup error for extensions, stranded all pages\n");
> +				goto out_free_hpa_list;
> +			}
> +
>  			hpa_list->phys[i] = page_to_phys(chunk + i);
> +		}
>  
>  		ret = tdx_ext_mem_add(hpa_list, chunk_pages);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-tdx-module-ext-v3-0-db52cb05b918@linux.intel.com?part=6