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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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");
>   	}
>   }
>