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

[PATCH] x86/amd: 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.

In fact, use the password variants in all cases because:
    a) they are safe to use even if not strictly required
    b) have a more useful function prototype for this purposes

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/cpu/amd.c |   41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index ea158cb..dc38d69 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -151,7 +151,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 	static unsigned int extfeat_ecx, extfeat_edx;
 	static unsigned int l7s0_eax, l7s0_ebx;
 	static unsigned int thermal_ecx;
-	static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
+	static bool_t skip_feat, skip_extfeat, skip_l7s0, skip_thermal;
 	static enum { not_parsed, no_mask, set_mask } status;
 	unsigned int eax, ebx, ecx, edx;
 
@@ -220,7 +220,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 		printk("Writing CPUID leaf 7 subleaf 0 feature mask EAX:EBX -> %08Xh:%08Xh\n",
 		       l7s0_eax, l7s0_ebx);
 	} else
-		skip_l7s0_eax_ebx = 1;
+		skip_l7s0 = 1;
 
 	/* Only Fam15 has the respective MSR. */
 	ecx = c->x86 == 0x15 && c->cpuid_level >= 6 ? cpuid_ecx(6) : 0;
@@ -229,22 +229,33 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 		printk("Writing CPUID thermal/power feature mask ECX -> %08Xh\n",
 		       thermal_ecx);
 	} else
-		skip_thermal_ecx = 1;
+		skip_thermal = 1;
 
  setmask:
 	/* AMD processors prior to family 10h required a 32-bit password */
-	if (c->x86 >= 0x10) {
-		wrmsr(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
-		wrmsr(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
-		if (!skip_l7s0_eax_ebx)
-			wrmsr(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax);
-		if (!skip_thermal_ecx) {
-			rdmsr(MSR_AMD_THRM_FEATURE_MASK, eax, edx);
-			wrmsr(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx);
-		}
-	} else {
-		wrmsr_amd(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
-		wrmsr_amd(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
+	if (!skip_feat &&
+	    wrmsr_amd_safe(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx)) {
+		skip_feat = 1;
+		printk("Failed to set CPUID feature mask\n");
+	}
+
+	if (!skip_extfeat &&
+	    wrmsr_amd_safe(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx)) {
+		skip_extfeat = 1;
+		printk("Failed to set CPUID extended feature mask\n");
+	}
+
+	if (!skip_l7s0 &&
+	    wrmsr_amd_safe(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax)) {
+		skip_l7s0 = 1;
+		printk("Failed to set CPUID leaf 7 subleaf 0 feature mask\n");
+	}
+
+	if (!skip_thermal &&
+	    (rdmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, &eax, &edx) ||
+	     wrmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx))){
+		skip_thermal = 1;
+		printk("Failed to set CPUID thermal/power feature mask\n");
 	}
 }
 
-- 
1.7.10.4

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

[Boris Ostrovsky]

On 06/05/2014 07:20 AM, Andrew Cooper 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.
>
> In fact, use the password variants in all cases because:
>      a) they are safe to use even if not strictly required
>      b) have a more useful function prototype for this purposes
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


