Re: [PATCH v2 04/21] x86/virt/tdx: Add skeleton for detecting and initializing TDX on demand

From: Kai Huang <kai.huang@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Gleixner, Thomas" <thomas.gleixner@intel.com>
Cc: "Hansen, Dave" <dave.hansen@intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>
Subject: Re: [PATCH v2 04/21] x86/virt/tdx: Add skeleton for detecting and initializing TDX on demand
Date: Tue, 29 Mar 2022 16:10:46 +1300	[thread overview]
Message-ID: <15c0212fc4ff591a878369ea6ae964e43574a8d9.camel@intel.com> (raw)
In-Reply-To: <BN9PR11MB5276C837FE25BACCD53DB5D58C1E9@BN9PR11MB5276.namprd11.prod.outlook.com>

> > > 
> > > *all cpus* is questionable.
> > > 
> > > Say BIOS enabled 8 CPUs: [0 - 7]
> > > 
> > > cpu_present_map covers [0 - 5], due to nr_cpus=6
> > > 
> > > You compared cpus_booted_once_mask to cpu_present_mask so if
> > maxcpus
> > > is set to a number < nr_cpus SEAMRR is considered disabled because you
> > > cannot verify CPUs between [max_cpus, nr_cpus).

Sorry my bad.  We can verify CPUs between [max_cpus, nr_cpus).  When any cpu
within that range becomes online, the detection code is run.  The paranoid check
in seamrr_enabled() is used to check whether all cpus within [max_cpus, nr_cpus)
(if there's any -- cpus within [0, max_cpus) are up during boot) have been
brought up at least once. 

> > 
> > SEAMRR is not considered as disabled in this case, at least in my intention.
> 
> the function is called seamrr_enabled(), and false is returned if above
> check is not passed. So it is the intention from the code.

The false is returned if something error is discovered among cpus [0 -
present_cpus].  It returns true even if we cannot verify [present_cpus,
bios_enabled_cpus).

> 
> > My
> > understanding on the spec is if SEAMRR is configured as enabled on one core
> > (the
> > SEAMRR MSRs are core-scope), the SEAMCALL instruction can work on that
> > core.  It
> > is TDX's requirement that some SEAMCALL needs to be done on all BIOS-
> > enabled
> > CPUs to finish TDX initialization, but not SEAM's.
> > 
> > From this perspective, if we forget TDX at this moment but talk about SEAM
> > alone, it might make sense to not just treat SEAMRR as disabled if kernel
> > usable
> > cpus are limited by 'nr_cpus'.  The chance that BIOS misconfigured SEAMRR is
> > really rare.  If kernel can detect potential BIOS misconfiguration, it
> > should do
> > it.  Otherwise, perhaps it's more reasonable not to just treat SEAM as
> > disabled.
> 
> My problem is just that you didn't adopt consistency policy for CPUs in
> [maxcpus, nr_cpus) and CPUs in [nr_cpus, nr_bios_enabled_cpus). This is
> the only trouble to me no matter what policy you want to pursue...

Let's separate SEAMRR detection and TDX initialization.  The paranoid check is
only for SEAM detection, but not for TDX initialization.  As I said, it is TDX's
requirement that some SEAMCALL must be run on all bios-enabled cpus, but not
SEAM's.

-- 
Thanks,
-Kai