[PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

[PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

[Andrew Cooper]

Virtual environments such as Xen HVM containers and VirtualBox do not
necessarily provide support for feature masking MSRs.

As their presence is detected by model numbers alone, and their use predicated
on command line parameters, use the safe() variants of {wr,rd}msr() to avoid
dying with an early #GP fault.

While playing in this function, make some further improvements.

* Rename the masking MSR names for consistency, and name the CPU models for
  the benefit of humans reading the code.
* Correct the CPU selection as specified in the flexmigration document.  All
  steppings of 0x17 and 0x1a are stated to have masking MSRs.
* Provide log messages indicating the masks applied, or lack of masking
  capabilities.
* Call set_cpuidmask() unconditionally so faulting-capable hardware still gets
  a log message indicating to the user why their command line arguments are
  not taking effect.

References:
http://www.intel.com/content/www/us/en/virtualization/virtualization-technology-flexmigration-application-note.html
http://software.intel.com/en-us/articles/intel-architecture-and-processor-identification-with-cpuid-model-and-family-numbers

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Yang Zhang <yang.z.zhang@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

---

Can anyone shed light as to which CPU model 0x1f is?  It is specified in the
flexmigrate document, and was present in the old code, but I can't find any
further information online.
---
 xen/arch/x86/cpu/intel.c        |  150 +++++++++++++++++++++++++--------------
 xen/include/asm-x86/msr-index.h |   13 ++--
 2 files changed, 102 insertions(+), 61 deletions(-)

diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index e178b5c..f1ee5b3 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -51,68 +51,109 @@ void set_cpuid_faulting(bool_t enable)
  */
 static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 {
-	u32 eax, edx;
-	const char *extra = "";
+	static unsigned int msr_basic, msr_ext, msr_xsave;
+	static enum { not_parsed, no_mask, set_mask } status;
+	u64 msr_val;
+
+	if (status == no_mask)
+		return;
+
+	if (status == set_mask)
+		goto setmask;
+
+	ASSERT((status == not_parsed) && (c == &boot_cpu_data));
+	status = no_mask;
 
 	if (!~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
 	       opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
-               opt_cpuid_mask_xsave_eax))
+	       opt_cpuid_mask_xsave_eax))
 		return;
 
