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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/04/14 07:54, Jan Beulich wrote:
>>>> On 04.04.14 at 18:21, <aravind.gopalakrishnan@amd.com> wrote:
>> 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
> I think that's going too far - no-one is required to pass these command
> line options, and all that would happen if someone did was that the
> system would crash during boot. The one thing we may want to check
> (and adjust if necessary) is that this code only runs after an eventual
> microcode update was already applied, to increase the chances of
> things working. After all, if a minimum microcode level is known, I
> suppose respective microcode updates are or will be made available
> publicly.
>
> Andrew otoh, in his attempt to make the feature masking per-VM,
> will clearly need to take this into consideration, or else he'd be
> risking runtime crashes.

That is a good point which I had not anticipated.  The way my fragments
of per-VM code currently works involve explicitly probing the expected
set of MSRs at boot, disabling if not found.  In a case like this, the
fam16 would not get the masks if the microcode started below 0x700010a
but subsequently got patched higher.

Catching this case at runtime is hard - if the command line option is
given, the BIOS is out of date wrt microcode, dom0 boots and applies new
microcode, it is not generally safe to retroactively apply the boot time
mask to dom0 which has already started running.

~Andrew