| Message ID | 20180601145921.9500-4-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@oracle.com> > > --- > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: David Woodhouse <dwmw@amazon.co.uk> > Cc: Kees Cook <keescook@chromium.org> > Cc: KarimAllah Ahmed <karahmed@amazon.de> > --- > 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 think you will have to keep the case statements and explicitly check for X86_FEATURE_AMD_SSBD before using SPEC_CTRL. Thanks, Tom > x86_spec_ctrl_base |= SPEC_CTRL_SSBD; > x86_spec_ctrl_mask |= SPEC_CTRL_SSBD; > wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base); > - break; > } > } > >
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@oracle.com> > > > > --- > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Borislav Petkov <bp@suse.de> > > Cc: David Woodhouse <dwmw@amazon.co.uk> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: KarimAllah Ahmed <karahmed@amazon.de> > > --- > > 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 :-)
On 6/11/2018 9:01 AM, Konrad Rzeszutek Wilk wrote: > 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@oracle.com> >>> >>> --- >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: Ingo Molnar <mingo@redhat.com> >>> Cc: "H. Peter Anvin" <hpa@zytor.com> >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> Cc: Borislav Petkov <bp@suse.de> >>> Cc: David Woodhouse <dwmw@amazon.co.uk> >>> Cc: Kees Cook <keescook@chromium.org> >>> Cc: KarimAllah Ahmed <karahmed@amazon.de> >>> --- >>> 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 ? Whichever you feel is best, so long as we only use SPEC_CTRL for SSBD on AMD when X86_FEATURE_AMD_SSBD is present. Thanks, Tom > > > 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 :-) >
On Tue, 12 Jun 2018, Tom Lendacky wrote: > On 6/11/2018 9:01 AM, Konrad Rzeszutek Wilk wrote: > >> 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 ? > > Whichever you feel is best, so long as we only use SPEC_CTRL for SSBD on > AMD when X86_FEATURE_AMD_SSBD is present. Is there anyone working on a fix or has this been forgotten? Thanks, tglx
On Fri, Jun 15, 2018 at 08:57:40PM +0200, Thomas Gleixner wrote: > On Tue, 12 Jun 2018, Tom Lendacky wrote: > > On 6/11/2018 9:01 AM, Konrad Rzeszutek Wilk wrote: > > >> 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 ? > > > > Whichever you feel is best, so long as we only use SPEC_CTRL for SSBD on > > AMD when X86_FEATURE_AMD_SSBD is present. > > Is there anyone working on a fix or has this been forgotten? I have it on my TODO. But a bit busy with the fpu lazy so will get to it done next week. Maybe along with the X86_FEATURE_IA32_ARCH_CAPS Bit(1): IBRS_ALL to disable retpoline on those machines. CCing Alex > > Thanks, > > tglx
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 { x86_spec_ctrl_base |= SPEC_CTRL_SSBD; x86_spec_ctrl_mask |= SPEC_CTRL_SSBD; wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base); - break; } }
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@oracle.com> --- Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Borislav Petkov <bp@suse.de> Cc: David Woodhouse <dwmw@amazon.co.uk> Cc: Kees Cook <keescook@chromium.org> Cc: KarimAllah Ahmed <karahmed@amazon.de> --- arch/x86/kernel/cpu/bugs.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)