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

On Fri, Aug 21, 2026 at 08:53:57PM +0000, Edgecombe, Rick P wrote:
> 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()

Yeah, the PA array is an in-memory ABI, which is not constructed in the
newly introduced helper. The construction of an in-memory ABI usually
involves allocating/freeing memory, I remember there was an objection to
allocate memory inside the SEAMCALL helpers.

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

I think all the confusions come from the differences between "in-memory
ABI" and "register based ABI". The SEAMCALL leaf invoking is always the
register-based ABI. But Some registers reference some shared buffer
which contains the "in-memory ABI".

This patch introduces a helper to wrap the constructing of the
register-based ABI, but doesn't wrap the constructing of the in-memory
ABI, which should be the work of another helper if needed.

All helpers should take kernel data type as input. The previous
"u64 *tdmr_pa_array" is already a kernel data type to represent the
in-memory ABI that is referenced by the register-based ABI, but it is
easy to get confused (u64 * vs u64). So we define a named kernel
structure to clearly describe the in-memory ABI.

[...]

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

Yes, I'm good to the reason to wrap the register-based ABI.

>    
>    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

My reading of this paragraph is that we shouldn't wrap both in-memory ABI and
register-based ABI constructions in one helper, cause that may duplicate
allocations and copies. Because otherwise the combined helper should receive
a kernel managed buffer that represents the in-memory ABI but doesn't conform
to its layout, resulting in the extra allocation and copies in the helper.

Do we need to talk about this register-based/in-memory ABI pattern in general?
Not just for TDH.SYS.CONFIG. We already have exsiting examples and more to come,
this is not special to TDH.SYS.CONFIG.

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