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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

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@xxxxxxxxxx>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx>
>
> ---
> 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@xxxxxxx>
> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel