linux-kernel - Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12144256-b71a-4331-8309-2e805dc120d1@linux.intel.com>
Date: Tue, 25 Nov 2025 16:09:53 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc: bp@...en8.de, chao.gao@...el.com, dave.hansen@...el.com,
 isaku.yamahata@...el.com, kai.huang@...el.com, kas@...nel.org,
 kvm@...r.kernel.org, linux-coco@...ts.linux.dev,
 linux-kernel@...r.kernel.org, mingo@...hat.com, pbonzini@...hat.com,
 seanjc@...gle.com, tglx@...utronix.de, vannapurve@...gle.com,
 x86@...nel.org, yan.y.zhao@...el.com, xiaoyao.li@...el.com,
 binbin.wu@...el.com
Subject: Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers



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
> +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).

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

> +#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?

> +			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().

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

> +			goto out_free;
> +		}
> +
> +		atomic_inc(pamt_refcount);
> +	}
> +
> +	return ret;

Maybe just return 0 here since all error paths must be directed to out_free.

> +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);
> +
>
[...]