Re: [PATCH v2 1/2] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work

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

On 09/09/14 13:43, Ian Campbell wrote:
> On Tue, 2014-09-09 at 13:29 +0100, Andrew Cooper wrote:
>> On 09/09/14 13:21, Ian Campbell wrote:
>>> On Tue, 2014-09-09 at 11:45 +0100, Jan Beulich wrote:
>>>>>>> On 09.09.14 at 06:31, <alfred.z.song@gmail.com> wrote:
>>>>> @@ -195,16 +186,14 @@ static void intel_xc_cpuid_policy(
>>>>>          break;
>>>>>  
>>>>>      case 0x80000001: {
>>>>> -        int is_64bit = hypervisor_is_64bit(xch) && is_pae;
>>>>> -
>>>>>          /* Only a few features are advertised in Intel's 0x80000001. */
>>>>> -        regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) |
>>>>> -                               bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
>>>>> -                               bitmaskof(X86_FEATURE_ABM);
>>>>> -        regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
>>>>> -                    (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
>>>>> -                    (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
>>>>> -                    (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
>>>>> +        regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) |
>>>>> +                    bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
>>>>> +                    bitmaskof(X86_FEATURE_ABM);
>>>>> +        regs[3] &= (bitmaskof(X86_FEATURE_NX) |
>>>>> +                    bitmaskof(X86_FEATURE_LM) |
>>>>> +                    (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
>>>>> +                    (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
>>>> As said before, tying these two features to is_pae seems a
>>>> little strange, but if the tools maintainers can live with that, I
>>>> guess I can too (short of having a better suggestion other
>>>> than to drop the conditionals altogether).
>>> Patch #2 here seems to remove it from the RDTSCP, surely that should be
>>> folded in.
>>>
>>> I also don't understand the link between PAE and the presence of
>>> SYSCALL.
>> On Intel, syscall is strictly only available in long mode, being an AMD
>> instruction mandated in the 64bit spec.
>>
>> is_64bit is disappearing as Xen is unconditionally 64bit these days, but
>> preventing the guest using PAE will preclude it being able to enter long
>> mode.
>>
>> I would agree that it is not necessarily obvious, and based on this
>> consideration, I think it would be better to keep the variable
>> "is_64bit" as it is more informative than "is_pae" in the contexts used.
> But right above we are advertising X86_FEATURE_LM unconditionally, so
> what is to stop the guest switching to long mode and therefore using
> syscall?

I made the same mistake.  That is setting FEATURE_LM in an AND mask,
where the underlying bit may or may not be set from the common logic. 
It is certainly not obvious.

>
> Does real h/w change the cpuid features reported depending on the
> current processor mode? 

It is not the current processor mode.  All of this logic is run during
domain create, before hvmloader starts.

>
> One other bit of confusion I'm having is whether is_pae refers to the
> guest or the host. Previously is_64bit seemed to be a hybrid of both...
> Ian.
>

It kind of was.  It was a hybrid of "Xen is 64bit" (i.e. will permit the
guest to move into long mode if it tries), and "we have advertised pae
to the guest" which is a prerequisite to enter long mode.

All this code is nasty.

~Andrew