-	/* Only family 6 supports this feature  */
-	switch ((c->x86 == 6) * c->x86_model) {
-	case 0x17:
-		if ((c->x86_mask & 0x0f) < 4)
-			break;
-		/* fall through */
-	case 0x1d:
-		wrmsr(MSR_INTEL_CPUID_FEATURE_MASK,
-		      opt_cpuid_mask_ecx,
-		      opt_cpuid_mask_edx);
-		if (~(opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx))
-			extra = "extended ";
-		else if (~opt_cpuid_mask_xsave_eax)
-			extra = "xsave ";
-		else
-			return;
+	/* Only family 6 supports this feature. */
+	if (c->x86 != 6) {
+		printk("No CPUID feature masking support available\n");
+		return;
+	}
+
+	switch (c->x86_model) {
+	case 0x17: /* Yorkfield, Wolfdale, Penryn, Harpertown(DP) */
+	case 0x1d: /* Dunnington(MP) */
+		msr_basic = MSR_INTEL_MASK_V1_CPUID1;
 		break;
-/* 
- * CPU supports this feature if the processor signature meets the following:
- * (CPUID.(EAX=01h):EAX) > 000106A2h, or
- * (CPUID.(EAX=01h):EAX) == 000106Exh, 0002065xh, 000206Cxh, 000206Exh, or 000206Fxh
- *
- */
-	case 0x1a:
-		if ((c->x86_mask & 0x0f) <= 2)
-			break;
-		/* fall through */
-	case 0x1e: case 0x1f:
-	case 0x25: case 0x2c: case 0x2e: case 0x2f:
-		wrmsr(MSR_INTEL_CPUID1_FEATURE_MASK,
-		      opt_cpuid_mask_ecx,
-		      opt_cpuid_mask_edx);
-		wrmsr(MSR_INTEL_CPUID80000001_FEATURE_MASK,
-		      opt_cpuid_mask_ext_ecx,
-		      opt_cpuid_mask_ext_edx);
-		if (!~opt_cpuid_mask_xsave_eax)
-			return;
-		extra = "xsave ";
+
+	case 0x1a: /* Bloomfield, Nehalem-EP */
+	case 0x1e: /* Clarksfield, Lynnfield, Jasper Forest */
+	case 0x1f: /* ??? */
+	case 0x25: /* Arrandale, Clarksdale */
+	case 0x2c: /* Gulftown, Westmere-EP */
+	case 0x2e: /* Nehalem-EX */
+	case 0x2f: /* Westmere-EX */
+		msr_basic = MSR_INTEL_MASK_V2_CPUID1;
+		msr_ext   = MSR_INTEL_MASK_V2_CPUID80000001;
+		break;
+
+	case 0x2a: /* SandyBridge */
+	case 0x2d: /* SandyBridge-E, SandyBridge-EN, SandyBridge-EP */
+		msr_basic = MSR_INTEL_MASK_V3_CPUID1;
+		msr_ext   = MSR_INTEL_MASK_V3_CPUID80000001;
+		msr_xsave = MSR_INTEL_MASK_V3_CPUIDD_01;
 		break;
-	case 0x2a: case 0x2d:
-		wrmsr(MSR_INTEL_CPUID1_FEATURE_MASK_V2,
-		      opt_cpuid_mask_ecx,
-		      opt_cpuid_mask_edx);
-		rdmsr(MSR_INTEL_CPUIDD_01_FEATURE_MASK, eax, edx);
-		wrmsr(MSR_INTEL_CPUIDD_01_FEATURE_MASK,
-		      opt_cpuid_mask_xsave_eax, edx);
-		wrmsr(MSR_INTEL_CPUID80000001_FEATURE_MASK_V2,
-		      opt_cpuid_mask_ext_ecx,
-		      opt_cpuid_mask_ext_edx);
-		return;
 	}
 
-	printk(XENLOG_ERR "Cannot set CPU %sfeature mask on CPU#%d\n",
-	       extra, smp_processor_id());
+	status = set_mask;
+
+	if (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx)) {
+		if (msr_basic)
+			printk("Writing CPUID feature mask ecx:edx -> %08x:%08x\n",
+			       opt_cpuid_mask_ecx, opt_cpuid_mask_edx);
+		else
+			printk("No CPUID feature mask available\n");
+	}
+	else
+		msr_basic = 0;
+
+	if (~(opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx)) {
+		if (msr_ext)
+			printk("Writing CPUID extended feature mask ecx:edx -> %08x:%08x\n",
+			       opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx);
+		else
+			printk("No CPUID extended feature mask available\n");
+	}
+	else
+		msr_ext = 0;
+
+	if (~opt_cpuid_mask_xsave_eax) {
+		if (msr_xsave)
+			printk("Writing CPUID xsave feature mask eax -> %08x\n",
+			       opt_cpuid_mask_xsave_eax);
+		else
+			printk("No CPUID xsave feature mask available\n");
+	}
+	else
+		msr_xsave = 0;
+
+ setmask:
+	if (msr_basic &&
+	    wrmsr_safe(msr_basic,
+		       ((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx)){
+		msr_basic = 0;
+		printk("Failed to set CPUID feature mask\n");
+	}
+
+	if (msr_ext &&
+	    wrmsr_safe(msr_ext,
+		       ((u64)opt_cpuid_mask_ext_edx << 32) | opt_cpuid_mask_ext_ecx)){
+		msr_ext = 0;
+		printk("Failed to set CPUID extended feature mask\n");
+	}
+
+	if (msr_xsave &&
+	    (rdmsr_safe(msr_xsave, msr_val) ||
+	     wrmsr_safe(msr_xsave,
+			((u64)msr_val << 32) | opt_cpuid_mask_xsave_eax))){
+		msr_xsave = 0;
+		printk("Failed to set CPUID xsave feature mask\n");
+	}
 }
 
 void __devinit early_intel_workaround(struct cpuinfo_x86 *c)
@@ -220,8 +261,7 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
 		set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
 	}
 
