Re: [PATCH v10 02/10] xen: Add support for VMware cpuid leaves

[Don Slutz <dslutz@verizon.com>]

On 05/19/15 16:02, Andrew Cooper wrote:
> On 15/05/15 00:34, Don Slutz wrote:
>> This is done by adding xen_arch_domainconfig vmware_hw. It is set to
>> the VMware virtual hardware version.
>>
...
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> v10:
>>     Did not add "Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>"
>>     because of changes here to do things the new way.
>>   Reword comment message to reflect new way.
>
> In which case by above tag doesn't count.

Just as I expected.  I am assuming that Reviewed-by below does apply.

>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 1f1550e..bc3d3a5 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -518,6 +518,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>          hvm_funcs.hap_supported &&
>>          (domcr_flags & DOMCRF_hap);
>>      d->arch.hvm_domain.mem_sharing_enabled = 0;
>> +    if ( config )
>> +        d->arch.hvm_domain.vmware_hwver = config->vmware_hwver;
> 
> Urgh - as a result of this I have found a differet bug in this
> function.  Please rebase this change over my bugfix patch which I will
> post shortly.
> 

Ok,  Will re-base.

>>
>>      d->arch.s3_integrity = !!(domcr_flags & DOMCRF_s3_integrity);
>>

>>
>> @@ -5656,6 +5660,13 @@ static int hvm_allow_set_param(struct domain *d,
>>      {
>>      /* The following parameters should only be changed once. */
>>      case HVM_PARAM_VIRIDIAN:
>> +        /* Disallow if vmware_hwver */
> 
> "is in use" or "is enabled"
> 

Will do.

>> +        if ( d->arch.hvm_domain.vmware_hwver )
>> +        {
>> +            rc = -EOPNOTSUPP;
>> +            break;
>> +        }

>> +    switch ( idx - 0x40000000 )
>> +    {
>> +    case 0x0:
>> +        *eax = 0x40000010;  /* Largest leaf */
>> +        *ebx = 0x61774d56;  /* "VMwa" */
>> +        *ecx = 0x4d566572;  /* "reVM" */
>> +        *edx = 0x65726177;  /* "ware" */
>> +        break;
> 
> Newline here please.
> 

Ok, and also below.

>> +    case 0x10:
>> +        /* (Virtual) TSC frequency in kHz. */
>> +        *eax =  d->arch.tsc_khz;
>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +        *ebx = 1000000ull / APIC_BUS_CYCLE_NS;
>> +        *ecx = 0;          /* Reserved */
>> +        *edx = 0;          /* Reserved */
>> +        break;
> 
> And here please.
> 
>> +    default:
>> +        return 0;
>> +    }
>> +
>> +    return 1;
>> +}
>> +

> 
> Don:  Feel free to retain my Reviewed-by, subject to the adjustments listed.
> 

Ok,
  Thanks.

   -Don Slutz

> ~Andrew
>