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

From: "Huang, Kai" <kai.huang@intel.com>
To: "tglx@linutronix.de" <tglx@linutronix.de>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Hansen, Dave" <dave.hansen@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	"bagasdotme@gmail.com" <bagasdotme@gmail.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Christopherson,, Sean" <seanjc@google.com>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v7 04/20] x86/virt/tdx: Add skeleton to initialize TDX on demand
Date: Wed, 23 Nov 2022 00:30:58 +0000	[thread overview]
Message-ID: <246a4eaac29855c522bd26627b03418cb7ead66f.camel@intel.com> (raw)
In-Reply-To: <87mt8ig3ja.ffs@tglx>

On Tue, 2022-11-22 at 21:03 +0100, Thomas Gleixner wrote:
> > >      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.

Hi Thomas,

Thanks for review!

I agree on hardware level there shouldn't be such requirement (not 100% sure
though), but I guess from kernel's perspective, "the implementation detail of
the TDX module" is sort of "architectural requirement" -- at least Intel arch
guys think so I guess.

> 
> 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.

"kernel soft offline" actually isn't an issue.  We can bring down a logical cpu
after it gets initialized and then bring it up again.

Only add/removal of physical cpu will cause problem: 

TDX MCHECK verifies all boot-time present cpus to make sure they are TDX-
compatible before it enables TDX in hardware.  MCHECK cannot run on hot-added
CPU, so TDX cannot support physical CPU hotplug.

We tried to get it clarified in the specification, and below is what TDX/module
arch guys agreed to put to the TDX module spec (just checked it's not in latest
public spec yet, but they said it will be in next release):

"
4.1.3.2.  CPU Configuration

During platform boot, MCHECK verifies all logical CPUs to ensure they meet TDX’s
security and certain functionality requirements, and MCHECK passes the following
CPU configuration information to the NP-SEAMLDR, P-SEAMLDR and the TDX Module:

·         Total number of logical processors in the platform.
·         Total number of installed packages in the platform.
·         A table of per-package CPU family, model and stepping etc.
identification, as enumerated by CPUID(1).EAX.
The above information is static and does not change after platform boot and
MCHECK run.

Note:     TDX doesn’t support adding or removing CPUs from TDX security
perimeter, as checked my MCHECK.  BIOS should prevent CPUs from being hot-added
or hot-removed after platform boots.

The TDX module performs additional checks of the CPU’s configuration and
supported features, by reading MSRs and CPUID information as described in the
following sections.
"

> 
> 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.

It depends on the TDX Module implementation, which TDX arch guys think should be
"architectural" I think.