From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: Fwd: [PATCH RFC 3/4] x86/AMD: support further feature masking MSRs Date: Fri, 4 Apr 2014 11:21:53 -0500 Message-ID: <533EDC21.8070603@amd.com> References: <532880230200007800125450@nat28.tlf.novell.com> <53296E90020000780012593B@nat28.tlf.novell.com> <532975D6020000780012598D@nat28.tlf.novell.com> <533B4772.2020008@amd.com> <533DE43B.1020502@amd.com> <533E9975020000780000590E@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WW6sU-000823-2L for xen-devel@lists.xenproject.org; Fri, 04 Apr 2014 16:22:02 +0000 In-Reply-To: <533E9975020000780000590E@nat28.tlf.novell.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: Jan Beulich , "xen-devel@lists.xenproject.org" Cc: Keir Fraser , Ian.Jackson@eu.citrix.com, ian.campbell@citrix.com, Suravee Suthikulpanit , Sherry Hurwitz List-Id: xen-devel@lists.xenproject.org On 4/4/2014 4:37 AM, Jan Beulich wrote: >>>> On 04.04.14 at 00:44, wrote: >> On 4/1/2014 6:10 PM, Aravind Gopalakrishnan wrote: >>>> Newer AMD CPUs also allow masking CPUID leaf 6 ECX and CPUID leaf 7 >>>> sub-leaf 0 EAX and EBX. >>>> >>>> Signed-off-by: Jan Beulich >>> > >>>> >>>> TBD: >>>> - Fam15M0x is documented to not have MSR C0011002 despite >>>> CPUID[7,0].EBX != 0 from model 02 >>>> onwards, and contrary to what I see on the system I have access to >>>> (which is model 02) >>>> - Fam12, Fam14, and Fam16 are documented to not have MSR C0011003 >>>> despite CPUID[6].ECX != 0 >>> Fam10 too has cpuid[6].ecx != 0 but no MSR C0011003 >>> >>>> - Fam11 is documented to not even have MSR C0011004 and C0011005 >>>> >>> I am still trying to get some clarity on this; >> Here's more info regarding your questions: >> 1. This is a documentation error. >> 2. We cannot mask cpuid[6].ecx on any of these families: 0x10, 0x11, >> 0x12,0x14,0x16 as feature is not supported.. >> 3. We cannot mask standard,extended cpuid feature bits on Fam0x11 as >> feature is not supported. >> >> So simple enough additions to your patch to include some family checks: >> >> + if (c->cpuid_level >= 7) >> + cpuid_count(7, 0, &eax, &ebx, &ecx, &edx); >> + else >> + ebx = eax = 0; >> + if ((eax | ebx) && ~(l7s0_eax & l7s0_ebx) && >> + c->x86 == 0x15 && c->x86_model >= 0x2) { > Also, is Fam16 indeed not capable of masking features here too? > > Assuming "yes" and "no", I'd rather use > > if ((eax | ebx) && ~(l7s0_eax & l7s0_ebx) && > (c->x86 != 0x15 || c->x86_model >= 0x2)) { > > But then again models 0 and 1 are documented to have this CPUID > leaf blank, so we shouldn't need a family/model check here at all. Yes, you are right. No need for above family check at all. But for fam16h, I was not able to access the MSR on my machine. And here's why: fam16h can use the MSR to mask the bit only if microcode patch level >= 0x700010a While at it, I tried to clean up the issues you pointed above (and the == 0x15 check below..). Does this seem better? - + if (c->cpuid_level >= 7) + cpuid_count(7, 0, &eax, &ebx, &ecx, &edx); + else + ebx = eax = 0; + + skip_l7s0_eax_ebx = skip_thermal_ecx = 1; + if ((eax | ebx) && ~(l7s0_eax & l7s0_ebx)) { + /* fam16h can mask l7s0 only if microcode lvl >= 0x700010a */ + if (c->x86 == 0x16) { + uint32_t rev; + rdmsrl(MSR_AMD_PATCHLEVEL, rev); + if (rev < 0x700010a) { + printk("Cannot apply l7s0 mask as microcode patch lvl is insufficient\n"); + /* + * fam16h cannot mask MSR_AMD_THRM_FEATURE_MASK anyway + * so, fall through + */ + goto setmask; + } + } + if (l7s0_eax > eax) + l7s0_eax = eax; + l7s0_ebx &= ebx; + skip_l7s0_eax_ebx = 0; + printk("Writing CPUID leaf 7 subleaf 0 feature mask EAX:EBX -> %08Xh:%08Xh\n", + l7s0_eax, l7s0_ebx); + } + ecx = c->cpuid_level >= 6 ? cpuid_ecx(6) : 0; + /* MSR_AMD_THRM_FEATURE_MASK can be applied only on fam15h */ + if (c->x86 != 0x15) + goto setmask; + if (ecx && ~thermal_ecx) { + thermal_ecx &= ecx; + skip_thermal_ecx = 0; + printk("Writing CPUID thermal/power feature mask ECX -> %08Xh\n", + thermal_ecx); + } >> + if (l7s0_eax > eax) >> + l7s0_eax = eax; >> + l7s0_ebx &= ebx; >> + printk("Writing CPUID leaf 7 subleaf 0 feature mask EAX:EBX -> %08Xh:%08Xh\n", >> + l7s0_eax, l7s0_ebx); >> + } else >> + skip_l7s0_eax_ebx = 1; >> + ecx = c->cpuid_level >= 6 ? cpuid_ecx(6) : 0; >> + if (ecx && ~thermal_ecx && c->x86 == 0x15) { > Will merge this, albeit I don't like == in checks like this at all. > >> + thermal_ecx &= ecx; >> + printk("Writing CPUID thermal/power feature mask ECX -> %08Xh\n", >> + thermal_ecx); >> + } else >> + skip_thermal_ecx = 1; >> setmask: >> - /* FIXME check if processor supports CPUID masking */ >> /* AMD processors prior to family 10h required a 32-bit password */ >> - if (c->x86 >= 0x10) { >> + if (c->x86 >= 0x10 && c->x86 != 0x11) { > I'll use this one, but in a separate prefix patch (as I'll want to backport > this), and in a different fashion: I don't think the "else" path to this "if" > applies to Fam11 either based on documentation and your statements > above. > Yes, missed it. Sorry about that.. Thanks, -Aravind