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

From: "Tian, Kevin" <kevin.tian@intel.com>
To: "Huang, Kai" <kai.huang@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: Mon, 28 Mar 2022 11:47:42 +0000	[thread overview]
Message-ID: <BN9PR11MB52761E8DE55DC8872EC093758C1D9@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <b4d97c46c52dbbecc6061f743b172015a73ec189.camel@intel.com>

> From: Huang, Kai <kai.huang@intel.com>
> Sent: Monday, March 28, 2022 5:24 PM
> >
> > cpu_present_mask does not always represent BIOS-enabled CPUs due
> > to those boot options. Then why do we care whether CPUs in this mask
> > (if only representing a subset of BIOS-enabled CPUs) are at least brought
> > up once? It will fail at TDH.SYS.CONFIG anyway.
> 
> As I said, this is used to make sure SEAMRR has been detected on all cpus, so
> that any BIOS misconfiguration on SEAMRR has been detected.  Otherwise,
> seamrr_enabled() may not be reliable (theoretically).

*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). If following the same
rationale then you also need a proper way to detect the case where nr_cpus
< BIOS enabled number i.e. when you cannot verify SEAMRR on CPUs
between [nr_cpus, 7]. otherwise this check is just incomplete.

But the entire check is actually unnecessary. You just need to verify SEAMRR
and do TDX cpu init on online CPUs. Any gap between online ones and BIOS
enabled ones will be caught by the TDX module at TDH.SYS.CONFIG point.

> 
> Alternatively, I think we can also add check to disable TDX when 'maxcpus'
> has
> been specified, but I think the current way is better.
> 
> >
> > btw your comment said that 'maxcpus' is basically an invalid mode
> > due to MCE broadcase problem. I didn't find any code to block it when
> > MCE is enabled,
> 
> Please see below comment in cpu_smt_allowed():
> 
> static inline bool cpu_smt_allowed(unsigned int cpu)
> {
> 	...
>         /*
>          * On x86 it's required to boot all logical CPUs at least once so
>          * that the init code can get a chance to set CR4.MCE on each
>          * CPU. Otherwise, a broadcasted MCE observing CR4.MCE=0b on any
>          * core will shutdown the machine.
>          */
> 	 return !cpumask_test_cpu(cpu, &cpus_booted_once_mask);
> }

I saw that code. My point is more about your statement that maxcpus
is almost invalid due to above situation then why didn't we do anything
to document such restriction or throw out a warning when it's
misconfigured...

> 
> > thus wonder the rationale behind and whether that
> > rationale can be brought to this series (i.e. no check against those
> > conflicting boot options and just let SEAMCALL itself to detect and fail).
> >
> > @Thomas, any guidance here?
> >
> > Thanks
> > Kevin