Re: [PATCH v10 05/10] xen: Add vmware_port support

[Don Slutz <dslutz@verizon.com>]

On 05/19/15 16:23, Andrew Cooper wrote:
> On 15/05/15 00:34, Don Slutz wrote:
>> This includes adding is_vmware_port_enabled
>>


>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index bc3d3a5..153048a 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -519,7 +519,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>          (domcr_flags & DOMCRF_hap);
>>      d->arch.hvm_domain.mem_sharing_enabled = 0;
>>      if ( config )
>> +    {
>>          d->arch.hvm_domain.vmware_hwver = config->vmware_hwver;
>> +        d->arch.hvm_domain.is_vmware_port_enabled =
>> +            !!(config->arch_flags & XEN_DOMCTL_CONFIG_VMWARE_PORT_MASK);
>> +    }
> 
> This hunk is also subject to my rebase request from patch 2.
>

Yes, will re-base.


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


>> +static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>> +{
>> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>> +
>> +    /*
>> +     * While VMware expects only 32-bit in, they do support using
>> +     * other sizes and out.  However they do require only the 1 port
>> +     * and the correct value in eax.  Since some of the data
>> +     * returned in eax is smaller the 32 bits and/or you only need
>> +     * the other registers the dir and bytes do not need any
>> +     * checking.  The caller will handle the bytes, and dir is
>> +     * handled below for eax.
>> +     */
>> +    if ( port == BDOOR_PORT && regs->_eax == BDOOR_MAGIC )
>> +    {
>> +        uint32_t new_eax = ~0u;
>> +        uint64_t value;
>> +        struct vcpu *curr = current;
>> +        struct domain *currd = curr->domain;
>> +
>> +        ASSERT(is_hvm_domain(currd));
> 
> You will not be getting here for a non HVM domain, but there is nothing
> anti PVH here.  I would drop the assert.
> 

Ok, will drop.

>> +        /*
>> +         * VMware changes the other (non eax) registers ignoring dir
>> +         * (IN vs OUT).  It also changes only the 32-bit part
>> +         * leaving the high 32-bits unchanged, unlike what one would
>> +         * expect to happen.
>> +         */
>> +        switch ( regs->_ecx & 0xffff )
>> +        {
>> +        case BDOOR_CMD_GETMHZ:
>> +            new_eax = currd->arch.tsc_khz / 1000;
>> +            break;
> 
> Newlines after break statements please.
> 

Will add.

>> +        case BDOOR_CMD_GETVERSION:
>> +            /* MAGIC */
>> +            regs->_ebx = BDOOR_MAGIC;
...
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>  I
> have not paid any attention to the implementation of the backdoor port
> handler, but everything else seems ok.
> 

Thanks.
   -Don Slutz

> ~Andrew
>