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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

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


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