>   xen/arch/x86/cpu/amd.c |   41 ++++++++++++++++++++++++++---------------
>   1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index ea158cb..dc38d69 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -151,7 +151,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>   	static unsigned int extfeat_ecx, extfeat_edx;
>   	static unsigned int l7s0_eax, l7s0_ebx;
>   	static unsigned int thermal_ecx;
> -	static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
> +	static bool_t skip_feat, skip_extfeat, skip_l7s0, skip_thermal;
>   	static enum { not_parsed, no_mask, set_mask } status;
>   	unsigned int eax, ebx, ecx, edx;
>   
> @@ -220,7 +220,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>   		printk("Writing CPUID leaf 7 subleaf 0 feature mask EAX:EBX -> %08Xh:%08Xh\n",
>   		       l7s0_eax, l7s0_ebx);
>   	} else
> -		skip_l7s0_eax_ebx = 1;
> +		skip_l7s0 = 1;
>   
>   	/* Only Fam15 has the respective MSR. */
>   	ecx = c->x86 == 0x15 && c->cpuid_level >= 6 ? cpuid_ecx(6) : 0;
> @@ -229,22 +229,33 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>   		printk("Writing CPUID thermal/power feature mask ECX -> %08Xh\n",
>   		       thermal_ecx);
>   	} else
> -		skip_thermal_ecx = 1;
> +		skip_thermal = 1;
>   
>    setmask:
>   	/* AMD processors prior to family 10h required a 32-bit password */
> -	if (c->x86 >= 0x10) {
> -		wrmsr(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
> -		wrmsr(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
> -		if (!skip_l7s0_eax_ebx)
> -			wrmsr(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax);
> -		if (!skip_thermal_ecx) {
> -			rdmsr(MSR_AMD_THRM_FEATURE_MASK, eax, edx);
> -			wrmsr(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx);
> -		}
> -	} else {
> -		wrmsr_amd(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
> -		wrmsr_amd(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
> +	if (!skip_feat &&
> +	    wrmsr_amd_safe(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx)) {
> +		skip_feat = 1;
> +		printk("Failed to set CPUID feature mask\n");
> +	}
> +
> +	if (!skip_extfeat &&
> +	    wrmsr_amd_safe(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx)) {
> +		skip_extfeat = 1;
> +		printk("Failed to set CPUID extended feature mask\n");
> +	}
> +
> +	if (!skip_l7s0 &&
> +	    wrmsr_amd_safe(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax)) {
> +		skip_l7s0 = 1;
> +		printk("Failed to set CPUID leaf 7 subleaf 0 feature mask\n");
> +	}
> +
> +	if (!skip_thermal &&
> +	    (rdmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, &eax, &edx) ||
> +	     wrmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx))){
> +		skip_thermal = 1;
> +		printk("Failed to set CPUID thermal/power feature mask\n");
>   	}
>   }
>   

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

[Jan Beulich]

>>> On 05.06.14 at 13:20, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -151,7 +151,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>  	static unsigned int extfeat_ecx, extfeat_edx;
>  	static unsigned int l7s0_eax, l7s0_ebx;
>  	static unsigned int thermal_ecx;
> -	static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
> +	static bool_t skip_feat, skip_extfeat, skip_l7s0, skip_thermal;

Please don't drop the _eax_ebx and _ecx suffixes - they're there
for a reason (leaf 7 likely also getting ECX and EDX a bitmap assigned
at some point, and the thermal leaf also having further unused
registers that may become subject to masking later on).

Jan

[PATCH v2] x86/amd: 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.

In fact, use the password variants in all cases because:
    a) they are safe to use even if not strictly required
    b) have a more useful function prototype for this purposes

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

---
v2: Retain register suffixes for skip_??? booleans
---
 xen/arch/x86/cpu/amd.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index ea158cb..53ffbdb 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -151,6 +151,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 	static unsigned int extfeat_ecx, extfeat_edx;
 	static unsigned int l7s0_eax, l7s0_ebx;
 	static unsigned int thermal_ecx;
+	static bool_t skip_feat_ecx_edx, skip_extfeat_ecx_edx;
 	static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
 	static enum { not_parsed, no_mask, set_mask } status;
 	unsigned int eax, ebx, ecx, edx;
@@ -233,18 +234,29 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
 
  setmask:
 	/* AMD processors prior to family 10h required a 32-bit password */
