Re: [PATCH 5/6] x86/virt/tdx: Make TDX module initialize the extensions

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

> > After providing all required memory to the TDX module, initialize TDX
> > module extensions via TDH.EXT.INIT
> > 
> 
> What exactly does TDH.EXT.INIT actually do,

The SPEC just said it "initializes TDX module extension". My
understanding is that it builds the SEAMCALL execution context by
setting up its internal data on the memory added by TDH.EXT.MEM.ADD

> and why can't whatever that is
> happen during TDH.EXT.MEM.ADD?

Do you mean why can't the extensions initialization happen on last
TDH.EXT.MEM.ADD? I can think of some case that TDH.EXT.MEM.ADD is not
needed but the extensions initialization is needed. For example: after
an compatible TDX module update.

I may ask the TDX module team to get a better understanding.

> Not saying we need to jam anything in awkwardly.
> Can we say this is analogous to TDH.SYS.CONFIG but for the extensions? It only
> exists because SYS.CONFIG gives the features to enable before the memory is
> there to actually do it. So it needs a second call?

I'm not sure I understand the question. But I think extension memory is
more like PAMT, and TDH.EXT.INIT is more like TDH.SYS.TDMR.INIT, is it?

				PAMT			the extensions
				====			==============
add memory			TDH.SYS.CONFIG		TDH.EXT.MEM.ADD
init memory for functionality	TDH.SYS.TDMR.INIT	TDH.EXT.INIT

> 
> 
> > , then those add-on features can use
> > their SEAMCALL leaves normally.
> 
> Out of curiosity, what happens if you don't complete these steps and call one of
> the extension seamcalls?

They return the error code - TDX_EXT_NOT_INITIALIZED.

> 
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> > Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Code looks good. Except that it leaves me wondering if this could have all been
> a while loop around tdh_sys_confg() that gives memory on a memory error code.
> Actually it looks like we have a TDX_EXT_MEMORY_POOL_REQUIRED error code to use.
> 
> Consider something like this:
> 
> do {
> 	ret = tdh_sys_init();
> 	if (ret == TDX_EXT_MEMORY_POOL_REQUIRED)
> 		tdh_ext_mem_add() //single page
> while (ret == TDX_EXT_MEMORY_POOL_REQUIRED)
> 
> If that could technically work, I guess the benefit of the current approach is
> that it allows to use contiguous physical allocations. Not sure if you think
> that simpler snippet would actually be that simple in the real world.

I think there are several simplifications here, let's break down:

 1. Forget about the 512-page limitation for hpa_list_info, always add 1 page
    at a time.
    This can also be applied to current flow. So put it aside.

 2. The loop strategy:
    - read total memory size	vs.	- loop on error code
    - prealloc all memory		  - alloc a page
    - loop on size			  - memory add
      - memory add

    The main saving is that we don't read memory_pool_required_pages any more.
    Others are similar lines of code.

 3. But we'd better keep reading ext_required. I tested when ext_required == 0:
      - tdh_sys_init() returns TDX_EXT_MEMORY_POOL_REQUIRED,
      - tdh_ext_mem_add() returns TDX_EXT_MEMORY_POOL_NOT_PENDING,
    So we need a weird return code combination to identify "extensions not
    required" then skip. And I don't know if the combination is stable,
    it is not specified in SPEC.

 4. If we still need to read at least one extensions metadata
    (ext_required), saving a memory_pool_required_pages read is not
    significant.

So generally I don't think the snippet would be that simple.

> But if we
> are basically building all this to help fragmentation, we should say it.
> 

[...]

> > +static __init int tdx_ext_init(void)
> > +{
> > +	struct tdx_module_args args = {};
> > +	u64 ret;
> > +
> > +	do {
> > +		ret = seamcall(TDH_EXT_INIT, &args);
> 
> What happens if no init is needed? Does it return success or error? I'm

It returns error - TDX_EXT_MEMORY_POOL_REQUIRED

> wondering if we need to really check ext_required and can just call TDH_EXT_INIT
> unconditionally. We need to check for TDX_FEATURES0_EXT in any case. But do we
> need both checks?

As mentioned above, we need a weird return code combination to identify
ext_required, so I think we should retain ext_required checking.