Re: [PATCH v1 08/26] x86/virt/tdx: Add tdx_enable_ext() to enable of TDX Module Extensions

Dave Hansen
2025-11-17T17:34:32Z

I really dislike subjects like this. I honestly don't need to know what
the function's name is. The _rest_ of the subject is just words that
don't tell me _anything_ about what this patch does.

In this case, I suspect it's because the patch is doing about 15
discrete things and it's impossible to write a subject that's anything
other than some form of:

x86/virt/tdx: Implement $FOO by making miscellaneous changes

So it's a symptom of the real disease.

On 11/16/25 18:22, Xu Yilun wrote:
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
> Add a kAPI tdx_enable_ext() for kernel to enable TDX Module Extensions
> after basic TDX Module initialization.
>
> The extension initialization uses the new TDH.EXT.MEM.ADD and
> TDX.EXT.INIT seamcalls. TDH.EXT.MEM.ADD add pages to a shared memory
> pool for extensions to consume.

"Shared memory" is an exceedingly unfortunate term to use here. They're
TDX private memory, right?

> The number of pages required is
> published in the MEMORY_POOL_REQUIRED_PAGES field from TDH.SYS.RD. Then
> on TDX.EXT.INIT, the extensions consume from the pool and initialize.

This all seems backwards to me. I don't need to read the ABI names in
the changelog. I *REALLY* don't need to read the TDX documentation names
for them. If *ANYTHING* these names should be trivialy mappable to the
patch that sits below this changelog. They're not.

This changelog _should_ begin:

Currently, TDX module memory use is relatively static. But, some
new features (called "TDX Module Extensions") need to use memory
more dynamically.

How much memory does this consume?

> TDH.EXT.MEM.ADD is the first user of tdx_page_array. It provides pages
> to TDX Module as control (private) pages. A tdx_clflush_page_array()
> helper is introduced to flush shared cache before SEAMCALL, to avoid
> shared cache write back damages these private pages.

First, this talks about "control pages". But I don't know what a control
page is.

Second, these all need to be in imperative voice. Not:

It provides pages to TDX Module as control (private) pages.

Do this:

Provide pages to TDX Module as control (private) pages.

> TDH.EXT.MEM.ADD uses HPA_LIST_INFO as parameter so could leverage the
> 'first_entry' field to simplify the interrupted - retry flow. Host
> don't have to care about partial page adding and 'first_entry'.
>
> Use a new version TDH.SYS.CONFIG for VMM to tell TDX Module which
> optional features (e.g. TDX Connect, and selecting TDX Connect implies
> selecting TDX Module Extensions) to use and let TDX Module update its
> global metadata (e.g. memory_pool_required_pages for TDX Module
> Extensions). So after calling this new version TDH.SYS.CONFIG, VMM
> updates the cached tdx_sysinfo.
>
> Note that this extension initialization does not impact existing
> in-flight SEAMCALLs that are not implemented by the extension. So only
> the first user of an extension-seamcall needs invoke this helper.

Ahh, so this is another bit of very useful information buried deep in
this changelog.

Extensions consume memory, but they're *optional*.

> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 3a3ea3fa04f2..1eeb77a6790a 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -125,11 +125,13 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
> #define seamcall(_fn, _args) sc_retry(__seamcall, (_fn), (_args))
> #define seamcall_ret(_fn, _args) sc_retry(__seamcall_ret, (_fn), (_args))
> #define seamcall_saved_ret(_fn, _args) sc_retry(__seamcall_saved_ret, (_fn), (_args))
> +int tdx_enable_ext(void);
> const char *tdx_dump_mce_info(struct mce *m);
>
> /* Bit definitions of TDX_FEATURES0 metadata field */
> #define TDX_FEATURES0_TDXCONNECT BIT_ULL(6)
> #define TDX_FEATURES0_NO_RBP_MOD BIT_ULL(18)
> +#define TDX_FEATURES0_EXT BIT_ULL(39)
>
> const struct tdx_sys_info *tdx_get_sysinfo(void);
>
> @@ -223,6 +225,7 @@ u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td);
> u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, struct page *page);
> #else
> static inline void tdx_init(void) { }
> +static inline int tdx_enable_ext(void) { return -ENODEV; }
> static inline u32 tdx_get_nr_guest_keyids(void) { return 0; }
> static inline const char *tdx_dump_mce_info(struct mce *m) { return NULL; }
> static inline const struct tdx_sys_info *tdx_get_sysinfo(void) { return NULL; }
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 4370d3d177f6..b84678165d00 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -46,6 +46,8 @@
> #define TDH_PHYMEM_PAGE_WBINVD 41
> #define TDH_VP_WR 43
> #define TDH_SYS_CONFIG 45
> +#define TDH_EXT_INIT 60
> +#define TDH_EXT_MEM_ADD 61
>
> /*
> * SEAMCALL leaf:
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 9a5c32dc1767..bbf93cad5bf2 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -59,6 +59,9 @@ static LIST_HEAD(tdx_memlist);
> static struct tdx_sys_info tdx_sysinfo __ro_after_init;
> static bool tdx_module_initialized __ro_after_init;
>
> +static DEFINE_MUTEX(tdx_module_ext_lock);
> +static bool tdx_module_ext_initialized;
> +
> typedef void (*sc_err_func_t)(u64 fn, u64 err, struct tdx_module_args *args);
>
> static inline void seamcall_err(u64 fn, u64 err, struct tdx_module_args *args)
> @@ -517,7 +520,7 @@ EXPORT_SYMBOL_GPL(tdx_page_array_ctrl_release);
> #define HPA_LIST_INFO_PFN GENMASK_U64(51, 12)
> #define HPA_LIST_INFO_LAST_ENTRY GENMASK_U64(63, 55)
>
> -static u64 __maybe_unused hpa_list_info_assign_raw(struct tdx_page_array *array)
> +static u64 hpa_list_info_assign_raw(struct tdx_page_array *array)
> {
> return FIELD_PREP(HPA_LIST_INFO_FIRST_ENTRY, 0) |
> FIELD_PREP(HPA_LIST_INFO_PFN, page_to_pfn(array->root)) |
> @@ -1251,7 +1254,14 @@ static __init int config_tdx_module(struct tdmr_info_list *tdmr_list,
> args.rcx = __pa(tdmr_pa_array);
> args.rdx = tdmr_list->nr_consumed_tdmrs;
> args.r8 = global_keyid;
> - ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> +
> + if (tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_TDXCONNECT) {
> + args.r9 |= TDX_FEATURES0_TDXCONNECT;
> + args.r11 = ktime_get_real_seconds();
> + ret = seamcall_prerr(TDH_SYS_CONFIG | (1ULL << TDX_VERSION_SHIFT), &args);
> + } else {
> + ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> + }

I'm in the first actual hunk of code and I'm lost. I don't have any idea
what the "(1ULL << TDX_VERSION_SHIFT)" is doing.

Also, bifurcating code paths is discouraged. It's much better to not
copy and paste the code and instead name your variables and change
*them* in a single path:

u64 module_function = TDH_SYS_CONFIG;
u64 features = 0;
u64 timestamp = 0;

if (tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_TDXCONNECT) {
features |= TDX_FEATURES0_TDXCONNECT;
timestamp = ktime_get_real_seconds();
module_function |= 1ULL << TDX_VERSION_SHIFT;
}

ret = seamcall_prerr(module_function, &args);

This would also provide a place to say what the heck is going on with
the whole "(1ULL << TDX_VERSION_SHIFT)" thing. Just hacking it in and
open-coding makes it actually harder to comment and describe it.

> /* Free the array as it is not required anymore. */
> kfree(tdmr_pa_array);
> @@ -1411,6 +1421,11 @@ static __init int init_tdx_module(void)
> if (ret)
> goto err_free_pamts;
>
> + /* configuration to tdx module may change tdx_sysinfo, update it */
> + ret = get_tdx_sys_info(&tdx_sysinfo);
> + if (ret)
> + goto err_reset_pamts;
> +
> /* Config the key of global KeyID on all packages */
> ret = config_global_keyid();
> if (ret)
> @@ -1488,6 +1503,160 @@ static __init int tdx_enable(void)
> }
> subsys_initcall(tdx_enable);
>
> +static int enable_tdx_ext(void)
> +{

Comments, please. "ext" can mean too many things. What does this do and
why can it fail?

> + struct tdx_module_args args = {};
> + u64 r;
> +
> + if (!tdx_sysinfo.ext.ext_required)
> + return 0;

Is this an optimization or is it functionally required?

> + do {
> + r = seamcall(TDH_EXT_INIT, &args);
> + cond_resched();
> + } while (r == TDX_INTERRUPTED_RESUMABLE);
> +
> + if (r != TDX_SUCCESS)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static void tdx_ext_mempool_free(struct tdx_page_array *mempool)
> +{
> + /*
> + * Some pages may have been touched by the TDX module.
> + * Flush cache before returning these pages to kernel.
> + */
> + wbinvd_on_all_cpus();
> + tdx_page_array_free(mempool);
> +}
> +
> +DEFINE_FREE(tdx_ext_mempool_free, struct tdx_page_array *,
> + if (!IS_ERR_OR_NULL(_T)) tdx_ext_mempool_free(_T))
> +
> +/*
> + * The TDX module exposes a CLFLUSH_BEFORE_ALLOC bit to specify whether
> + * a CLFLUSH of pages is required before handing them to the TDX module.
> + * Be conservative and make the code simpler by doing the CLFLUSH
> + * unconditionally.
> + */
> +static void tdx_clflush_page(struct page *page)
> +{
> + clflush_cache_range(page_to_virt(page), PAGE_SIZE);
> +}

arch/x86/virt/vmx/tdx/tdx.c has:

static void tdx_clflush_page(struct page *page)
{
clflush_cache_range(page_to_virt(page), PAGE_SIZE);
}

Seems odd to see this here.

> +static void tdx_clflush_page_array(struct tdx_page_array *array)
> +{
> + for (int i = 0; i < array->nents; i++)
> + tdx_clflush_page(array->pages[array->offset + i]);
> +}
> +
> +static int tdx_ext_mem_add(struct tdx_page_array *mempool)
> +{

I just realized the 'mempool' has nothing to do with 'struct mempool',
which makes this a rather unfortunate naming choice.

> + struct tdx_module_args args = {
> + .rcx = hpa_list_info_assign_raw(mempool),
> + };
> + u64 r;
> +
> + tdx_clflush_page_array(mempool);
> +
> + do {
> + r = seamcall_ret(TDH_EXT_MEM_ADD, &args);
> + cond_resched();
> + } while (r == TDX_INTERRUPTED_RESUMABLE);
> +
> + if (r != TDX_SUCCESS)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static struct tdx_page_array *tdx_ext_mempool_setup(void)
> +{
> + unsigned int nr_pages, nents, offset = 0;
> + int ret;
> +
> + nr_pages = tdx_sysinfo.ext.memory_pool_required_pages;
> + if (!nr_pages)
> + return NULL;
> +
> + struct tdx_page_array *mempool __free(tdx_page_array_free) =
> + tdx_page_array_alloc(nr_pages);
> + if (!mempool)
> + return ERR_PTR(-ENOMEM);
> +
> + while (1) {
> + nents = tdx_page_array_fill_root(mempool, offset);

This is really difficult to understand. It's not really filling a
"root", it's populating an array. The structure of the loop is also
rather non-obvious. It's doing:

while (1) {
fill(&array);
tell_tdx_module(&array);
}

Why can't it be:

while (1)
fill(&array);
while (1)
tell_tdx_module(&array);

for example?

> + if (!nents)
> + break;
> +
> + ret = tdx_ext_mem_add(mempool);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + offset += nents;
> + }
> +
> + return no_free_ptr(mempool);
> +}

This patch is getting waaaaaaaaaaaaaaay too long. I'd say it needs to be
4 or 5 patches, just eyeballing it.

Call be old fashioned, but I suspect the use of __free() here is atually
hurting readability.

> +static int init_tdx_ext(void)
> +{
> + int ret;
> +
> + if (!(tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_EXT))
> + return -EOPNOTSUPP;
> +
> + struct tdx_page_array *mempool __free(tdx_ext_mempool_free) =
> + tdx_ext_mempool_setup();
> + /* Return NULL is OK, means no need to setup mempool */
> + if (IS_ERR(mempool))
> + return PTR_ERR(mempool);

That's a somewhat odd comment to put above an if() that doesn't return NULL.

> + ret = enable_tdx_ext();
> + if (ret)
> + return ret;
> +
> + /* Extension memory is never reclaimed once assigned */
> + if (mempool)
> + tdx_page_array_ctrl_leak(no_free_ptr(mempool));
> +
> + return 0;
> +}

> +/**
> + * tdx_enable_ext - Enable TDX module extensions.
> + *
> + * This function can be called in parallel by multiple callers.
> + *
> + * Return 0 if TDX module extension is enabled successfully, otherwise error.
> + */
> +int tdx_enable_ext(void)
> +{
> + int ret;
> +
> + if (!tdx_module_initialized)
> + return -ENOENT;
> +
> + guard(mutex)(&tdx_module_ext_lock);
> +
> + if (tdx_module_ext_initialized)
> + return 0;
> +
> + ret = init_tdx_ext();
> + if (ret) {
> + pr_debug("module extension initialization failed (%d)\n", ret);
> + return ret;
> + }
> +
> + pr_debug("module extension initialized\n");
> + tdx_module_ext_initialized = true;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_enable_ext);
> +
> static bool is_pamt_page(unsigned long phys)
> {
> struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
> @@ -1769,17 +1938,6 @@ static inline u64 tdx_tdr_pa(struct tdx_td *td)
> return page_to_phys(td->tdr_page);
> }
>
> -/*
> - * The TDX module exposes a CLFLUSH_BEFORE_ALLOC bit to specify whether
> - * a CLFLUSH of pages is required before handing them to the TDX module.
> - * Be conservative and make the code simpler by doing the CLFLUSH
> - * unconditionally.
> - */
> -static void tdx_clflush_page(struct page *page)
> -{
> - clflush_cache_range(page_to_virt(page), PAGE_SIZE);
> -}

Ahh, here's the code move.

This should be in its own patch.