-	if (c->x86 >= 0x10) {
-		wrmsr(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
-		wrmsr(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
-		if (!skip_l7s0_eax_ebx)
-			wrmsr(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax);
-		if (!skip_thermal_ecx) {
-			rdmsr(MSR_AMD_THRM_FEATURE_MASK, eax, edx);
-			wrmsr(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx);
-		}
-	} else {
-		wrmsr_amd(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
-		wrmsr_amd(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
+	if (!skip_feat_ecx_edx &&
+	    wrmsr_amd_safe(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx)) {
+		skip_feat_ecx_edx = 1;
+		printk("Failed to set CPUID feature mask\n");
+	}
+
+	if (!skip_extfeat_ecx_edx &&
+	    wrmsr_amd_safe(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx)) {
+		skip_extfeat_ecx_edx = 1;
+		printk("Failed to set CPUID extended feature mask\n");
+	}
+
+	if (!skip_l7s0_eax_ebx &&
+	    wrmsr_amd_safe(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax)) {
+		skip_l7s0_eax_ebx = 1;
+		printk("Failed to set CPUID leaf 7 subleaf 0 feature mask\n");
+	}
+
+	if (!skip_thermal_ecx &&
+	    (rdmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, &eax, &edx) ||
+	     wrmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx))){
+		skip_thermal_ecx = 1;
+		printk("Failed to set CPUID thermal/power feature mask\n");
 	}
 }
 
-- 
1.7.10.4

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

[Aravind Gopalakrishnan]

On 6/5/2014 10:23 AM, Andrew Cooper 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.
>
> In fact, use the password variants in all cases because:
>      a) they are safe to use even if not strictly required
>      b) have a more useful function prototype for this purposes
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>
> ---
> v2: Retain register suffixes for skip_??? booleans
> ---
>   xen/arch/x86/cpu/amd.c |   36 ++++++++++++++++++++++++------------
>   1 file changed, 24 insertions(+), 12 deletions(-)

Acked-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index ea158cb..53ffbdb 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -151,6 +151,7 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>   	static unsigned int extfeat_ecx, extfeat_edx;
>   	static unsigned int l7s0_eax, l7s0_ebx;
>   	static unsigned int thermal_ecx;
> +	static bool_t skip_feat_ecx_edx, skip_extfeat_ecx_edx;
>   	static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
>   	static enum { not_parsed, no_mask, set_mask } status;
>   	unsigned int eax, ebx, ecx, edx;
> @@ -233,18 +234,29 @@ static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>   
>    setmask:
>   	/* AMD processors prior to family 10h required a 32-bit password */
> -	if (c->x86 >= 0x10) {
> -		wrmsr(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
> -		wrmsr(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
> -		if (!skip_l7s0_eax_ebx)
> -			wrmsr(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax);
> -		if (!skip_thermal_ecx) {
> -			rdmsr(MSR_AMD_THRM_FEATURE_MASK, eax, edx);
> -			wrmsr(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx);
> -		}
> -	} else {
> -		wrmsr_amd(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx);
> -		wrmsr_amd(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx);
> +	if (!skip_feat_ecx_edx &&
> +	    wrmsr_amd_safe(MSR_K8_FEATURE_MASK, feat_edx, feat_ecx)) {
> +		skip_feat_ecx_edx = 1;
> +		printk("Failed to set CPUID feature mask\n");
> +	}
> +
> +	if (!skip_extfeat_ecx_edx &&
> +	    wrmsr_amd_safe(MSR_K8_EXT_FEATURE_MASK, extfeat_edx, extfeat_ecx)) {
> +		skip_extfeat_ecx_edx = 1;
> +		printk("Failed to set CPUID extended feature mask\n");
> +	}
> +
> +	if (!skip_l7s0_eax_ebx &&
> +	    wrmsr_amd_safe(MSR_AMD_L7S0_FEATURE_MASK, l7s0_ebx, l7s0_eax)) {
> +		skip_l7s0_eax_ebx = 1;
> +		printk("Failed to set CPUID leaf 7 subleaf 0 feature mask\n");
> +	}
> +
> +	if (!skip_thermal_ecx &&
> +	    (rdmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, &eax, &edx) ||
> +	     wrmsr_amd_safe(MSR_AMD_THRM_FEATURE_MASK, thermal_ecx, edx))){
> +		skip_thermal_ecx = 1;
> +		printk("Failed to set CPUID thermal/power feature mask\n");
>   	}
>   }
>   

With these changes, I guess no one is using 'wrmsr_amd' now.
So why not remove those bits as part of this patch?

Thanks,
-Aravind.