From mboxrd@z Thu Jan  1 00:00:00 1970
From: Don Slutz <dslutz@verizon.com>
Subject: Re: [PATCH v10 02/10] xen: Add support for VMware cpuid
	leaves
Date: Wed, 20 May 2015 13:48:23 -0400
Message-ID: <555CC8E7.40707@one.verizon.com>
References: <1431646469-23022-1-git-send-email-dslutz@verizon.com>
	<1431646469-23022-3-git-send-email-dslutz@verizon.com>
	<555B96B8.4000005@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <555B96B8.4000005@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Julien Grall <julien.grall@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Jun Nakajima <jun.nakajima@intel.com>, Eddie Dong <eddie.dong@intel.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, George Dunlap <George.Dunlap@eu.citrix.com>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>, Jan Beulich <jbeulich@suse.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
List-Id: xen-devel@lists.xenproject.org

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
>