On Mon, Oct 18, 2021 at 09:06:07PM +0100, Andrew Cooper wrote:
> ... this is 0x18 for Hygon, and ...
Sure, whatever :)
> >
> > /* All the remaining ones are affected */
> > set_cpu_bug(c, X86_BUG_NULL_SEG);
>
> ... hypervisor && !ncsb still needs to set BUG_NULL_SEG, so you really
> can't exit early.
Yeah, we had a session on IRC, we came up with this rough version, more
polishing tomorrow:
static void early_probe_null_seg_clearing_base(struct cpuinfo_x86 *c)
{
/* Zen3 CPUs advertise Null Selector Clears Base in CPUID. */
if (c->extended_cpuid_level >= 0x80000021 && cpuid_eax(0x80000021) & BIT(6))
return;
/*
* CPUID bit above wasn't set. If this kernel is still running as a HV guest,
* then the HV has decided not to advertize that CPUID bit for whatever reason.
* For example, one member of the migration pool might be vulnerable.
* Which means, the bug is present: set the BUG flag and return.
*/
if (cpu_has(c, X86_FEATURE_HYPERVISOR)) {
set_cpu_bug(c, X86_BUG_NULL_SEG);
return;
}
/* Zen2 CPUs also have this behaviour, but no CPUID bit. 0x18 for Hygon. */
if ((c->x86 == 0x17 || c->x86 == 0x18) && check_null_seg_clears_base(c))
return;
/* All the remaining ones are affected */
set_cpu_bug(c, X86_BUG_NULL_SEG);
}
So I really want to have those comments explaining each step in the
complex check because we will forget why this crazy dance is being done
and as I said in a previous thread, we're not all virtualizers. :)
> No other CPU vendors are known to have this issue.
How do you know? Or should there be a comment along the lines of "Cooper
says that..."
:-)
> (And by "issue", even this is complicated. Back in the 32bit
> days, it was a plausible perf improvement, but it backfired massively
> for AMD64 where there was a possibility/expectation to use NULL
> segments.)
>
> Andy only put the check in unilaterally just in case, and even that was
> fine-ish until AMD went and fixed it silently in Zen2.
Yeah, there's the context switch overhead too but that's for another
thread.
> > Because if this null seg behavior detection should happen on all
> > CPUs - and I think it should, because, well, it has been that way
> > until now - then the vendor specific identification minus what
> > detect_null_seg_behavior() does should run first and then after
> > ->c_identify() is done, you should do something like:
> >
> > if (!cpu_has_bug(c, X86_BUG_NULL_SEG)) { if
> > (!check_null_seg_clears_base(c)) set_cpu_bug(c, X86_BUG_NULL_SEG);
> > }
> >
> > so that it still takes place on all CPUs.
>
> This would only really work for boot cpu and setup_force_cap(),
> because no CPU is going to have X86_BUG_NULL_SEG set by
> default, but this still misses the point of the bugfix which
> is "check_null_seg_clears_base() must not be called when
> cpu_has_hypervisor".
>
> In practice, the BSP is good enough. The behaviour predates the
> K8, which was the first CPU where it became observable without
> SMM/PUSHALL/etc, and quite possibly goes back to the dawn of time, and
> you can't mix a Zen1 and Zen2 in a 2-socket system.
Oh, I didn't express myself properly "should happen on all CPUs" was
supposed to mean, if this detection should happen on all vendors like it
does now. Not BSP vs AP.
> It is made unused by this patch, so can't be pulled out earlier, but
> should be adjusted.
Right.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
