Re: [PATCH v2 02/17] x86/virt/tdx: Configure add-on features on TDX module init and update

[Xu Yilun]

> >For runtime update, Linux applies a policy that no newer features should
> >be added after update to avoid disrupting live TDX operations. To adhere
> >to this, TDH.SYS.UPDATE must configure the same features as the
> >TDH.SYS.CONFIG. Record the kernel required add-on feature bitmap in a
> >global var so that both phases can use it.
> 
> Actually, we do not need another global variable here. tdx_features0 is cached
> and is not updated across a runtime update, so the derived add-on feature
> bitmap will be the same before and after the update.

I think a global var "static u64 tdx_addon_feature0 *__ro_after_init*;"
better illustrates the policy that add-on feature bitmap should be decided at
boot up and never change later. It will also be used to decide if a specific
add-on feature initialization is needed. We don't want to calculate the bitmap
again and again, though the result must be the same.

Maybe I should strenghthen the commit message:

  ... both phases can use it. This actually mirrors a TDX module internal state
  so that kernel knows which add-on TDX operations (for example, quoting
  SEAMCALLs, which will be added in later patches) are valid.

> 
> 
> > static __init int config_tdx_module(struct tdmr_info_list *tdmr_list,
> > 				    u64 global_keyid)
> > {
> >+	u64 seamcall_fn = TDH_SYS_CONFIG_V0;
> > 	struct tdx_module_args args = {};
> > 	u64 *tdmr_pa_array;
> > 	size_t array_sz;
> >@@ -1032,7 +1042,15 @@ static __init int config_tdx_module(struct tdmr_info_list *tdmr_list,
> > 	args.rcx = __pa(tdmr_pa_array);
> > 	args.rdx = tdmr_list->nr_consumed_tdmrs;
> > 	args.r8 = global_keyid;
> >-	ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> >+
> >+	set_tdx_addon_features();
> >+
> >+	if (tdx_addon_feature0) {
> >+		args.r9 = tdx_addon_feature0;
> 
> How about moving this r9 assignment out of the if block and placing it next to
> 'args.r8 = global_keyid;'? There is no need to guard it, because args.r9 will
> be 0 when no add-on features are enabled, which is perfectly fine.

I tend to keep r9 assignment in the block, it clearly shows which
SEAMCALL version needs what parameters, help people map the code to TDX
module spec.

> 
> >+		seamcall_fn = TDH_SYS_CONFIG;
> >+	}
> >+
> >+	ret = seamcall_prerr(seamcall_fn, &args);