Re: [PATCH v2 1/5] x86/virt/tdx: Move TDH.SYS.CONFIG operations into a wrapper

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

On Tue, 2026-09-15 at 18:26 +0800, Xu Yilun wrote:
> In Linux, SEAMCALL wrappers are introduced to avoid broad SEAMCALL
> access by exposing only a selection of SEAMCALL leafs, but also to
> abstract the SEAMCALL register ABIs.
> 


> The latter improves readability and
> reuse for SEAMCALL leafs that are called multiple times.

I'm trying to adjust to not using former/latter. The feedback I've seen is that
it is too much to remember as you read along. How about:
The abstraction improves...

> 
> Some SEAMCALL leafs are not explicitly wrapped because the level of TDX
> ABI details needed to perform the call is low enough to flow well with
                                         ^are
> the calling code.
> 
> For some of the currently unwrapped SEAMCALL leafs, TDX architecture
> adjusts the ABI and adds SEAMCALL version selection for backward
> compatibility.
> 

Reads a little weird to me. Like I'm not sure when this adjusting is happening.
How about:

..., the TDX architecture has evolved the ABI to introduce new versions of
existing SEAMCALLs. VMM code can select the version to call based on what is
supported by the loaded TDX module.

>  Future kernel will need to support the changes. 
> 

?? I guess you mean selecting between SEAMCALL versions?

> This will
> leak more ABI details into the surrounding caller code and decrease
> readability of the other logic. To keep the ABI details contained, move
> the SEAMCALL leafs that will need version selection into wrappers.
> 
> The cleanest separation would be to have kernel data types for the
> SEAMCALL wrapper arguments,
> 

This is now talking about general seamcall wrapper design. It could read like
it's instead talking about clean separation of SEAMCALL versions?

>  and have them marshaled into SEAMCALL leaf
> ABI types (often u64s) inside the wrapper. This works for many SEAMCALL
> leafs but becomes cumbersome when the register ABI type is a physical
> address which points to a buffer for an in-memory ABI. If the SEAMCALL
> wrapper only accepts kernel data types, it may need duplicate buffer
> allocation and copies to match the in-memory ABI. Another solution is
> to define a named helper structure that mirrors the in-memory ABI,
> populate it in a separate flow, then pass it to the SEAMCALL wrapper.
> struct seamldr_params is an existing example of this pattern.
> 
> TDH.SYS.CONFIG requires a list of TDMR information in the form of a PA
> array. The PA array is the in-memory ABI. Create a structure for the PA
> array, use it as the argument when creating the wrapper for
> TDH.SYS.CONFIG.
> 
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
> v2:
>  - Remove TDH.SYS.UPDATE wrapper (Dave & Rick)
>  - Talk about the handling of in-memory ABIs for SEAMCALL wrappers
>    (Rick)
>  - Refactor the entire changelog according to Rick's suggestion (Rick)
>  - Add code comment for struct tdmr_info_pa_array (AI nitpicker)
>  - Use kernel data type for nr_tdmr_pa parameter (AI nitpicker)
> 
> v1:
>  - This patch is split out from the last series (Rick)
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 1668f8615607..e06932f80395 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -998,11 +998,33 @@ static __init int construct_tdmrs(struct list_head *tmb_list,
>  	return ret;
>  }
>  
> +/*
> + * This is an array of HPAs, each points to a TDMR_INFO data structure (see
> + * struct tdmr_info).
> + *
> + * It is the in-memory ABI that the kernel passes to the TDX module to specify
> + * the ranges of TD Memory Regions (TDMRs) and their associated PAMT memory.
> + */
> +struct tdmr_info_pa_array {
> +	DECLARE_FLEX_ARRAY(u64, phys);
> +};
> +
> +static __init int tdx_sys_config(struct tdmr_info_pa_array *tdmr_pa_array,
> +				 unsigned int nr_tdmr_pa, u64 global_keyid)
> +{
> +	struct tdx_module_args args = {
> +		.rcx = __pa(tdmr_pa_array),
> +		.rdx = nr_tdmr_pa,
> +		.r8 = global_keyid,
> +	};
> +
> +	return seamcall_prerr(TDH_SYS_CONFIG, &args);
> +}
> +
>  static __init int config_tdx_module(struct tdmr_info_list *tdmr_list,
>  				    u64 global_keyid)
>  {
> -	struct tdx_module_args args = {};
> -	u64 *tdmr_pa_array;
> +	struct tdmr_info_pa_array *tdmr_pa_array;
>  	size_t array_sz;
>  	int i, ret;
>  
> @@ -1021,12 +1043,10 @@ static __init int config_tdx_module(struct tdmr_info_list *tdmr_list,
>  		return -ENOMEM;


Outside the diff it has:
array_sz = tdmr_list->nr_consumed_tdmrs * sizeof(u64);

Could be now changed to:
array_sz = tdmr_list->nr_consumed_tdmrs * sizeof(*tdmr_pa_array->phys);

A bit of existing cleanup, but the u64 is especially tucked away compared to
before, so I'd argue its maintaining readability of the existing code.

>  
>  	for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++)
> -		tdmr_pa_array[i] = __pa(tdmr_entry(tdmr_list, i));
> +		tdmr_pa_array->phys[i] = __pa(tdmr_entry(tdmr_list, i));
>  
> -	args.rcx = __pa(tdmr_pa_array);
> -	args.rdx = tdmr_list->nr_consumed_tdmrs;
> -	args.r8 = global_keyid;
> -	ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> +	ret = tdx_sys_config(tdmr_pa_array, tdmr_list->nr_consumed_tdmrs,
> +			     global_keyid);
>  
>  	/* Free the array as it is not required anymore. */
>  	kfree(tdmr_pa_array);
> --
> 2.25.1