Re: [PATCH v9 05/13] xen: Add vmware_port support

[Don Slutz <dslutz@verizon.com>]

On 02/23/15 10:05, Jan Beulich wrote:
>>>> On 17.02.15 at 00:05, <dslutz@verizon.com> wrote:
>> This includes adding is_vmware_port_enabled
>>
>> This is a new domain_create() flag, DOMCRF_vmware_port.  It is
>> passed to domctl as XEN_DOMCTL_CDF_vmware_port.
> 
> As indicated before, I don't think this is a good use case for a
> domain creation flag. Some of the others we have may not be either,
> but as I have said quite often recently - let's not make the situation
> worse by following bad examples.
> 

I was not sure on the way to go based on the emails.

I will code up a version that has the additional code to make a
HVM_PARAM a write once.  The current way allows multiple write of the
same value and changes from 0 to non-zero.


>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmware/vmport.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * HVM VMPORT emulation
>> + *
>> + * Copyright (C) 2012 Verizon Corporation
>> + *
>> + * This file is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License Version 2 (GPLv2)
>> + * as published by the Free Software Foundation.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details. <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/lib.h>
>> +#include <asm/hvm/hvm.h>
>> +#include <asm/hvm/support.h>
>> +#include <asm/hvm/vmport.h>
>> +
>> +#include "backdoor_def.h"
>> +
>> +void vmport_register(struct domain *d)
>> +{
>> +    register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
>> +}
>> +
>> +int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
> 
> static
> 

Andrew already pointed this out.

>> +{
>> +    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.
>> +     */
>> +    if ( port == BDOOR_PORT && regs->_eax == BDOOR_MAGIC )
>> +    {
>> +        uint32_t new_eax = BDOOR_MAGIC;
>> +        uint64_t value;
>> +        struct vcpu *curr = current;
>> +        struct domain *d = curr->domain;
> 
> currd
> 

Will change.

>> +
>> +        switch ( regs->_ecx & 0xffff )
>> +        {
>> +        case BDOOR_CMD_GETMHZ:
>> +            new_eax = d->arch.tsc_khz / 1000;
>> +            break;
>> +        case BDOOR_CMD_GETVERSION:
>> +            /* MAGIC */
>> +            regs->_ebx = BDOOR_MAGIC;
>> +            /* VERSION_MAGIC */
>> +            new_eax = 6;
>> +            /* Claim we are an ESX. VMX_TYPE_SCALABLE_SERVER */
>> +            regs->_ecx = 2;
> 
> Are you sure you don't want to zero the high halves of 64-bit
> registers here?
> 

Yes, VMware does not zero the high halves.

>> +            break;
>> +        case BDOOR_CMD_GETSCREENSIZE:
>> +            /* We have no screen size */
>> +            new_eax = ~0u;
> 
> Then just have this handled into the default case.
> 

Will do.

>> +            break;
>> +        case BDOOR_CMD_GETHWVERSION:
>> +            /* vmware_hw */
>> +            ASSERT(is_hvm_vcpu(curr));
> 
> is_hvm_domain(currd)
> 

Ok.

> And - why here rather than before the switch() or even right at the
> start of the function?
> 

Since this was the only place it was needed.  Will move to the start.

>> +            {
>> +                struct hvm_domain *hd = &d->arch.hvm_domain;
>> +
>> +                new_eax = hd->params[HVM_PARAM_VMWARE_HWVER];
>> +            }
>> +            /*
>> +             * Returning zero is not the best.  VMware was not at
>> +             * all consistent in the handling of this command until
>> +             * VMware hardware version 4.  So it is better to claim
>> +             * 4 then 0.  This should only happen in strange configs.
>> +             */
>> +            if ( !new_eax )
>> +                new_eax = 4;
>> +            break;
>> +        case BDOOR_CMD_GETHZ:
>> +        {
>> +            struct segment_register sreg;
>> +
>> +            hvm_get_segment_register(curr, x86_seg_ss, &sreg);
>> +            if ( sreg.attr.fields.dpl == 0 )
>> +            {
>> +                value = d->arch.tsc_khz * 1000;
>> +                /* apic-frequency (bus speed) */
>> +                regs->_ecx = 1000000000ULL / APIC_BUS_CYCLE_NS;
>> +                /* High part of tsc-frequency */
>> +                regs->_ebx = value >> 32;
>> +                /* Low part of tsc-frequency */
>> +                new_eax = value;
>> +            }
>> +            break;
>> +        }
>> +        case BDOOR_CMD_GETTIME:
>> +            value = get_localtime_us(d) - d->time_offset_seconds * 1000000ULL;
>> +            /* hostUsecs */
>> +            regs->_ebx = value % 1000000UL;
>> +            /* hostSecs */
>> +            new_eax = value / 1000000ULL;
>> +            /* maxTimeLag */
>> +            regs->_ecx = 1000000;
>> +            /* offset to GMT in minutes */
>> +            regs->_edx = d->time_offset_seconds / 60;
>> +            break;
>> +        case BDOOR_CMD_GETTIMEFULL:
>> +            value = get_localtime_us(d) - d->time_offset_seconds * 1000000ULL;
>> +            /* hostUsecs */
>> +            regs->_ebx = value % 1000000UL;
>> +            /* hostSecs low 32 bits */
>> +            regs->_edx = value / 1000000ULL;
>> +            /* hostSecs high 32 bits */
>> +            regs->_esi = (value / 1000000ULL) >> 32;
>> +            /* maxTimeLag */
>> +            regs->_ecx = 1000000;
>> +            break;
> 
> No setting of new_eax?
> 

This is using the "new_eax = BDOOR_MAGIC" above.

>> +        default:
>> +            new_eax = ~0u;
> 
> Why is this not the initializer value for the variable then?
> 

Because of the use above.  Since you ask, I will switch the usage.

>> +            break;
>> +        }
>> +        if ( dir == IOREQ_READ )
>> +            *val = new_eax;
> 
> With that, is it really correct that OUT updates the other registers
> just like IN? If so, this deserves a comment, so that readers won't
> think this is in error.
> 

Yes.  Will add a comment about this.

> Jan
>