-	if (!cpu_has_cpuid_faulting)
-		set_cpuidmask(c);
+	set_cpuidmask(c);
 
 	/* Work around errata */
 	Intel_errata_workarounds(c);
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 70a8201..7d38bcb 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -469,13 +469,14 @@
 #define MSR_CORE_PERF_GLOBAL_OVF_CTRL	0x00000390
 
 /* Intel cpuid spoofing MSRs */
-#define MSR_INTEL_CPUID_FEATURE_MASK	0x00000478
-#define MSR_INTEL_CPUID1_FEATURE_MASK	0x00000130
-#define MSR_INTEL_CPUID80000001_FEATURE_MASK 0x00000131
+#define MSR_INTEL_MASK_V1_CPUID1	0x00000478
 
-#define MSR_INTEL_CPUID1_FEATURE_MASK_V2        0x00000132
-#define MSR_INTEL_CPUID80000001_FEATURE_MASK_V2 0x00000133
-#define MSR_INTEL_CPUIDD_01_FEATURE_MASK        0x00000134
+#define MSR_INTEL_MASK_V2_CPUID1	0x00000130
+#define MSR_INTEL_MASK_V2_CPUID80000001 0x00000131
+
+#define MSR_INTEL_MASK_V3_CPUID1	0x00000132
+#define MSR_INTEL_MASK_V3_CPUID80000001 0x00000133
+#define MSR_INTEL_MASK_V3_CPUIDD_01	0x00000134
 
 /* Intel cpuid faulting MSRs */
 #define MSR_INTEL_PLATFORM_INFO		0x000000ce
-- 
1.7.10.4

