Re: Fwd: [PATCH RFC 3/4] x86/AMD: support further feature masking MSRs

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

On 4/4/2014 4:37 AM, Jan Beulich wrote:
>>>> On 04.04.14 at 00:44, <aravind.gopalakrishnan@amd.com> 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 <jbeulich@suse.com
>>>> <mailto:jbeulich@suse.com>>
>>>>
>>>> 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