Re: [PATCH v7 04/20] x86/virt/tdx: Add skeleton to initialize TDX on demand

[Thomas Gleixner]

On Tue, Nov 22 2022 at 07:35, Dave Hansen wrote:

> On 11/22/22 02:31, Thomas Gleixner wrote:
>> Nothing in the TDX specs and docs mentions physical hotplug or a
>> requirement for invoking seamcall on the world.
>
> The TDX module source is actually out there[1] for us to look at.  It's
> in a lovely, convenient zip file, but you can read it if sufficiently
> motivated.

zip file? Version control from the last millenium?

The whole thing wants to be @github with a proper change history if
Intel wants anyone to trust this and take it serious.

/me refrains from ranting about the outrageous license choice.

> It has this lovely nugget in it:
>
> WARNING!!! Proprietary License!!  Avert your virgin eyes!!!

It's probably not the only reasons to avert the eyes.

>>     if (tdx_global_data_ptr->num_of_init_lps < tdx_global_data_ptr->num_of_lps)
>>     {
>>         TDX_ERROR("Num of initialized lps %d is smaller than total num of lps %d\n",
>>                     tdx_global_data_ptr->num_of_init_lps, tdx_global_data_ptr->num_of_lps);
>>         retval = TDX_SYS_CONFIG_NOT_PENDING;
>>         goto EXIT;
>>     }
>
> tdx_global_data_ptr->num_of_init_lps is incremented at TDH.SYS.INIT
> time.  That if() is called at TDH.SYS.CONFIG time to help bring the
> module up.
>
> So, I think you're right.  I don't see the docs that actually *explain*
> this "you must seamcall all the things" requirement.

The code actually enforces this.

At TDH.SYS.INIT which is the first operation it gets the total number
of LPs from the sysinfo table:

src/vmm_dispatcher/api_calls/tdh_sys_init.c:

    tdx_global_data_ptr->num_of_lps = sysinfo_table_ptr->mcheck_fields.tot_num_lps;

Then TDH.SYS.LP.INIT increments the count of initialized LPs.

src/vmm_dispatcher/api_calls/tdh_sys_lp_init.c:

    increment_num_of_lps(tdx_global_data_ptr)
       _lock_xadd_32b(&tdx_global_data_ptr->num_of_init_lps, 1);

Finally TDH.SYS.CONFIG checks whether _ALL_ LPs have been initialized.

src/vmm_dispatcher/api_calls/tdh_sys_config.c:

    if (tdx_global_data_ptr->num_of_init_lps < tdx_global_data_ptr->num_of_lps)

Clearly that's nowhere spelled out in the documentation, but I don't
buy the 'architecturaly required' argument not at all. It's an
implementation detail of the TDX module.

Technically there is IMO ZERO requirement to do so.

 1) The TDX module is global

 2) Seam-root and Seam-non-root operation are strictly a LP property.

    The only architectural prerequisite for using Seam on a LP is that
    obviously the encryption/decryption mechanics have been initialized
    on the package to which the LP belongs.

I can see why it might be complicated to add/remove an LP after
initialization fact, but technically it should be possible.

TDX/Seam is not that special.

But what's absolutely annoying is that the documentation lacks any
information about the choice of enforcement which has been hardcoded
into the Seam module for whatever reasons.

Maybe I overlooked it, but then it's definitely well hidden.

Thanks,

        tglx