Re: [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT

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

On 02/10/14 19:36, Don Slutz wrote:
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
> v2:
>     Fixup usage of hvmtrace_io_assist().
>     VMware only changes the 32bit part of the register.
>        Added vmware_ioreq_t
>
>  xen/arch/x86/hvm/emulate.c        | 72 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/io.c             | 19 +++++++++++
>  xen/arch/x86/hvm/vmware/vmport.c  | 24 ++++++++++++-
>  xen/include/asm-x86/hvm/emulate.h |  2 ++
>  xen/include/asm-x86/hvm/vcpu.h    |  1 +
>  xen/include/public/hvm/ioreq.h    | 19 +++++++++++
>  6 files changed, 136 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index c0f47d2..215f049 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -50,6 +50,78 @@ static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>      trace_var(event, 0/*!cycles*/, size, buffer);
>  }
>  
> +int hvmemul_do_vmport(uint16_t addr, int size, int dir,
> +                      struct cpu_user_regs *regs)
> +{
> +    struct vcpu *curr = current;
> +    struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> +    vmware_ioreq_t p = {
> +        .type = IOREQ_TYPE_VMWARE_PORT,
> +        .addr = addr,
> +        .size = size,
> +        .dir = dir,
> +        .eax = regs->rax,
> +        .ebx = regs->rbx,
> +        .ecx = regs->rcx,
> +        .edx = regs->rdx,
> +        .esi = regs->rsi,
> +        .edi = regs->rdi,
> +    };
> +    ioreq_t *pp = (ioreq_t *)&p;
> +    ioreq_t op;

Eww.

Just because the C type system lets you abuse it like this doesn't mean
it is a clever idea to.  Please refer to c/s 15a9f34d1b as an example of
the kinds of bugs it causes.

> +
> +    ASSERT(sizeof(p) == sizeof(op));
> +    ASSERT(offsetof(ioreq_t, type) == offsetof(vmware_ioreq_t, type));
> +    ASSERT(offsetof(ioreq_t, vp_eport) == offsetof(vmware_ioreq_t, vp_eport));

These can be evaluated by the compiler.  They should be BUILD_BUG_ON()s
rather than ASSERT()s.

> +
> +    switch ( vio->io_state )
> +    {
> +    case HVMIO_none:
> +        break;
> +    case HVMIO_completed:
> +        vio->io_state = HVMIO_none;
> +        goto finish_access_vmport;
> +    case HVMIO_dispatched:
> +        /* May have to wait for previous cycle of a multi-write to complete. */
> +    default:
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    if ( hvm_io_pending(curr) )
> +    {
> +        gdprintk(XENLOG_WARNING, "WARNING: io already pending?\n");
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    vio->io_state = HVMIO_handle_vmport_awaiting_completion;
> +    vio->io_size = size;
> +
> +    /* If there is no backing DM, just ignore accesses */
> +    if ( !hvm_has_dm(curr->domain) )
> +    {
> +        vio->io_state = HVMIO_none;
> +        vio->io_data = ~0ul;
> +    }
> +    else
> +    {
> +        if ( !hvm_send_assist_req(pp) )
> +            vio->io_state = HVMIO_none;
> +        return X86EMUL_RETRY;
> +    }
> +
> + finish_access_vmport:
> +    memset(&op, 0, sizeof(op));
> +    op.dir = dir;
> +    op.addr = (uint16_t)regs->rdx;
> +    op.data_is_ptr = 0;
> +    op.data = vio->io_data;

Most of this can be achieved with struct initialisation similar to 'p'
above.

> +    hvmtrace_io_assist(0, &op);
> +
> +    memcpy(&regs->rax, &vio->io_data, vio->io_size);

This is going to need some justification.  Clobbering the registers like
this looks bogus.

> +
> +    return X86EMUL_OKAY;
> +}
> +
>  static int hvmemul_do_io(
>      int is_mmio, paddr_t addr, unsigned long *reps, int size,
>      paddr_t ram_gpa, int dir, int df, void *p_data)
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index 68fb890..a8bf324 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -197,6 +197,25 @@ void hvm_io_assist(ioreq_t *p)
>          else
>              memcpy(&guest_cpu_user_regs()->rax, &p->data, vio->io_size);
>          break;
> +    case HVMIO_handle_vmport_awaiting_completion:
> +    {
> +        struct cpu_user_regs *regs = guest_cpu_user_regs();
> +        vmware_ioreq_t *vp = (vmware_ioreq_t *)p;
> +
> +        ASSERT(sizeof(*p) == sizeof(*vp));
> +        ASSERT(offsetof(ioreq_t, type) == offsetof(vmware_ioreq_t, type));
> +        ASSERT(offsetof(ioreq_t, vp_eport) == offsetof(vmware_ioreq_t, vp_eport));
> +        
> +        /* Always zero extension for eax */
> +        regs->rax = vp->eax;
> +        /* Only change the 32bit part of the register */
> +        regs->rbx = (regs->rbx & 0xffffffff00000000ull) | vp->ebx;

regs->_e?? allows you to directly access the lower half, avoiding these
bit manipulations.

~Andrew

> +        regs->rcx = (regs->rcx & 0xffffffff00000000ull) | vp->ecx;
> +        regs->rdx = (regs->rdx & 0xffffffff00000000ull) | vp->edx;
> +        regs->rsi = (regs->rsi & 0xffffffff00000000ull) | vp->esi;
> +        regs->rdi = (regs->rdi & 0xffffffff00000000ull) | vp->edi;
> +    }
> +        break;
>      default:
>          break;
>      }
> diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
> index 962ee32..0649106 100644
> --- a/xen/arch/x86/hvm/vmware/vmport.c
> +++ b/xen/arch/x86/hvm/vmware/vmport.c
> @@ -147,9 +147,31 @@ int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>              regs->rax = 0x0;
>              break;
>          default:
> +        {   /* Let backing DM handle */
> +            int rc;
> +
>              HVMTRACE_ND(VMPORT_UNKNOWN, 0, 1/*cycles*/, 6,
> -                        (bytes << 8) + dir, cmd, regs->rbx,
> +                        (bytes << 8) | dir, cmd, regs->rbx,
>                          regs->rcx, regs->rsi, regs->rdi);
> +            rc = hvmemul_do_vmport(BDOOR_PORT, bytes, dir, regs);
> +            switch (rc)
> +            {
> +            case X86EMUL_OKAY:
> +                break;
> +            case X86EMUL_RETRY:
> +            {
> +                struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> +
> +                if ( vio->io_state != HVMIO_handle_vmport_awaiting_completion )
> +                    return 0;
> +                break;
> +            }
> +            default:
> +                gdprintk(XENLOG_ERR, "Weird HVM ioemulation status %d.\n", rc);
> +                domain_crash(current->domain);
> +                break;
> +            }
> +        }
>              break;
>          }
>  
> diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
> index 5411302..7d18435 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -53,6 +53,8 @@ struct segment_register *hvmemul_get_seg_reg(
>  int hvmemul_do_pio(
>      unsigned long port, unsigned long *reps, int size,
>      paddr_t ram_gpa, int dir, int df, void *p_data);
> +int hvmemul_do_vmport(uint16_t addr, int size, int dir,
> +		      struct cpu_user_regs *regs);
>  
>  void hvm_dump_emulation_state(const char *prefix,
>                                struct hvm_emulate_ctxt *hvmemul_ctxt);
> diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
> index 01e0665..1e63d7f 100644
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -36,6 +36,7 @@ enum hvm_io_state {
>      HVMIO_awaiting_completion,
>      HVMIO_handle_mmio_awaiting_completion,
>      HVMIO_handle_pio_awaiting_completion,
> +    HVMIO_handle_vmport_awaiting_completion,
>      HVMIO_completed
>  };
>  
> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
> index 5b5fedf..7d5ba58 100644
> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -35,6 +35,7 @@
>  #define IOREQ_TYPE_PIO          0 /* pio */
>  #define IOREQ_TYPE_COPY         1 /* mmio ops */
>  #define IOREQ_TYPE_PCI_CONFIG   2
> +#define IOREQ_TYPE_VMWARE_PORT  3
>  #define IOREQ_TYPE_TIMEOFFSET   7
>  #define IOREQ_TYPE_INVALIDATE   8 /* mapcache */
>  
> @@ -48,6 +49,8 @@
>   * 
>   * 63....48|47..40|39..35|34..32|31........0
>   * SEGMENT |BUS   |DEV   |FN    |OFFSET
> + *
> + * For I/O type IOREQ_TYPE_VMWARE_PORT use the vmware_ioreq.
>   */
>  struct ioreq {
>      uint64_t addr;          /* physical address */
> @@ -66,6 +69,22 @@ struct ioreq {
>  };
>  typedef struct ioreq ioreq_t;
>  
> +struct vmware_ioreq {
> +    uint32_t esi;
> +    uint32_t edi;
> +    uint32_t eax;
> +    uint32_t ebx;
> +    uint32_t ecx;
> +    uint32_t edx;
> +    uint32_t vp_eport;      /* evtchn for notifications to/from device model */
> +    uint16_t addr;
> +    uint8_t  state:4;
> +    uint8_t  dir:1;         /* 1=read, 0=write */
> +    uint8_t  size:3;
> +    uint8_t  type;          /* I/O type */
> +};
> +typedef struct vmware_ioreq vmware_ioreq_t;
> +
>  struct shared_iopage {
>      struct ioreq vcpu_ioreq[1];
>  };