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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.15 at 00:05, <dslutz@xxxxxxxxxxx> 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.

> --- /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

> +{
> +    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

> +
> +        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?

> +            break;
> +        case BDOOR_CMD_GETSCREENSIZE:
> +            /* We have no screen size */
> +            new_eax = ~0u;

Then just have this handled into the default case.

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

is_hvm_domain(currd)

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

> +            {
> +                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?

> +        default:
> +            new_eax = ~0u;

Why is this not the initializer value for the variable then?

> +            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.

Jan

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