Re: [Xen-devel] [PATCH 08/27] x86/hvm: Dispatch cpuid_viridian_leaves() from guest_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/01/17 15:24, Jan Beulich wrote:
>>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>> One check against EFER_SVME is replaced with the more appropriate 
>> cpu_has_svm,
>> when determining whether MSR bitmaps are available.
> I don't think this is correct - start_svm() may fail, in which case
> the CPUID flag doesn't get cleared, yet EFER.SVME also doesn't
> get set. How about comparing hvm_funcs (if not NULL) ->name
> against "SVM"?

Hmm.  This shows that the same logical bug is present in the vmx side. 
Let me see about finding a better way of doing this.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>

Oops yes sorry.

>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -319,8 +319,21 @@ int init_domain_cpuid_policy(struct domain *d)
>>  void guest_cpuid(const struct vcpu *v, unsigned int leaf,
>>                   unsigned int subleaf, struct cpuid_leaf *res)
>>  {
>> +    const struct domain *d = v->domain;
>> +
>>      *res = EMPTY_LEAF;
>>  
>> +    /*
>> +     * First pass:
>> +     * - Dispatch the virtualised leaves to their respective handlers.
>> +     */
>> +    switch ( leaf )
>> +    {
>> +    case 0x40000000 ... 0x400000ff:
>> +        if ( is_viridian_domain(d) )
>> +            return cpuid_viridian_leaves(v, leaf, subleaf, res);
>> +    }
> Can we please have a break statement above here?

I think this got lost in a rebase.  The following patch makes it all
sensible.  I will adjust.

>
>> +void cpuid_viridian_leaves(const struct vcpu *v, unsigned int leaf,
>> +                           unsigned int subleaf, struct cpuid_leaf *res)
>>  {
>> -    struct domain *d = current->domain;
>> +    const struct domain *d = v->domain;
>>  
>> -    if ( !is_viridian_domain(d) )
>> -        return 0;
>> +    ASSERT(is_viridian_domain(d));
>> +    ASSERT(leaf >= 0x40000000 && leaf < 0x40000100);
>>  
>>      leaf -= 0x40000000;
>> -    if ( leaf > 6 )
>> -        return 0;
>>  
>> -    *eax = *ebx = *ecx = *edx = 0;
>>      switch ( leaf )
>>      {
>>      case 0:
>> -        *eax = 0x40000006; /* Maximum leaf */
>> -        *ebx = 0x7263694d; /* Magic numbers  */
>> -        *ecx = 0x666F736F;
>> -        *edx = 0x76482074;
>> +        res->a = 0x40000006; /* Maximum leaf */
>> +        res->b = 0x7263694d; /* Magic numbers  */
>> +        res->c = 0x666F736F;
>> +        res->d = 0x76482074;
>>          break;
>> +
>>      case 1:
>> -        *eax = 0x31237648; /* Version number */
>> +        res->a = 0x31237648; /* Version number */
>>          break;
>> +
>>      case 2:
>>          /* Hypervisor information, but only if the guest has set its
>>             own version number. */
>>          if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
>>              break;
>> -        *eax = 1; /* Build number */
>> -        *ebx = (xen_major_version() << 16) | xen_minor_version();
>> -        *ecx = 0; /* SP */
>> -        *edx = 0; /* Service branch and number */
>> +        res->a = 1; /* Build number */
>> +        res->b = (xen_major_version() << 16) | xen_minor_version();
> I think the comments warrant the zeroing of ECX and EDX to be
> retained.

Ok.

~Andrew

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