> 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.
This is equivalent to not having the paranoid check in seamrr_enabled(). By
detecting SEAMRR in identify_cpu(), the detection has already been done for any
online cpu.
>
> >
> > 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...
The sentence "'maxcpus' is an invalid operation mode due to the MCE broadcast
problem" was from Thomas. Perhaps I should not just put it into the comment.
Also, Thomas suggested:
"you should have a paranoia check which checks for the maxcpus
command line parameter and if it's there and smaller than the number of
present CPUs then you just refuse to enable TDX.
Alternatively you just have a separate cpumask tdx_availabe_mask and
keep track of the CPUs which have been checked. When TDX is initialized
you then can do:
if (!cpumask_equal(cpu_present_mask, tdx_available_mask))
return;
"
I found we can just use cpus_booted_once_mask, instead of tdx_available_mask, so
I used the second way. And instead of putting the check when initializing TDX,
I put to seamrr_enabled() since I guess it's more reasonable to be here as the
logic is to make sure SEAMRR has been detected on all cpus.
Hi Thomas,
If you see this, sorry for quoting your words here. Just want to have a better
discussion. And appreciate if you can have some guidance here.
>
> >
> > > 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
>
