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

> >  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.
> > 
> > TDX module advances the version of TDH.SYS.CONFIG and TDH.SYS.UPDATE for
> > the change, so use the latest version (v1) for add-on feature enabling.
> > But supporting existing modules which only support v0 is still necessary
> > until they are deprecated. In fact, it is unlikely that TDH.SYS.CONFIG
> > ever needs to change again and the code would stay in v1.
> 
> Ha, these are hard things to predict.
> 
> >  So there is
> > little value in worrying about deprecating v0 to save a couple lines of
> > code in 5-7 years when these original TDX platforms sunset.
> 
> We can't deprecate it today, so what is this saying exactly?

This paragraph is outdated, at that time the target is to always use the
latest SEAMCALL version. I'll remove it.

I updated the changelog according to these comments, appended at the
end.

[...]

> > +static __init void set_tdx_addon_features(void)
> > +{
> > +	/*
> > +	 * To add DICE-based TDX Quoting feature bit in tdx_addon_feature0
> > when
> > +	 * kernel is ready.
> > +	 */
> 
> Can we not make this DICE specific?

Yes.

> 
> > +}
> > +
> >  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();
> 
> This is a little opaque. Why not something like
> 
> tdx_addon_feature0 = TDX_KERNEL_SUPPORTED_ADDONS &
> tdx_sysinfo.features.tdx_features0;

Good to me.

> 
> tdx_addon_feature0 could probably be named better too. It means the features
> that the kernel has decided to use. HMm, any ideas?

How about tdx_supported_features0?

BTW: I'm going to take Chao's advice [1] to remove the global var
tdx_addon_feature0, and use a "static u64 get_tdx_supported_features0(void)"
for TDH.SYS.CONFIG/UPDATE.

> 
> > +
> > +	if (tdx_addon_feature0) {
> > +		args.r9 = tdx_addon_feature0;
> > +		seamcall_fn = TDH_SYS_CONFIG;
> > +	}
> 
> This kind of feels hacky to me. tdx_addon_feature0 is zero if there are none
> supported. It shouldn't interfere with the v0 call to set it unconditionally?
> 
> I think the connection between tdx_addon_feature0 and TDH_SYS_CONFIG is a bit
> tenuous too. It is set if a extension is found that the kernel supports.
> 
> What do you think about wrapping TDH_SYS_CONFIG. Have the wrapper take all the
> args including tdx_addon_feature0. Then have the wrapper decide which seamcall
> to use. Have a nice comment that says "Use version of seamcall that supports add
> ons if we have any, use the old one if none are supported for backward
> compatibility".

I'm good to add the wrapping for TDH_SYS_CONFIG and TDH_SYS_UPDATE.

> 
> BTW, looking at the tdx module, if I'm reading this right, r9 used to be
> ignored. So did it need a new seamcall version? Some other VMM was passing
> garbage?

Yes, I think it prevents possible garbage inputs suddenly take effect.

The updated changelog:

----8<----

    x86/virt/tdx: Set configurable features on TDX module init and update

    TDX module identifies some configurable features that must be
    explicitly enabled as the kernel supports them. These features affect
    existing TDX systems: they may change the behaviors of the existing
    features, reserve more memory, or impact the TDX initialization
    performance. The kernel needs to explicitly enable these features at
    boot or post-update initialization time.

    Features such as TDISP, DICE-based quoting and TD migration are among
    those configurable features. They use preemptible SEAMCALLs so require
    extra memory and setup time to build a global service environment - the
    TDX module extensions. Add support for the configurable feature setting
    as the prerequisite for initializing TDX module extensions.

    TDX module extends TDH.SYS.CONFIG and TDH.SYS.UPDATE with new bitmap
    input parameters to specify which features to enable. The bitmap uses
    the same definitions as TDX_FEATURES0. TDX module advances the version
    of TDH.SYS.CONFIG and TDH.SYS.UPDATE for the enhancement. A previous
    change [1] adds the 'version' field to struct tdx_module_args to support
    versioned SEAMCALLs. Assign the version field to 1 if any configurable
    feature is required.

    Add a get_tdx_addon_features0() helper to return the bitmap of the
    configurable features that the module & kernel both support. Initially,
    this helper returns 0. It will be updated to return specific feature
    bits as full kernel support lands. TDH.SYS.CONFIG consumes this bitmap
    during TDX initialization.

    Runtime updates keep the reported features unchanged across updates, so
    that existing TDX users can continue to operate without disruption. To
    adhere to this, provide TDH.SYS.UPDATE with the same bitmap returned by
    get_tdx_addon_features0(). The bitmap is guaranteed to match the initial
    TDH.SYS.CONFIG input because tdx_sysinfo.features.tdx_features0 is
    cached and not refreshed after update.

    Link: https://lore.kernel.org/all/[email protected]/ # [1]