Re: [PATCH 1/6] x86/virt/tdx: Wrap TDH.SYS.CONFIG/UPDATE operations in helpers

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

On Fri, 2026-08-21 at 11:29 +0800, Xu Yilun wrote:
> As part of the TDX module initialization, the kernel configures the TDX
> module with several information
> 

The tense is weird here around "several information". Should be "several pieces
of information". For curiosity sake, I looked it up and found a new-to-me
linguistic term:
https://en.wikipedia.org/wiki/Mass_noun

> , such as TDX-usable memory regions
> (TDMRs) and the global KeyID for protecting TDX metadata. During the
> configuration, the kernel does 2 operations: constructing kernel data
> types for the SEAMCALL leaf arguments and turning these data types into
> u64's according to TDX ABI.

What do you mean by "constructing kernel data types for the SEAMCALL leaf
arguments"? You are splitting the calculation of the pa via offset from setting
it in a u64? If that is what you are saying, it makes it seem like a much bigger
set of work.

> 
> Both operations are implemented in one function - config_tdx_module().

Well I guess my guess above was wrong, because the construction of the pa kernel
data type happens in tdmr_entry()

> This blurs the boundary between kernel managed structures and TDX ABI
> definitions.
> 

In the code after this patch it is still setting a u64 in config_tdx_module()...
so what is really changed with respect to this?

>  Moreover, future kernel change will need to add more
> nuances mandated by TDX ABI, such as setting the TDH.SYS.CONFIG leaf
> version to allow configuring add-on features. Keeping these operations
> tangled would further clutter the code.

This seems like a stronger reason to me.

> 
> Just like other SEAMCALL leaf helpers, wrap the invocation of
> TDH.SYS.CONFIG in a helper. Introduce a more descriptive kernel data
> type for the physical address array of TDMR information. This data type
> is similar to struct seamldr_params in that it is the container of the
> PA array layout which is an in-memory ABI. So the previous u64 * type
> for the array is not wrong, but a named structure provides better type
> safety and self-documentation. Use the data type as the argument of the
> TDH.SYS.CONFIG helper.
> 
> Future kernel change will also need to set the TDH.SYS.UPDATE leaf
> version for the same purpose. Add a similar helper to prepare for the
> change.
> 
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>

The change looks good to me, but I'm wondering if it will need a better
justification. How about something that hits these points:

   We have SEAMCALL wrappers mainly to not expose broad seamcall access, by
   exporting only a selection of seamcalls, but also to abstract the seamcall
   register ABIs. The latter improves readability and re-use for seamcalls that
   are made multiple times.
   
   Some seamcalls leafs are not explicitly wrapped because the level of TDX ABI
   details needed to perform the call is low enough that it can flow well enough
   with the calling code. 
   
   For some of the currently unwrapped seamcall leafs, future changes will add
   seamcall version selection that will adjust the ABI depending on TDX module
   version support. This will result in more ABI details to surrounding caller
   code and decrease readability of the other logic. To contain this, wrap the
   functions that will get version selections in seamcall wrappers.
   
   The cleanest separation would be to have kernel data types for the seamcall
   wrapper args, and have them marshaled into SEAMCALL ABI types (often u64s)
   inside the wrappers. But to avoid duplicating allocations and copies, don't
   do this when creating the wrapper for TDH.SYS.CONFIG. Instead clarify that
   the u64's in the array passed are pa's with explicit naming of the helper
   struct.
   

It's a bit rough, but as a general argument for the change, does it seem better?