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

~Andrew