Re: [PATCH v2 08/31] x86/virt/tdx: Configure TDX Module with optional TDX Connect feature

[Huang, Kai]

>  static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
>  {
>  	struct tdx_module_args args = {};
> +	u64 seamcall_fn = TDH_SYS_CONFIG_V0;
>  	u64 *tdmr_pa_array;
>  	size_t array_sz;
>  	int i, ret;
> @@ -1377,7 +1378,15 @@ static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
>  	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();
> +		/* These parameters requires version >= 1 */
> +		seamcall_fn = TDH_SYS_CONFIG;
> +	}
> +
> +	ret = seamcall_prerr(seamcall_fn, &args);
>  
>  	/* Free the array as it is not required anymore. */
>  	kfree(tdmr_pa_array);
> @@ -1537,6 +1546,11 @@ static 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)

Maybe a more generic comment:

I don't quite like hard-coding opt-in TDX_FEATURES0_TDXCONNECT inside
config_tdx_module(), especially currently we just unconditionally opt it in
if the module support this feature.

Initializing TDX Connect (and other features via TDX Module Extensions)
consumes more memory.  It would be better if we can choose to opt-in when
the kernel has enabled TDX Connect (or any other feature via TDX module
Extensions) in the Kconfig.

Unfortunately we need to opt-in all these features together during module
initialization, so we cannot make tdx_enable() to accept the additional
features to enable, and in each in-kernel TDX user, call tdx_enable() with
the new feature that that TDX user concerns.

But I think it makes sense to have a dedicated place to calculate all opt-in
features.  E.g., assuming we eventually are going to support TDX Connect and
live migration:

static u64 get_ext_features_tdx_connect(struct tdx_sys_info * sysinfo)
{
	if (!IS_ENABLED(TDX_CONNECT))
		return 0;

	return sysinfo->features.tdx_features0 & TDX_FEATURES0_TDXCONNECT ?
		TDX_FEATURES0_TDXCONNECT : 0;
}

static u64 get_ext_features_live_migration(struct tdx_sys_info *sysinfo)
{
	u64 mig_features = TDX_FEATURES0_NRX | TDX_FEATURES0_NON_BLOCKING;

	if (!IS_ENABLED(TDX_LIVE_MIGRATION))
		return 0;

	return sysinfo->features.tdx_features0 & mig_features;
}

static u64 calculate_ext_features(struct tdx_sys_info *sysinfo)
{
	u64 ext_features = 0;

	ext_features |= get_ext_features_tdx_connect(sysinfo);

	ext_features |= get_ext_features_live_migration(sysinfo);

	return ext_features;
}

int init_tdx_module()
{
	u64 ext_features = calculate_ext_features(&tdx_sysinfo);

	ret = config_tdx_module(&tdx_tdmr_list, &tdx_global_keyid,
				ext_features);

	/* do other initializations like TDH.SYS.KEY.CONFIG */
	...
	/*
	 * TDX Module Extension features must be initialized
	 * after TDH.SYS.KEY.CONFIG.
	 */
	if (ext_features)
		ret = init_tdx_ext();	

	...
}

One nasty thing is per public spec R11 of TDH.SYS.CONFIG needs to be RTC if
TDX_CONNECT is on, so we still need some special handing in
config_tdx_module():

	if (ext_features & TDX_RFEAURES0_TDXCONNECT)
		args.r11 = ktime_get_real_seconds();

But I think this is acceptable.