diff mbox

x86: avoid #GP for PV guest MSR accesses

Message ID 59C4EEA3020000780017E6DE@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Sept. 22, 2017, 9:06 a.m. UTC 
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>

Comments

Sergey Dyasli Sept. 22, 2017, 10:32 a.m. UTC | #1 
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
Jan Beulich Sept. 22, 2017, 10:44 a.m. UTC | #2 
>>> 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
diff mbox

Patch

--- 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 */