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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/02/15 23:05, Don Slutz 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.
>
> This enables limited support of VMware's hyper-call.
>
> This is both a more complete support then in currently provided by
> QEMU and/or KVM and less.  The missing part requires QEMU changes
> and has been left out until the QEMU patches are accepted upstream.
>
> VMware's hyper-call is also known as VMware Backdoor I/O Port.
>
> Note: this support does not depend on vmware_hw being non-zero.
>
> Summary is that VMware treats "in (%dx),%eax" (or "out %eax,(%dx)")
> to port 0x5658 specially.  Note: since many operations return data
> in EAX, "in (%dx),%eax" is the one to use.  The other lengths like
> "in (%dx),%al" will still do things, only AL part of EAX will be
> changed.  For "out %eax,(%dx)" of all lengths, EAX will remain
> unchanged.
>
> An open source example of using this is:
>
> http://open-vm-tools.sourceforge.net/
>
> Which only uses "inl (%dx)".  Also
>
> http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
>
> Some of the best info is at:
>
> https://sites.google.com/site/chitchatvmback/backdoor
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
> v9:
>   Switch to x86_emulator to handle #GP code moved to next patch.
>     Can you explain why a HVM param isn't suitable here?
>       Issue with changing QEMU on the fly.
>       Andrew Cooper: My recommendation is still to use a creation flag
>         So no change.
>     Please move SVM's identical definition into ...
>       Did this as #1.  No longer needed, but since the patch was ready
>       I have included it.
>     --Lots of questions about code that no long is part of this patch. --
>     With this, is handling other than 32-bit in/out really
>     meaningful/correct?
>       Added comment about this.
>     Since you can't get here for PV, I can't see what you need this.
>       Changed to an ASSERT.
>     Why version 4?
>       Added comment about this.
>     -- Several questions about register changes.
>       Re-coded to use new_eax and set *val to this.
>       Change to generealy use reg->_e..
>     These ei1/ei2 checks belong in the callers imo -
>       Moved.
>     the "port" function parameter isn't even checked
>       Add check for exact match.
>     If dropping the code is safe without also forbidding the
>     combination of nested and VMware emulation.
>       Added the forbidding the combination of nested and VMware.
>       Mostly do to the cases of the nested virtual code is the one
>       to handle VMware stuff if needed, not the root one.  Also I am
>       having issues testing xen nested in xen and using hvm.
>
> v7:
>       More on AMD in the commit message.
>       Switch to only change 32bit part of registers, what VMware
>         does.
>     Too much logging and tracing.
>       Dropped a lot of it.  This includes vmport_debug=
>
> v6:
>       Dropped the attempt to use svm_nextrip_insn_length via
>       __get_instruction_length (added in v2).  Just always look
>       at upto 15 bytes on AMD.
>
> v5:
>       we should make sure that svm_vmexit_gp_intercept is not executed for
>       any other guest.
>         Added an ASSERT on is_vmware_port_enabled.
>       magic integers?
>         Added #define for them.
>       I am fairly certain that you need some brackets here.
>         Added brackets.
>
>  xen/arch/x86/domain.c            |   2 +
>  xen/arch/x86/hvm/hvm.c           |  10 +++
>  xen/arch/x86/hvm/vmware/Makefile |   1 +
>  xen/arch/x86/hvm/vmware/vmport.c | 142 +++++++++++++++++++++++++++++++++++++++
>  xen/common/domctl.c              |   3 +
>  xen/include/asm-x86/hvm/domain.h |   3 +
>  xen/include/asm-x86/hvm/vmport.h |  32 +++++++++
>  xen/include/public/domctl.h      |   3 +
>  xen/include/xen/sched.h          |   3 +
>  9 files changed, 199 insertions(+)
>  create mode 100644 xen/arch/x86/hvm/vmware/vmport.c
>  create mode 100644 xen/include/asm-x86/hvm/vmport.h
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index cfe7945..fe45f11 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)
>      d->arch.hvm_domain.mem_sharing_enabled = 0;
>  
>      d->arch.s3_integrity = !!(domcr_flags & DOMCRF_s3_integrity);
> +    d->arch.hvm_domain.is_vmware_port_enabled =
> +        !!(domcr_flags & DOMCRF_vmware_port);
>  
>      INIT_LIST_HEAD(&d->arch.pdev_list);
>  
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index fefd023..812f880 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -61,6 +61,7 @@
>  #include <asm/hvm/trace.h>
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/vmware.h>
> +#include <asm/hvm/vmport.h>
>  #include <asm/mtrr.h>
>  #include <asm/apic.h>
>  #include <public/sched.h>
> @@ -1484,6 +1485,9 @@ int hvm_domain_initialise(struct domain *d)
>          goto fail1;
>      d->arch.hvm_domain.io_handler->num_slot = 0;
>  
> +    if ( d->arch.hvm_domain.is_vmware_port_enabled )
> +        vmport_register(d);
> +
>      if ( is_pvh_domain(d) )
>      {
>          register_portio_handler(d, 0, 0x10003, handle_pvh_io);
> @@ -5859,6 +5863,12 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>                      break;
>                  if ( a.value > 1 )
>                      rc = -EINVAL;
> +                /* Prevent nestedhvm with vmport */
> +                if ( d->arch.hvm_domain.is_vmware_port_enabled )
> +                {
> +                    rc = -EINVAL;

Probably better as EOPNOTSUPP, as it is a configuration problem.

> +                    break;
> +                }
>                  /* Remove the check below once we have
>                   * shadow-on-shadow.
>                   */
> diff --git a/xen/arch/x86/hvm/vmware/Makefile b/xen/arch/x86/hvm/vmware/Makefile
> index 3fb2e0b..cd8815b 100644
> --- a/xen/arch/x86/hvm/vmware/Makefile
> +++ b/xen/arch/x86/hvm/vmware/Makefile
> @@ -1 +1,2 @@
>  obj-y += cpuid.o
> +obj-y += vmport.o
> diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
> new file mode 100644
> index 0000000..3d9f30e
> --- /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)

