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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.