Message ID | 59C4EEA3020000780017E6DE@prv-mh.provo.novell.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, 2017-09-22 at 03:06 -0600, Jan Beulich wrote: > Halfway recent Linux kernels probe MISC_FEATURES_ENABLES on all CPUs, > leading to ugly recovered #GP fault messages with debug builds on older > systems. We can do better, so introduce synthetic feature flags for > both this and PLATFORM_INFO to avoid the rdmsr_safe() altogether. > > The rdmsr_safe() uses for MISC_ENABLE are left in place as benign - it > exists for all 64-bit capable Intel CPUs (see e.g. early_init_intel()). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> The intent of this patch (and the related "VMX: PLATFORM_INFO MSR is r/o") is somewhat intersects with my series "Generic MSR policy: infrastructure + cpuid_faulting". IMHO it's better to fix MSR-related issues in the scope of the MSR policy work. Also, I have one question below. > > --- a/xen/arch/x86/cpu/intel.c > +++ b/xen/arch/x86/cpu/intel.c > @@ -21,10 +21,19 @@ static bool __init probe_intel_cpuid_fau > { > uint64_t x; > > - if (rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x) || > - !(x & MSR_PLATFORM_INFO_CPUID_FAULTING)) > + if (rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x)) > return 0; > > + setup_force_cpu_cap(X86_FEATURE_MSR_PLATFORM_INFO); > + > + if (!(x & MSR_PLATFORM_INFO_CPUID_FAULTING)) { > + if (!rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, x)) > + setup_force_cpu_cap(X86_FEATURE_MSR_MISC_FEATURES); > + return 0; > + } > + > + setup_force_cpu_cap(X86_FEATURE_MSR_MISC_FEATURES); > + > expected_levelling_cap |= LCAP_faulting; > levelling_caps |= LCAP_faulting; > setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING); > --- a/xen/arch/x86/pv/emul-priv-op.c > +++ b/xen/arch/x86/pv/emul-priv-op.c > @@ -941,8 +941,7 @@ static int read_msr(unsigned int reg, ui > return X86EMUL_OKAY; > > case MSR_INTEL_PLATFORM_INFO: > - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || > - rdmsr_safe(MSR_INTEL_PLATFORM_INFO, *val) ) > + if ( !boot_cpu_has(X86_FEATURE_MSR_PLATFORM_INFO) ) > break; > *val = 0; > if ( this_cpu(cpuid_faulting_enabled) ) > @@ -950,8 +949,7 @@ static int read_msr(unsigned int reg, ui > return X86EMUL_OKAY; > > case MSR_INTEL_MISC_FEATURES_ENABLES: > - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || > - rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, *val) ) > + if ( !boot_cpu_has(X86_FEATURE_MSR_MISC_FEATURES) ) > break; > *val = 0; > if ( curr->arch.cpuid_faulting ) > @@ -1147,15 +1145,13 @@ static int write_msr(unsigned int reg, u > return X86EMUL_OKAY; > > case MSR_INTEL_PLATFORM_INFO: > - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || > - val || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val) ) > + if ( !boot_cpu_has(X86_FEATURE_MSR_PLATFORM_INFO) || val ) > break; > return X86EMUL_OKAY; Why writes to MSR_INTEL_PLATFORM_INFO shouldn't produce #GP faults for PV guests? > > case MSR_INTEL_MISC_FEATURES_ENABLES: > - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || > - (val & ~MSR_MISC_FEATURES_CPUID_FAULTING) || > - rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, temp) ) > + if ( !boot_cpu_has(X86_FEATURE_MSR_MISC_FEATURES) || > + (val & ~MSR_MISC_FEATURES_CPUID_FAULTING) ) > break; > if ( (val & MSR_MISC_FEATURES_CPUID_FAULTING) && > !this_cpu(cpuid_faulting_enabled) ) > --- a/xen/include/asm-x86/cpufeatures.h > +++ b/xen/include/asm-x86/cpufeatures.h > @@ -22,3 +22,5 @@ XEN_CPUFEATURE(APERFMPERF, (FSCAPIN > XEN_CPUFEATURE(MFENCE_RDTSC, (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTSC */ > XEN_CPUFEATURE(XEN_SMEP, (FSCAPINTS+0)*32+10) /* SMEP gets used by Xen itself */ > XEN_CPUFEATURE(XEN_SMAP, (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen itself */ > +XEN_CPUFEATURE(MSR_PLATFORM_INFO, (FSCAPINTS+0)*32+12) /* PLATFORM_INFO MSR present */ > +XEN_CPUFEATURE(MSR_MISC_FEATURES, (FSCAPINTS+0)*32+13) /* MISC_FEATURES_ENABLES MSR present */ > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel -- Thanks, Sergey
>>> On 22.09.17 at 12:32, <sergey.dyasli@citrix.com> wrote: > On Fri, 2017-09-22 at 03:06 -0600, Jan Beulich wrote: >> Halfway recent Linux kernels probe MISC_FEATURES_ENABLES on all CPUs, >> leading to ugly recovered #GP fault messages with debug builds on older >> systems. We can do better, so introduce synthetic feature flags for >> both this and PLATFORM_INFO to avoid the rdmsr_safe() altogether. >> >> The rdmsr_safe() uses for MISC_ENABLE are left in place as benign - it >> exists for all 64-bit capable Intel CPUs (see e.g. early_init_intel()). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > The intent of this patch (and the related "VMX: PLATFORM_INFO MSR is > r/o") is somewhat intersects with my series "Generic MSR policy: > infrastructure + cpuid_faulting". IMHO it's better to fix MSR-related > issues in the scope of the MSR policy work. I'm of the opposite opinion, as doing what you suggest would make the backport more difficult. >> @@ -1147,15 +1145,13 @@ static int write_msr(unsigned int reg, u >> return X86EMUL_OKAY; >> >> case MSR_INTEL_PLATFORM_INFO: >> - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || >> - val || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val) ) >> + if ( !boot_cpu_has(X86_FEATURE_MSR_PLATFORM_INFO) || val ) >> break; >> return X86EMUL_OKAY; > > Why writes to MSR_INTEL_PLATFORM_INFO shouldn't produce #GP faults for > PV guests? My mistake not carrying backwards what I had figured when doing the VMX side change. Jan
--- a/xen/arch/x86/cpu/intel.c +++ b/xen/arch/x86/cpu/intel.c @@ -21,10 +21,19 @@ static bool __init probe_intel_cpuid_fau { uint64_t x; - if (rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x) || - !(x & MSR_PLATFORM_INFO_CPUID_FAULTING)) + if (rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x)) return 0; + setup_force_cpu_cap(X86_FEATURE_MSR_PLATFORM_INFO); + + if (!(x & MSR_PLATFORM_INFO_CPUID_FAULTING)) { + if (!rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, x)) + setup_force_cpu_cap(X86_FEATURE_MSR_MISC_FEATURES); + return 0; + } + + setup_force_cpu_cap(X86_FEATURE_MSR_MISC_FEATURES); + expected_levelling_cap |= LCAP_faulting; levelling_caps |= LCAP_faulting; setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING); --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -941,8 +941,7 @@ static int read_msr(unsigned int reg, ui return X86EMUL_OKAY; case MSR_INTEL_PLATFORM_INFO: - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || - rdmsr_safe(MSR_INTEL_PLATFORM_INFO, *val) ) + if ( !boot_cpu_has(X86_FEATURE_MSR_PLATFORM_INFO) ) break; *val = 0; if ( this_cpu(cpuid_faulting_enabled) ) @@ -950,8 +949,7 @@ static int read_msr(unsigned int reg, ui return X86EMUL_OKAY; case MSR_INTEL_MISC_FEATURES_ENABLES: - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || - rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, *val) ) + if ( !boot_cpu_has(X86_FEATURE_MSR_MISC_FEATURES) ) break; *val = 0; if ( curr->arch.cpuid_faulting ) @@ -1147,15 +1145,13 @@ static int write_msr(unsigned int reg, u return X86EMUL_OKAY; case MSR_INTEL_PLATFORM_INFO: - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || - val || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val) ) + if ( !boot_cpu_has(X86_FEATURE_MSR_PLATFORM_INFO) || val ) break; return X86EMUL_OKAY; case MSR_INTEL_MISC_FEATURES_ENABLES: - if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || - (val & ~MSR_MISC_FEATURES_CPUID_FAULTING) || - rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, temp) ) + if ( !boot_cpu_has(X86_FEATURE_MSR_MISC_FEATURES) || + (val & ~MSR_MISC_FEATURES_CPUID_FAULTING) ) break; if ( (val & MSR_MISC_FEATURES_CPUID_FAULTING) && !this_cpu(cpuid_faulting_enabled) ) --- a/xen/include/asm-x86/cpufeatures.h +++ b/xen/include/asm-x86/cpufeatures.h @@ -22,3 +22,5 @@ XEN_CPUFEATURE(APERFMPERF, (FSCAPIN XEN_CPUFEATURE(MFENCE_RDTSC, (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTSC */ XEN_CPUFEATURE(XEN_SMEP, (FSCAPINTS+0)*32+10) /* SMEP gets used by Xen itself */ XEN_CPUFEATURE(XEN_SMAP, (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen itself */ +XEN_CPUFEATURE(MSR_PLATFORM_INFO, (FSCAPINTS+0)*32+12) /* PLATFORM_INFO MSR present */ +XEN_CPUFEATURE(MSR_MISC_FEATURES, (FSCAPINTS+0)*32+13) /* MISC_FEATURES_ENABLES MSR present */
Halfway recent Linux kernels probe MISC_FEATURES_ENABLES on all CPUs, leading to ugly recovered #GP fault messages with debug builds on older systems. We can do better, so introduce synthetic feature flags for both this and PLATFORM_INFO to avoid the rdmsr_safe() altogether. The rdmsr_safe() uses for MISC_ENABLE are left in place as benign - it exists for all 64-bit capable Intel CPUs (see e.g. early_init_intel()). Signed-off-by: Jan Beulich <jbeulich@suse.com>