Re: [PATCH v1 3/3] x86/bugs: Switch the selection of mitigation from CPU vendor to CPU features

[Konrad Rzeszutek Wilk]

On Fri, Jun 08, 2018 at 04:30:15PM -0500, Tom Lendacky wrote:
> On 6/1/2018 9:59 AM, Konrad Rzeszutek Wilk wrote:
> > Both AMD and Intel can have SPEC CTRL MSR for SSBD.
> > 
> > However AMD also has two more other ways of doing it - which
> > are !SPEC_CTRL MSR ways.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > 
> > ---
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxx>
> > Cc: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: KarimAllah Ahmed <karahmed@xxxxxxxxx>
> > ---
> >  arch/x86/kernel/cpu/bugs.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index 6bea81855cdd..cd0fda1fff6d 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -532,17 +532,12 @@ static enum ssb_mitigation __init __ssb_select_mitigation(void)
> >  		 * Intel uses the SPEC CTRL MSR Bit(2) for this, while AMD may
> >  		 * use a completely different MSR and bit dependent on family.
> >  		 */
> > -		switch (boot_cpu_data.x86_vendor) {
> > -		case X86_VENDOR_INTEL:
> > -		case X86_VENDOR_AMD:
> > -			if (!static_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
> > -				x86_amd_ssb_disable();
> > -				break;
> > -			}
> > +		if (!static_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
> > +			x86_amd_ssb_disable();
> > +		else {
> 
> As I think about this more, I don't think we can do this for AMD.  The
> X86_FEATURE_SSBD could be true because of the LS_CFG support and not the
> AMD_SSBD CPUID bit.  But if the IBRS CPUID bit was set, we would now try
> to use the SPEC_CTRL register for SSBD, which is not valid.

I was reading the AMD docs and while the SPEC CTRL provides IBRS my understanding
(from reading between the lines) is that AMD would actually never implement this.

That is it would have the 'Enhanced IBRS' bit exposed if at all, but nothing else.

Granted this is tea-reading at its best so, ..
> 
> I think you will have to keep the case statements and explicitly check for
> X86_FEATURE_AMD_SSBD before using SPEC_CTRL.

.. we could or alternatively add an extra check for X86_FEATURE_AMD_SSBD ?


I think Thomas already sent this out but it should be no problems to
add a fix as there is no hardware with this so it isn't like we are
breaking anything :-)