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

[Kai Huang <kai.huang@intel.com>]

On Tue, 2022-03-29 at 16:10 +1300, Kai Huang wrote:
> > > > 
> > > > *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).

Sorry typo.  Not "something error is discovered among cpus [0 - present_cpus)",
but "when there's any cpu within [0 - present_cpus) hasn't been brought up
once".
 
> 
> > 
> > > 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.
> 
>