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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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) {

This doesn't match up with any of the items listed above. Am I to
imply that models 0 and 1 have this CPUID leaf non-blank, but can't
mask the features?

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.

> +        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.

Jan


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