Re: [PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

[Jan Beulich]

>>> On 05.06.14 at 13:19, <andrew.cooper3@citrix.com> wrote:
> Virtual environments such as Xen HVM containers and VirtualBox do not
> necessarily provide support for feature masking MSRs.
> 
> As their presence is detected by model numbers alone, and their use 
> predicated
> on command line parameters, use the safe() variants of {wr,rd}msr() to avoid
> dying with an early #GP fault.

I'm tempted to say "Then just don't pass these options." There are
other options that can lead to boot failure if not used properly.

> * Call set_cpuidmask() unconditionally so faulting-capable hardware still gets
>   a log message indicating to the user why their command line arguments are
>   not taking effect.

I don't think Intel will particularly like this part.

> -	/* Only family 6 supports this feature  */
> -	switch ((c->x86 == 6) * c->x86_model) {
> -	case 0x17:
> -		if ((c->x86_mask & 0x0f) < 4)
> -			break;

I had been inquiring about this stepping specific check here too,
without ever getting clarification. I think we shouldn't blindly
drop it.

> + setmask:
> +	if (msr_basic &&
> +	    wrmsr_safe(msr_basic,
> +		       ((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx)){
> +		msr_basic = 0;
> +		printk("Failed to set CPUID feature mask\n");
> +	}
> +
> +	if (msr_ext &&
> +	    wrmsr_safe(msr_ext,
> +		       ((u64)opt_cpuid_mask_ext_edx << 32) | opt_cpuid_mask_ext_ecx)){
> +		msr_ext = 0;
> +		printk("Failed to set CPUID extended feature mask\n");
> +	}
> +
> +	if (msr_xsave &&
> +	    (rdmsr_safe(msr_xsave, msr_val) ||
> +	     wrmsr_safe(msr_xsave,
> +			((u64)msr_val << 32) | opt_cpuid_mask_xsave_eax))){

I'm afraid copy'n'paste went a little too far here: Neither does msr_val
need a u64 cast, nor do you want to write into the high half what you
read from the low one.

Jan

Re: [PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

[Andrew Cooper]

On 05/06/14 15:05, Jan Beulich wrote:
>>>> On 05.06.14 at 13:19, <andrew.cooper3@citrix.com> wrote:
>> Virtual environments such as Xen HVM containers and VirtualBox do not
>> necessarily provide support for feature masking MSRs.
>>
>> As their presence is detected by model numbers alone, and their use 
>> predicated
>> on command line parameters, use the safe() variants of {wr,rd}msr() to avoid
>> dying with an early #GP fault.
> I'm tempted to say "Then just don't pass these options." There are
> other options that can lead to boot failure if not used properly.

That is one opinion, but I would disagree.  There are few things as bad
as making a mistake on the command line and ending with a broken server
somewhere on the other side of the world because it failed to boot,
especially if Xen can deal sensibly with the failure and still boot the
server.

>
>> * Call set_cpuidmask() unconditionally so faulting-capable hardware still gets
>>   a log message indicating to the user why their command line arguments are
>>   not taking effect.
> I don't think Intel will particularly like this part.

Why not?  Until someone (most likely myself, following the migration
series is accepted) fixes faulting to actually be able to provide a
policy, users attempting to use cpuid_mask_XXX to perform levelling end
up with an unlevelled server and no hint as to why.

>
>> -	/* Only family 6 supports this feature  */
>> -	switch ((c->x86 == 6) * c->x86_model) {
>> -	case 0x17:
>> -		if ((c->x86_mask & 0x0f) < 4)
>> -			break;
> I had been inquiring about this stepping specific check here too,
> without ever getting clarification. I think we shouldn't blindly
> drop it.

I did some archaeology in the history.  This check was introduced
exactly as-is with the introduction of the masking code, without
specific with regard to the stepping.

The latest spec states "Extended model ID 1H and models 7H and DH (for
all values of stepping ID)", so I would opt for dropping the check.

I wonder whether I can find hardware in XenRT which would normally fail
that specific check.

>
>> + setmask:
>> +	if (msr_basic &&
>> +	    wrmsr_safe(msr_basic,
>> +		       ((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx)){
>> +		msr_basic = 0;
>> +		printk("Failed to set CPUID feature mask\n");
>> +	}
>> +
>> +	if (msr_ext &&
>> +	    wrmsr_safe(msr_ext,
>> +		       ((u64)opt_cpuid_mask_ext_edx << 32) | opt_cpuid_mask_ext_ecx)){
>> +		msr_ext = 0;
>> +		printk("Failed to set CPUID extended feature mask\n");
>> +	}
>> +
>> +	if (msr_xsave &&
>> +	    (rdmsr_safe(msr_xsave, msr_val) ||
>> +	     wrmsr_safe(msr_xsave,
>> +			((u64)msr_val << 32) | opt_cpuid_mask_xsave_eax))){
> I'm afraid copy'n'paste went a little too far here: Neither does msr_val
> need a u64 cast, nor do you want to write into the high half what you
> read from the low one.
>
> Jan
>

Oops - indeed it did.

This turns out to be safe as the MSRs initial value is ~0ULL, which is
why it passed my dev test.  I will fix up for v2.

~Andrew

Re: [PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

[Jan Beulich]

>>> On 05.06.14 at 16:24, <andrew.cooper3@citrix.com> wrote:
> On 05/06/14 15:05, Jan Beulich wrote:
>>>>> On 05.06.14 at 13:19, <andrew.cooper3@citrix.com> wrote:
>>> * Call set_cpuidmask() unconditionally so faulting-capable hardware still gets
>>>   a log message indicating to the user why their command line arguments are
>>>   not taking effect.
>> I don't think Intel will particularly like this part.
> 
> Why not?

Because they try to deprecate masking in favor of CPUID faulting as
much as they can.

Jan

Re: [PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

[Andrew Cooper]

On 05/06/14 15:56, Jan Beulich wrote:
>>>> On 05.06.14 at 16:24, <andrew.cooper3@citrix.com> wrote:
>> On 05/06/14 15:05, Jan Beulich wrote:
>>>>>> On 05.06.14 at 13:19, <andrew.cooper3@citrix.com> wrote:
>>>> * Call set_cpuidmask() unconditionally so faulting-capable hardware still gets
>>>>   a log message indicating to the user why their command line arguments are
>>>>   not taking effect.
>>> I don't think Intel will particularly like this part.
>> Why not?
> Because they try to deprecate masking in favor of CPUID faulting as
> much as they can.
>
> Jan
>

And that is a very good thing.

However, masking is only available SandyBridge and older (for a few
generations), whereas faulting is only available on IvyBridge and newer.

If the user tries setting the cpuid_mask_XXX in the hope that masking
occurs, It is kind to give them an error back explaining why nothing is
happening.

~Andrew