This function looks as if it should be 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;
> +
> +        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;
> +            break;
> +        case BDOOR_CMD_GETSCREENSIZE:
> +            /* We have no screen size */
> +            new_eax = ~0u;
> +            break;
> +        case BDOOR_CMD_GETHWVERSION:
> +            /* vmware_hw */
> +            ASSERT(is_hvm_vcpu(curr));
> +            {
> +                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 )
> +            {

Why is GETHZ the only one of these with a CPL check?

> +                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;
> +        default:
> +            new_eax = ~0u;
> +            break;
> +        }
> +        if ( dir == IOREQ_READ )
> +            *val = new_eax;
> +    }
> +    else if ( dir == IOREQ_READ )
> +        *val = ~0u;
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-set-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 452f3d9..e242c2d 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -543,6 +543,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>               ~(XEN_DOMCTL_CDF_hvm_guest
>                 | XEN_DOMCTL_CDF_pvh_guest
>                 | XEN_DOMCTL_CDF_hap
> +               | XEN_DOMCTL_CDF_vmware_port
>                 | XEN_DOMCTL_CDF_s3_integrity
>                 | XEN_DOMCTL_CDF_oos_off)) )
>              break;
> @@ -586,6 +587,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              domcr_flags |= DOMCRF_s3_integrity;
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
>              domcr_flags |= DOMCRF_oos_off;
> +        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_vmware_port )
> +            domcr_flags |= DOMCRF_vmware_port;
>  
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref);
>          if ( IS_ERR(d) )
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 0702bf5..ab0e4cf 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -122,6 +122,9 @@ struct hvm_domain {
>      spinlock_t             uc_lock;
>      bool_t                 is_in_uc_mode;
>  
> +    /* VMware backdoor port available */
> +    bool_t                 is_vmware_port_enabled;
> +
>      /* Pass-through */
>      struct hvm_iommu       hvm_iommu;
>  
> diff --git a/xen/include/asm-x86/hvm/vmport.h b/xen/include/asm-x86/hvm/vmport.h
> new file mode 100644
> index 0000000..eb3e472
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/vmport.h
> @@ -0,0 +1,32 @@
> +/*
> + * asm/hvm/vmport.h: 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/>.
> + */
> +
> +#ifndef ASM_X86_HVM_VMPORT_H__
> +#define ASM_X86_HVM_VMPORT_H__
> +
> +void vmport_register(struct domain *d);

I would suggest putting this declaration in hvm.h and forging vmport.h
entirely

~Andrew

> +int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val);
> +
> +#endif /* ASM_X86_HVM_VMPORT_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index b3413a2..28fa767 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -63,6 +63,9 @@ struct xen_domctl_createdomain {
>   /* Is this a PVH guest (as opposed to an HVM or PV guest)? */
>  #define _XEN_DOMCTL_CDF_pvh_guest     4
>  #define XEN_DOMCTL_CDF_pvh_guest      (1U<<_XEN_DOMCTL_CDF_pvh_guest)
> + /* Is VMware backdoor port available? */
> +#define _XEN_DOMCTL_CDF_vmware_port   5
> +#define XEN_DOMCTL_CDF_vmware_port    (1U<<_XEN_DOMCTL_CDF_vmware_port)
>      uint32_t flags;
>  };
>  typedef struct xen_domctl_createdomain xen_domctl_createdomain_t;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ccd7ed8..48bb001 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -546,6 +546,9 @@ struct domain *domain_create(
>   /* DOMCRF_pvh: Create PV domain in HVM container. */
>  #define _DOMCRF_pvh             5
>  #define DOMCRF_pvh              (1U<<_DOMCRF_pvh)
> + /* DOMCRF_vmware_port: Enable use of vmware backdoor port. */
> +#define _DOMCRF_vmware_port     6
> +#define DOMCRF_vmware_port      (1U<<_DOMCRF_vmware_port)
>  
>  /*
>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().