From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH v2] x86/amd: Protect set_cpuidmask() against #GP faults Date: Thu, 5 Jun 2014 10:40:26 -0500 Message-ID: <53908F6A.6090904@amd.com> References: <5390A57C0200007800018583@mail.emea.novell.com> <1401981802-19350-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1401981802-19350-1-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Xen-devel Cc: Suravee Suthikulpanit , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 > Reviewed-by: Boris Ostrovsky > CC: Jan Beulich > CC: Suravee Suthikulpanit > CC: Aravind Gopalakrishnan > > --- > 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 > 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.