| Message ID | 20161109132114.3ujq2wkhsm4kcytz@pd.tnic (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 9 Nov 2016, Borislav Petkov wrote: > On Tue, Nov 08, 2016 at 09:06:31PM +0100, Thomas Gleixner wrote: > > The upcoming ring3 mwait stuff can add its magic to tweak that MSR into > > this function. > > > > Stick the call at the end of init_scattered_cpuid_features() for now. I > > still need to figure out a proper place for it. > > So Thomas and I discussed this more on IRC and I think we can get rid > of the MSR iterating in scattered.c and integrate both the R3 MWAIT and > CPUID faulting like this: I agree mostly. > --- > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index fcd484d2bb03..5c38a85af2e7 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -452,6 +457,39 @@ static void intel_bsp_resume(struct cpuinfo_x86 *c) > init_intel_energy_perf(c); > } > > +static void init_misc_enables(struct cpuinfo_x86 *c) > +{ > + u64 val, misc_en; > + > + if (rdmsrl_safe(MSR_MISC_FEATURES_ENABLES, &misc_en)) > + return; > + > + misc_en &= ~MSR_MISC_ENABLES_CPUID_FAULT_ENABLE; I rather prefer to write this MSR to 0 right away and just enable the bits which we really support. Whatever Intel comes up with next, e.g. faulting of random other instructions or whatever (mis)feature they think is valuable, will lead to a debugging nightmare if some incompetent BIOS writer sets the bit and the kernel does not know about it. Yes, I know that there might be bits forced to 1 at some point in the future, but let's deal with that when it happens. Right now I can enable the CPUID FAULT bit on my broadwell and watch user space programs die unexpectedly without a hint why. Simply because it's not documented in the SDM. So we rather be safe than surprised. @hpa: Thoughts? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 9, 2016 at 5:21 AM, Borislav Petkov <bp@suse.de> wrote: > On Tue, Nov 08, 2016 at 09:06:31PM +0100, Thomas Gleixner wrote: >> The upcoming ring3 mwait stuff can add its magic to tweak that MSR into >> this function. >> >> Stick the call at the end of init_scattered_cpuid_features() for now. I >> still need to figure out a proper place for it. > > So Thomas and I discussed this more on IRC and I think we can get rid > of the MSR iterating in scattered.c and integrate both the R3 MWAIT and > CPUID faulting like this: > > --- > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index fcd484d2bb03..5c38a85af2e7 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -452,6 +457,39 @@ static void intel_bsp_resume(struct cpuinfo_x86 *c) > init_intel_energy_perf(c); > } > > +static void init_misc_enables(struct cpuinfo_x86 *c) > +{ > + u64 val, misc_en; > + > + if (rdmsrl_safe(MSR_MISC_FEATURES_ENABLES, &misc_en)) > + return; > + > + misc_en &= ~MSR_MISC_ENABLES_CPUID_FAULT_ENABLE; > + > + if (!rdmsrl_safe(MSR_PLATFORM_INFO, &val)) { > + if (val & PLATINFO_CPUID_FAULT_BIT) > + set_cpu_cap(c, X86_FEATURE_CPUID_FAULT); > + } > + > + wrmsrl(MSR_MISC_FEATURES_ENABLES, misc_en); > + this_cpu_write(msr_misc_features_enables_shadow, misc_en); > +} > + > static void init_intel(struct cpuinfo_x86 *c) > { > unsigned int l2 = 0; > @@ -565,6 +603,8 @@ static void init_intel(struct cpuinfo_x86 *c) > detect_vmx_virtcap(c); > > init_intel_energy_perf(c); > + > + init_misc_enables(c); > } > > #ifdef CONFIG_X86_32 > --- > > Please redo your patchset and add the detection to init_intel() like above. > > Also, let's call that MSR mask MSR_MISC_ENABLES_CPUID_FAULT_ENABLE like > the rest of the bits in msr-index.h There's already an IA32_MISC_ENABLE, so I've made this MSR_MISC_FEATURES_ENABLES_CPUID_FAULT instead. - Kyle -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 9, 2016 at 5:34 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, 9 Nov 2016, Borislav Petkov wrote: >> +static void init_misc_enables(struct cpuinfo_x86 *c) >> +{ >> + u64 val, misc_en; >> + >> + if (rdmsrl_safe(MSR_MISC_FEATURES_ENABLES, &misc_en)) >> + return; >> + >> + misc_en &= ~MSR_MISC_ENABLES_CPUID_FAULT_ENABLE; > > I rather prefer to write this MSR to 0 right away and just enable the bits > which we really support. I've gone ahead and done this. - Kyle -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index fcd484d2bb03..5c38a85af2e7 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -452,6 +457,39 @@ static void intel_bsp_resume(struct cpuinfo_x86 *c) init_intel_energy_perf(c); } +static void init_misc_enables(struct cpuinfo_x86 *c) +{ + u64 val, misc_en; + + if (rdmsrl_safe(MSR_MISC_FEATURES_ENABLES, &misc_en)) + return; + + misc_en &= ~MSR_MISC_ENABLES_CPUID_FAULT_ENABLE; + + if (!rdmsrl_safe(MSR_PLATFORM_INFO, &val)) { + if (val & PLATINFO_CPUID_FAULT_BIT) + set_cpu_cap(c, X86_FEATURE_CPUID_FAULT); + } + + wrmsrl(MSR_MISC_FEATURES_ENABLES, misc_en); + this_cpu_write(msr_misc_features_enables_shadow, misc_en); +} + static void init_intel(struct cpuinfo_x86 *c) { unsigned int l2 = 0; @@ -565,6 +603,8 @@ static void init_intel(struct cpuinfo_x86 *c) detect_vmx_virtcap(c); init_intel_energy_perf(c); + + init_misc_enables(c); } #ifdef CONFIG_X86_32 --- Please redo your patchset and add the detection to init_intel() like above. Also, let's call that MSR mask MSR_MISC_ENABLES_CPUID_FAULT_ENABLE like the rest of the bits in msr-index.h