Re: [PATCH v3 2/6] x86/virt/tdx: Configure add-on features on TDX module init

[Xu Yilun <yilun.xu@linux.intel.com>]

On Tue, Oct 06, 2026 at 11:56:23PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2026-10-06 at 01:41 +0800, Xu Yilun wrote:
> > The TDX architecture identifies some features that are off by default
> > but can be enabled by the host during TDX module initialization. They
> > are classified as add-on features because enabling them affects existing
> > TDX systems: they may change existing feature behavior, or reserve more
> > memory.
> > 
> > The TDX module extends TDH.SYS.CONFIG with a new register argument to
> > specify which add-on features to enable. This new argument is a bitmap
> > that uses the same feature bits as TDX_FEATURES0. Note that Dynamic PAMT
> > is an exception: although it is an add-on feature, it is controlled via
> > a legacy, dedicated register argument [1].
> > 
> > The kernel needs to enable these add-on features when it supports them.
> 
> I made a similar comment on v1:
> https://lore.kernel.org/lkml/41f5a558ca67e2895fcb114c418f0e453e933974.camel@intel.com/
> 
> It is up to the kernel to decide if it wants to enable these features, when it
> supports them. The sentence sounds like there is some hard requirement to enable
> them when possible. I think the reasons to enable them based solely on available
> support could be:
>  1. They don't take significantly more memory or CPU resources, and limiting
> possible configs reduces kernel complexity.
>  2. It would be rare that people don't want them on.
> 
> Maybe we tweak this to be?
> 
> Despite that these features can be add-ons due to introducing some overhead to
> TDX runtime, the impact of the first features will be low enough to pursue a
> simple approach of just enabling them when supported. If a feature with a very
> high overhead appears in the future, this can be revisited.

Good to me. I'll include it in v4.

[...]

> > @@ -1056,6 +1066,15 @@ static __init int tdx_sys_config(struct tdmr_info_pa_array *tdmr_pa_array,
> >  		args.r8 |= TDX_SYS_CONFIG_DYNAMIC_PAMT;
> >  	}
> >  
> > +	/*
> > +	 * Use SEAMCALL version 1 that supports add-on features if any are
> > +	 * requested. Otherwise use version 0 for backward compatibility.
> > +	 */
> > +	if (addon_features0) {
> > +		args.r9 = addon_features0;
> > +		args.version = 1;
> 
> This is a slightly silly pattern:
> args.r9 = 0;
> if (addon_features0 != 0)
> 	args.r9 = addon_features0;
> 
> ...could just be:
> args.r9 = addon_features0;

Yes, that's workable.

> 
> It could actually set args.version a similar branchless way.

Not sure how?

I don't think just "args.version = 1" can work. It will fail older
modules which doesn't support add-on features & doesn't recognize
TDH.SYS.CONFIG v1.

How about:

	struct tdx_module_args args = {
		.rcx = __pa(tdmr_pa_array),
		.rdx = nr_tdmr_pa,
		.r8 = global_keyid,
		.r9 = addon_features0,
	};

	...

	if (args.r9)
		args.version = 1;

> 
> But I think the version in this patch is super obvious about what is happening
> and has a nice place for the comment. And this is extremely far from a
> performance critical path.
> 
> 
> > +	}
> > +
> >  	return seamcall_prerr(TDH_SYS_CONFIG, &args);
> >  }
> >  
> > 
>