Re: [Xen-devel] [PATCH 4/6] x86/hvm: Move hvm_funcs.cpuid_intercept() handling into hvm_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/11/16 15:20, Jan Beulich wrote:
>>>> On 16.11.16 at 13:31, <andrew.cooper3@xxxxxxxxxx> wrote:
>> @@ -3676,6 +3671,12 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
>> unsigned int *ebx,
>>              if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
>>                  *edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
>>          }
>> +
>> +        /* SYSCALL is hidden outside of long mode on Intel. */
>> +        if ( d->arch.x86_vendor == X86_VENDOR_INTEL &&
>> +             !hvm_long_mode_enabled(v))
>> +            *edx &= ~cpufeat_mask(X86_FEATURE_SYSCALL);
> Original code also set the bit in its opposite path - don't you think
> this should be retained (or otherwise the change be explained in
> the description)?

Yeah - that was conceptually wrong.  It is legitimate (although
unlikely) for an admin to explicitly configure the VM to hide syscall
even in long mode, as they have distinct feature bits.

In reality however, SYSCALL already set in edx by this point.  I will
update the commit message.

>
>> @@ -3700,6 +3701,14 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
>> unsigned int *ebx,
>>  
>>          *ebx &= hvm_featureset[FEATURESET_e8b];
>>          break;
>> +
>> +    case 0x8000001c:
>> +        if ( !(v->arch.xcr0 & XSTATE_LWP) )
>> +            *eax = 0;
>> +        else if ( cpu_has_svm && cpu_has_lwp )
>> +            /* Turn on available bit and other features specified in 
>> lwp_cfg. */
>> +            *eax = (*edx & v->arch.hvm_svm.guest_lwp_cfg) | 1;
>> +        break;
> Wouldn't it be better to do
>
> +        if ( cpu_has_svm && cpu_has_lwp && (v->arch.xcr0 & XSTATE_LWP) )
> +            /* Turn on available bit and other features specified in 
> lwp_cfg. */
> +            *eax = (*edx & v->arch.hvm_svm.guest_lwp_cfg) | 1;
> +        else
> +            *eax = 0;
>
> the more that you're (slightly) altering original behavior anyway?

That would be slightly better.

~Andrew

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