[RFC][PATCH v2 0/1] Add support for Xen access to vmport

[RFC][PATCH v2 0/1] Add support for Xen access to vmport

[Don Slutz]

Changes v1 to v2:
    Fixup usage of hvmtrace_io_assist().
    VMware only changes the 32bit part of the register.

  Stefano Stabellini -- QEMU part
    the registers being passes explicitely by Xen rather than
    "hiding" them into other ioreq fields.
       Added vmware_ioreq_t

Note: to use this with Xen either a version of:

[Qemu-devel] [PATCH] -machine vmport=off: Allow disabling of VMWare ioport emulation

And a manual adjustment so that QEMU is started with vmport=1

or

>From f70663d9fb86914144ba340b6186cb1e67ac6eec Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Fri, 26 Sep 2014 08:11:39 -0400
Subject: [PATCH 1/2] hack: force enable vmport

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/i386/pc_piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 103d756..b76dfbc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -234,7 +234,7 @@ static void pc_init1(MachineState *machine,
     pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled(),
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false,
         0x4);
 
     pc_nic_init(isa_bus, pci_bus);
-- 
1.8.4

needs to be done to QEMU.

Don Slutz (1):
  Add IOREQ_TYPE_VMWARE_PORT

 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(-)

-- 
1.8.4

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

[Don Slutz]

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;
+
+    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));
+
+    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;
+    hvmtrace_io_assist(0, &op);
+
+    memcpy(&regs->rax, &vio->io_data, vio->io_size);
+
+    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->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];
 };
-- 
1.8.4

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

[Andrew Cooper]

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];
>  };

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

[Don Slutz]

On 10/02/14 16:33, Andrew Cooper wrote:
> 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.

This is a direct result of:


Subject: 	Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport
Date: 	Tue, 30 Sep 2014 11:35:44 +0100
From: 	Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: 	Don Slutz <dslutz@verizon.com>
CC: 	Stefano Stabellini <stefano.stabellini@eu.citrix.com>, 
qemu-devel@nongnu.org, xen-devel@lists.xensource.com, Alexander Graf 
<agraf@suse.de>, Andreas Färber <afaerber@suse.de>, Anthony Liguori 
<aliguori@amazon.com>, Marcel Apfelbaum <marcel.a@redhat.com>, Markus 
Armbruster <armbru@redhat.com>, Michael S. Tsirkin <mst@redhat.com>



On Mon, 29 Sep 2014, Don Slutz wrote:
> On 09/29/14 06:25, Stefano Stabellini wrote:
> > On Mon, 29 Sep 2014, Stefano Stabellini wrote:
> > > On Fri, 26 Sep 2014, Don Slutz wrote:
> > > > This adds synchronisation of the vcpu registers
> > > > between Xen and QEMU.

...

> > > > +            CPUX86State *env;
> > > > +            ioreq_t fake_req = {
> > > > +                .type = IOREQ_TYPE_PIO,
> > > > +                .addr = (uint16_t)req->size,
> > > > +                .size = 4,
> > > > +                .dir = IOREQ_READ,
> > > > +                .df = 0,
> > > > +                .data_is_ptr = 0,
> > > > +            };
> > Why do you need a fake req?
>
> To transport the 6 VCPU registers (only 32bits of them) that vmport.c
> needs to do it's work.
>
> > Couldn't Xen send the real req instead?
>
> Yes, but then a 2nd exchange between QEMU and Xen would be needed
> to fetch the 6 VCPU registers.  The ways I know of to fetch the VCPU registers
> from Xen, all need many cycles to do their work and return
> a lot of data that is not needed.
>
> The other option that I have considered was to extend the ioreq_t type
> to have room for these registers, but that reduces by almost half the
> maximum number of VCPUs that are supported (They all live on 1 page).

Urgh. Now that I understand the patch better is think it's horrible, no
offense :-)

Why don't you add another new ioreq type to send out the vcpu state?
Something like IOREQ_TYPE_VCPU_SYNC_REGS? You could send it to QEMU
before IOREQ_TYPE_VMWARE_PORT. Actually this solution looks very imilar
to Alex's suggestion.

...


And the ASSERTs below are the attempt to prevent bugs from being added.

Sigh.  Too much with both XEN and QEMU in hard freeze.  I may
have a way to avoid the cast.

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

I looked for this, but did not find it.  Will change.

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

Ok, will make it a block.   I did it here (and will continue here) because
this is an error path that should not be taken.

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

Copied code from hvmemul_do_io().  Not sure how to justify it.  I have
some ideas on why it is good.

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

Also did not know of these (besides being bad names...)

Will change to use them.

    -Don Slutz

> ~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];
>>   };
>

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

[Andrew Cooper]

On 02/10/2014 22:56, Don Slutz wrote:
> On 10/02/14 16:33, Andrew Cooper wrote:
>> 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.
>
> This is a direct result of:
>
>
> Subject:     Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to
> vmport
> Date:     Tue, 30 Sep 2014 11:35:44 +0100
> From:     Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> To:     Don Slutz <dslutz@verizon.com>
> CC:     Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
> qemu-devel@nongnu.org, xen-devel@lists.xensource.com, Alexander Graf
> <agraf@suse.de>, Andreas Färber <afaerber@suse.de>, Anthony Liguori
> <aliguori@amazon.com>, Marcel Apfelbaum <marcel.a@redhat.com>, Markus
> Armbruster <armbru@redhat.com>, Michael S. Tsirkin <mst@redhat.com>
>
>
>
> On Mon, 29 Sep 2014, Don Slutz wrote:
>> On 09/29/14 06:25, Stefano Stabellini wrote:
>> > On Mon, 29 Sep 2014, Stefano Stabellini wrote:
>> > > On Fri, 26 Sep 2014, Don Slutz wrote:
>> > > > This adds synchronisation of the vcpu registers
>> > > > between Xen and QEMU.
>
> ...
>
>> > > > +            CPUX86State *env;
>> > > > +            ioreq_t fake_req = {
>> > > > +                .type = IOREQ_TYPE_PIO,
>> > > > +                .addr = (uint16_t)req->size,
>> > > > +                .size = 4,
>> > > > +                .dir = IOREQ_READ,
>> > > > +                .df = 0,
>> > > > +                .data_is_ptr = 0,
>> > > > +            };
>> > Why do you need a fake req?
>>
>> To transport the 6 VCPU registers (only 32bits of them) that vmport.c
>> needs to do it's work.
>>
>> > Couldn't Xen send the real req instead?
>>
>> Yes, but then a 2nd exchange between QEMU and Xen would be needed
>> to fetch the 6 VCPU registers.  The ways I know of to fetch the VCPU
>> registers
>> from Xen, all need many cycles to do their work and return
>> a lot of data that is not needed.
>>
>> The other option that I have considered was to extend the ioreq_t type
>> to have room for these registers, but that reduces by almost half the
>> maximum number of VCPUs that are supported (They all live on 1 page).
>
> Urgh. Now that I understand the patch better is think it's horrible, no
> offense :-)
>
> Why don't you add another new ioreq type to send out the vcpu state?
> Something like IOREQ_TYPE_VCPU_SYNC_REGS? You could send it to QEMU
> before IOREQ_TYPE_VMWARE_PORT. Actually this solution looks very imilar
> to Alex's suggestion.
>
> ...
>
>
> And the ASSERTs below are the attempt to prevent bugs from being added.
>
> Sigh.  Too much with both XEN and QEMU in hard freeze.  I may
> have a way to avoid the cast.

I put 2 and 2 together to make something close to 4 after sending the
first email, but unions are the C way of doing things like this.

~Andrew

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5350 bytes --]

On Thu, 2 Oct 2014, Andrew Cooper wrote:
> On 02/10/2014 22:56, Don Slutz wrote:
> > On 10/02/14 16:33, Andrew Cooper wrote:
> >> 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.
> >
> > This is a direct result of:
> >
> >
> > Subject:     Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to
> > vmport
> > Date:     Tue, 30 Sep 2014 11:35:44 +0100
> > From:     Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > To:     Don Slutz <dslutz@verizon.com>
> > CC:     Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
> > qemu-devel@nongnu.org, xen-devel@lists.xensource.com, Alexander Graf
> > <agraf@suse.de>, Andreas Färber <afaerber@suse.de>, Anthony Liguori
> > <aliguori@amazon.com>, Marcel Apfelbaum <marcel.a@redhat.com>, Markus
> > Armbruster <armbru@redhat.com>, Michael S. Tsirkin <mst@redhat.com>
> >
> >
> >
> > On Mon, 29 Sep 2014, Don Slutz wrote:
> >> On 09/29/14 06:25, Stefano Stabellini wrote:
> >> > On Mon, 29 Sep 2014, Stefano Stabellini wrote:
> >> > > On Fri, 26 Sep 2014, Don Slutz wrote:
> >> > > > This adds synchronisation of the vcpu registers
> >> > > > between Xen and QEMU.
> >
> > ...
> >
> >> > > > +            CPUX86State *env;
> >> > > > +            ioreq_t fake_req = {
> >> > > > +                .type = IOREQ_TYPE_PIO,
> >> > > > +                .addr = (uint16_t)req->size,
> >> > > > +                .size = 4,
> >> > > > +                .dir = IOREQ_READ,
> >> > > > +                .df = 0,
> >> > > > +                .data_is_ptr = 0,
> >> > > > +            };
> >> > Why do you need a fake req?
> >>
> >> To transport the 6 VCPU registers (only 32bits of them) that vmport.c
> >> needs to do it's work.
> >>
> >> > Couldn't Xen send the real req instead?
> >>
> >> Yes, but then a 2nd exchange between QEMU and Xen would be needed
> >> to fetch the 6 VCPU registers.  The ways I know of to fetch the VCPU
> >> registers
> >> from Xen, all need many cycles to do their work and return
> >> a lot of data that is not needed.
> >>
> >> The other option that I have considered was to extend the ioreq_t type
> >> to have room for these registers, but that reduces by almost half the
> >> maximum number of VCPUs that are supported (They all live on 1 page).
> >
> > Urgh. Now that I understand the patch better is think it's horrible, no
> > offense :-)
> >
> > Why don't you add another new ioreq type to send out the vcpu state?
> > Something like IOREQ_TYPE_VCPU_SYNC_REGS? You could send it to QEMU
> > before IOREQ_TYPE_VMWARE_PORT. Actually this solution looks very imilar
> > to Alex's suggestion.
> >
> > ...
> >
> >
> > And the ASSERTs below are the attempt to prevent bugs from being added.
> >
> > Sigh.  Too much with both XEN and QEMU in hard freeze.  I may
> > have a way to avoid the cast.
> 
> I put 2 and 2 together to make something close to 4 after sending the
> first email, but unions are the C way of doing things like this.

I think that vmware_ioreq_t is still far from ideal but it is an
improvement over the previous version that was silently abusing the
ioreq_t struct. This version at least does that explicitly.

The issue with a union is compatibility with older QEMU versions: we can
introduce the union and retain compatibility only if we use anonymous
unions.  However I seem to recall Jan arguing against anonymous unions
in public interfaces in past.

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

On Fri, 2014-10-03 at 10:47 +0100, Stefano Stabellini wrote:
> The issue with a union is compatibility with older QEMU versions: we can
> introduce the union and retain compatibility only if we use anonymous
> unions.  However I seem to recall Jan arguing against anonymous unions
> in public interfaces in past.

The canonical headers in xen/include/public are supposed to be strict
ANSI C and anonymous unions are a gcc extension.

However no-one is obliged to use this copy and several projects
(including Linux, *BSD and others) take copies and modify them to suite
their local coding styles/conventions etc. That could include using
anonymous unions if that is preferable. I'm not sure if that helps you
here though (since the issue AIUI is with existing qemu releases...)

Ian.

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

[Stefano Stabellini]

On Fri, 3 Oct 2014, Ian Campbell wrote:
> On Fri, 2014-10-03 at 10:47 +0100, Stefano Stabellini wrote:
> > The issue with a union is compatibility with older QEMU versions: we can
> > introduce the union and retain compatibility only if we use anonymous
> > unions.  However I seem to recall Jan arguing against anonymous unions
> > in public interfaces in past.
> 
> The canonical headers in xen/include/public are supposed to be strict
> ANSI C and anonymous unions are a gcc extension.
> 
> However no-one is obliged to use this copy and several projects
> (including Linux, *BSD and others) take copies and modify them to suite
> their local coding styles/conventions etc. That could include using
> anonymous unions if that is preferable. I'm not sure if that helps you
> here though (since the issue AIUI is with existing qemu releases...)

Right, it doesn't help.
Even upstream QEMU still builds against external Xen header files.

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

[Don Slutz]

On 10/03/14 05:54, Stefano Stabellini wrote:
> On Fri, 3 Oct 2014, Ian Campbell wrote:
>> On Fri, 2014-10-03 at 10:47 +0100, Stefano Stabellini wrote:
>>> The issue with a union is compatibility with older QEMU versions: we can
>>> introduce the union and retain compatibility only if we use anonymous
>>> unions.  However I seem to recall Jan arguing against anonymous unions
>>> in public interfaces in past.
>> The canonical headers in xen/include/public are supposed to be strict
>> ANSI C and anonymous unions are a gcc extension.
>>
>> However no-one is obliged to use this copy and several projects
>> (including Linux, *BSD and others) take copies and modify them to suite
>> their local coding styles/conventions etc. That could include using
>> anonymous unions if that is preferable. I'm not sure if that helps you
>> here though (since the issue AIUI is with existing qemu releases...)
> Right, it doesn't help.
> Even upstream QEMU still builds against external Xen header files.

Here is a union that as far as I know is ANSI C and avoids a cast to a 
different
type.  But it is a lot of changes just to avoid this.

Should I spend the time to make it part of this?

 From 84b4f8eb944d436b4e47e4024ebefbbe4460cd32 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Fri, 3 Oct 2014 15:15:11 -0400
Subject: [PATCH] Add union_ioreq_t

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
  xen/arch/x86/hvm/emulate.c     | 90 
+++++++++++++++++++++---------------------
  xen/arch/x86/hvm/hvm.c         | 73 +++++++++++++++++-----------------
  xen/arch/x86/hvm/io.c          | 27 ++++++-------
  xen/include/asm-x86/hvm/hvm.h  |  2 +-
  xen/include/asm-x86/hvm/io.h   |  2 +-
  xen/include/public/hvm/ioreq.h | 11 ++++++
  6 files changed, 109 insertions(+), 96 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 5f8c551..cee8a37 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -55,20 +55,18 @@ int hvmemul_do_vmport(uint16_t addr, int size, int dir,
  {
      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,
+    union_ioreq_t up = {
+        .vreq.type = IOREQ_TYPE_VMWARE_PORT,
+        .vreq.addr = addr,
+        .vreq.size = size,
+        .vreq.dir = dir,
+        .vreq.eax = regs->rax,
+        .vreq.ebx = regs->rbx,
+        .vreq.ecx = regs->rcx,
+        .vreq.edx = regs->rdx,
+        .vreq.esi = regs->rsi,
+        .vreq.edi = regs->rdi,
      };
-    ioreq_t *pp = (ioreq_t *)&p;
-    ioreq_t op;

      BUILD_BUG_ON(sizeof(ioreq_t) != sizeof(vmware_ioreq_t));
      BUILD_BUG_ON(offsetof(ioreq_t, type) != offsetof(vmware_ioreq_t, 
type));
@@ -104,21 +102,25 @@ int hvmemul_do_vmport(uint16_t addr, int size, int 
dir,
      }
      else
      {
-        if ( !hvm_send_assist_req(pp) )
+        if ( !hvm_send_assist_req(&up) )
              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;
-    hvmtrace_io_assist(0, &op);
+    {
+        ioreq_t op = {
+            .type = IOREQ_TYPE_PIO,
+            .dir = dir,
+            .addr = addr,
+            .data_is_ptr = 0,
+            .data = vio->io_data,
+        };
+
+        hvmtrace_io_assist(0, &op);
+    }

      memcpy(&regs->rax, &vio->io_data, vio->io_size);
-
      return X86EMUL_OKAY;
  }

@@ -128,14 +130,14 @@ static int hvmemul_do_io(
  {
      struct vcpu *curr = current;
      struct hvm_vcpu_io *vio;
-    ioreq_t p = {
-        .type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO,
-        .addr = addr,
-        .size = size,
-        .dir = dir,
-        .df = df,
-        .data = ram_gpa,
-        .data_is_ptr = (p_data == NULL),
+    union_ioreq_t up = {
+        .oreq.type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO,
+        .oreq.addr = addr,
+        .oreq.size = size,
+        .oreq.dir = dir,
+        .oreq.df = df,
+        .oreq.data = ram_gpa,
+        .oreq.data_is_ptr = (p_data == NULL),
      };
      unsigned long ram_gfn = paddr_to_pfn(ram_gpa);
      p2m_type_t p2mt;
@@ -173,15 +175,15 @@ static int hvmemul_do_io(
          return X86EMUL_UNHANDLEABLE;
      }

-    if ( !p.data_is_ptr && (dir == IOREQ_WRITE) )
+    if ( !up.oreq.data_is_ptr && (dir == IOREQ_WRITE) )
      {
-        memcpy(&p.data, p_data, size);
+        memcpy(&up.oreq.data, p_data, size);
          p_data = NULL;
      }

      vio = &curr->arch.hvm_vcpu.hvm_io;

-    if ( is_mmio && !p.data_is_ptr )
+    if ( is_mmio && !up.oreq.data_is_ptr )
      {
          /* Part of a multi-cycle read or write? */
          if ( dir == IOREQ_WRITE )
@@ -225,7 +227,7 @@ static int hvmemul_do_io(
          goto finish_access;
      case HVMIO_dispatched:
          /* May have to wait for previous cycle of a multi-write to 
complete. */
-        if ( is_mmio && !p.data_is_ptr && (dir == IOREQ_WRITE) &&
+        if ( is_mmio && !up.oreq.data_is_ptr && (dir == IOREQ_WRITE) &&
               (addr == (vio->mmio_large_write_pa +
                         vio->mmio_large_write_bytes)) )
          {
@@ -258,31 +260,31 @@ static int hvmemul_do_io(
      if ( vio->mmio_retrying )
          *reps = 1;

-    p.count = *reps;
+    up.oreq.count = *reps;

      if ( dir == IOREQ_WRITE )
-        hvmtrace_io_assist(is_mmio, &p);
+        hvmtrace_io_assist(is_mmio, &up.oreq);

      if ( is_mmio )
      {
-        rc = hvm_mmio_intercept(&p);
+        rc = hvm_mmio_intercept(&up.oreq);
          if ( rc == X86EMUL_UNHANDLEABLE )
-            rc = hvm_buffered_io_intercept(&p);
+            rc = hvm_buffered_io_intercept(&up.oreq);
      }
      else
      {
-        rc = hvm_portio_intercept(&p);
+        rc = hvm_portio_intercept(&up.oreq);
      }

      switch ( rc )
      {
      case X86EMUL_OKAY:
      case X86EMUL_RETRY:
-        *reps = p.count;
-        p.state = STATE_IORESP_READY;
+        *reps = up.oreq.count;
+        up.oreq.state = STATE_IORESP_READY;
          if ( !vio->mmio_retry )
          {
-            hvm_io_assist(&p);
+            hvm_io_assist(&up);
              vio->io_state = HVMIO_none;
          }
          else
@@ -299,7 +301,7 @@ static int hvmemul_do_io(
          else
          {
              rc = X86EMUL_RETRY;
-            if ( !hvm_send_assist_req(&p) )
+            if ( !hvm_send_assist_req(&up) )
                  vio->io_state = HVMIO_none;
              else if ( p_data == NULL )
                  rc = X86EMUL_OKAY;
@@ -318,12 +320,12 @@ static int hvmemul_do_io(

   finish_access:
      if ( dir == IOREQ_READ )
-        hvmtrace_io_assist(is_mmio, &p);
+        hvmtrace_io_assist(is_mmio, &up.oreq);

      if ( p_data != NULL )
          memcpy(p_data, &vio->io_data, size);

-    if ( is_mmio && !p.data_is_ptr )
+    if ( is_mmio && !up.oreq.data_is_ptr )
      {
          /* Part of a multi-cycle read or write? */
          if ( dir == IOREQ_WRITE )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 83a7000..d1f3e52 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -372,9 +372,9 @@ void hvm_migrate_pirqs(struct vcpu *v)
      spin_unlock(&d->event_lock);
  }

-static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
+static union_ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
  {
-    shared_iopage_t *p = s->ioreq.va;
+    union_shared_iopage_t *p = s->ioreq.va;

      ASSERT((v == current) || !vcpu_runnable(v));
      ASSERT(p != NULL);
@@ -391,34 +391,35 @@ bool_t hvm_io_pending(struct vcpu *v)
&d->arch.hvm_domain.ioreq_server.list,
                            list_entry )
      {
-        ioreq_t *p = get_ioreq(s, v);
+        union_ioreq_t *up = get_ioreq(s, v);

-        if ( p->state != STATE_IOREQ_NONE )
+        if ( up->oreq.state != STATE_IOREQ_NONE )
              return 1;
      }

      return 0;
  }

-static bool_t hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
+static bool_t hvm_wait_for_io(struct hvm_ioreq_vcpu *sv,
+                              union_ioreq_t *up)
  {
-    /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
-    while ( p->state != STATE_IOREQ_NONE )
+    /* NB. Optimised for common case (up->oreq.state == 
STATE_IOREQ_NONE). */
+    while ( up->oreq.state != STATE_IOREQ_NONE )
      {
-        switch ( p->state )
+        switch ( up->oreq.state )
          {
          case STATE_IORESP_READY: /* IORESP_READY -> NONE */
              rmb(); /* see IORESP_READY /then/ read contents of ioreq */
-            hvm_io_assist(p);
+            hvm_io_assist(up);
              break;
          case STATE_IOREQ_READY:  /* IOREQ_{READY,INPROCESS} -> 
IORESP_READY */
          case STATE_IOREQ_INPROCESS:
              wait_on_xen_event_channel(sv->ioreq_evtchn,
-                                      (p->state != STATE_IOREQ_READY) &&
-                                      (p->state != STATE_IOREQ_INPROCESS));
+                                      (up->oreq.state != 
STATE_IOREQ_READY) &&
+                                      (up->oreq.state != 
STATE_IOREQ_INPROCESS));
              break;
          default:
-            gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", 
p->state);
+            gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", 
up->oreq.state);
              domain_crash(sv->vcpu->domain);
              return 0; /* bail */
          }
@@ -598,9 +599,9 @@ static void hvm_update_ioreq_evtchn(struct 
hvm_ioreq_server *s,

      if ( s->ioreq.va != NULL )
      {
-        ioreq_t *p = get_ioreq(s, sv->vcpu);
+        union_ioreq_t *up = get_ioreq(s, sv->vcpu);

-        p->vp_eport = sv->ioreq_evtchn;
+        up->oreq.vp_eport = sv->ioreq_evtchn;
      }
  }

@@ -2526,27 +2527,27 @@ bool_t 
hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
          if ( sv->vcpu == curr )
          {
              evtchn_port_t port = sv->ioreq_evtchn;
-            ioreq_t *p = get_ioreq(s, curr);
+            union_ioreq_t *up = get_ioreq(s, curr);

-            if ( unlikely(p->state != STATE_IOREQ_NONE) )
+            if ( unlikely(up->oreq.state != STATE_IOREQ_NONE) )
              {
                  gdprintk(XENLOG_ERR,
                           "Device model set bad IO state %d.\n",
-                         p->state);
+                         up->oreq.state);
                  goto crash;
              }

-            if ( unlikely(p->vp_eport != port) )
+            if ( unlikely(up->oreq.vp_eport != port) )
              {
                  gdprintk(XENLOG_ERR,
                           "Device model set bad event channel %d.\n",
-                         p->vp_eport);
+                         up->oreq.vp_eport);
                  goto crash;
              }

              proto_p->state = STATE_IOREQ_NONE;
              proto_p->vp_eport = port;
-            *p = *proto_p;
+            up->oreq = *proto_p;

              prepare_wait_on_xen_event_channel(port);

@@ -2555,7 +2556,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct 
hvm_ioreq_server *s,
               * contents. prepare_wait_on_xen_event_channel() is an 
implicit
               * barrier.
               */
-            p->state = STATE_IOREQ_READY;
+            up->oreq.state = STATE_IOREQ_READY;
              notify_via_xen_event_channel(d, port);
              break;
          }
@@ -2568,44 +2569,44 @@ bool_t 
hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
      return 0;
  }

-static bool_t hvm_complete_assist_req(ioreq_t *p)
+static bool_t hvm_complete_assist_req(union_ioreq_t *up)
  {
-    switch ( p->type )
+    switch ( up->oreq.type )
      {
      case IOREQ_TYPE_COPY:
      case IOREQ_TYPE_PIO:
-        if ( p->dir == IOREQ_READ )
+        if ( up->oreq.dir == IOREQ_READ )
          {
-            if ( !p->data_is_ptr )
-                p->data = ~0ul;
+            if ( !up->oreq.data_is_ptr )
+                up->oreq.data = ~0ul;
              else
              {
-                int i, step = p->df ? -p->size : p->size;
+                int i, step = up->oreq.df ? -up->oreq.size : up->oreq.size;
                  uint32_t data = ~0;

-                for ( i = 0; i < p->count; i++ )
-                    hvm_copy_to_guest_phys(p->data + step * i, &data,
-                                           p->size);
+                for ( i = 0; i < up->oreq.count; i++ )
+                    hvm_copy_to_guest_phys(up->oreq.data + step * i, &data,
+                                           up->oreq.size);
              }
          }
          /* FALLTHRU */
      default:
-        p->state = STATE_IORESP_READY;
-        hvm_io_assist(p);
+        up->oreq.state = STATE_IORESP_READY;
+        hvm_io_assist(up);
          break;
      }

      return 1;
  }

-bool_t hvm_send_assist_req(ioreq_t *p)
+bool_t hvm_send_assist_req(union_ioreq_t *up)
  {
-    struct hvm_ioreq_server *s = 
hvm_select_ioreq_server(current->domain, p);
+    struct hvm_ioreq_server *s = 
hvm_select_ioreq_server(current->domain, &up->oreq);

      if ( !s )
-        return hvm_complete_assist_req(p);
+        return hvm_complete_assist_req(up);

-    return hvm_send_assist_req_to_ioreq_server(s, p);
+    return hvm_send_assist_req_to_ioreq_server(s, &up->oreq);
  }

  void hvm_broadcast_assist_req(ioreq_t *p)
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 5d7d221..a06a507 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -169,13 +169,13 @@ int handle_pio(uint16_t port, unsigned int size, 
int dir)
      return 1;
  }

-void hvm_io_assist(ioreq_t *p)
+void hvm_io_assist(union_ioreq_t *up)
  {
      struct vcpu *curr = current;
      struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
      enum hvm_io_state io_state;

-    p->state = STATE_IOREQ_NONE;
+    up->oreq.state = STATE_IOREQ_NONE;

      io_state = vio->io_state;
      vio->io_state = HVMIO_none;
@@ -184,39 +184,38 @@ void hvm_io_assist(ioreq_t *p)
      {
      case HVMIO_awaiting_completion:
          vio->io_state = HVMIO_completed;
-        vio->io_data = p->data;
+        vio->io_data = up->oreq.data;
          break;
      case HVMIO_handle_mmio_awaiting_completion:
          vio->io_state = HVMIO_completed;
-        vio->io_data = p->data;
+        vio->io_data = up->oreq.data;
          (void)handle_mmio();
          break;
      case HVMIO_handle_pio_awaiting_completion:
          if ( vio->io_size == 4 ) /* Needs zero extension. */
-            guest_cpu_user_regs()->rax = (uint32_t)p->data;
+            guest_cpu_user_regs()->rax = (uint32_t)up->oreq.data;
          else
-            memcpy(&guest_cpu_user_regs()->rax, &p->data, vio->io_size);
+            memcpy(&guest_cpu_user_regs()->rax, &up->oreq.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;

          /* Always zero extension for eax */
-        regs->rax = vp->eax;
+        regs->rax = up->vreq.eax;
          /* Only change the 32bit part of the register */
-        regs->_ebx = vp->ebx;
-        regs->_ecx = vp->ecx;
-        regs->_edx = vp->edx;
-        regs->_esi = vp->esi;
-        regs->_edi = vp->edi;
+        regs->_ebx = up->vreq.ebx;
+        regs->_ecx = up->vreq.ecx;
+        regs->_edx = up->vreq.edx;
+        regs->_esi = up->vreq.esi;
+        regs->_edi = up->vreq.edi;
      }
          break;
      default:
          break;
      }

-    if ( p->state == STATE_IOREQ_NONE )
+    if ( up->oreq.state == STATE_IOREQ_NONE )
      {
          msix_write_completion(curr);
          vcpu_end_shutdown_deferral(curr);
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 0910147..844271c 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v);
  void hvm_vcpu_cacheattr_destroy(struct vcpu *v);
  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip);

-bool_t hvm_send_assist_req(ioreq_t *p);
+bool_t hvm_send_assist_req(union_ioreq_t *up);
  void hvm_broadcast_assist_req(ioreq_t *p);

  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index d257161..1a183a1 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -123,7 +123,7 @@ int handle_mmio_with_translation(unsigned long gva, 
unsigned long gpfn,
                                   struct npfec);
  int handle_pio(uint16_t port, unsigned int size, int dir);
  void hvm_interrupt_post(struct vcpu *v, int vector, int type);
-void hvm_io_assist(ioreq_t *p);
+void hvm_io_assist(union_ioreq_t *p);
  void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
                    const union vioapic_redir_entry *ent);
  void msix_write_completion(struct vcpu *);
diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
index 7d5ba58..d75ce66 100644
--- a/xen/include/public/hvm/ioreq.h
+++ b/xen/include/public/hvm/ioreq.h
@@ -85,11 +85,22 @@ struct vmware_ioreq {
  };
  typedef struct vmware_ioreq vmware_ioreq_t;

+union union_ioreq {
+    ioreq_t oreq;
+    vmware_ioreq_t vreq;
+};
+typedef union union_ioreq union_ioreq_t;
+
  struct shared_iopage {
      struct ioreq vcpu_ioreq[1];
  };
  typedef struct shared_iopage shared_iopage_t;

+struct union_shared_iopage {
+    union union_ioreq vcpu_ioreq[1];
+};
+typedef struct union_shared_iopage union_shared_iopage_t;
+
  struct buf_ioreq {
      uint8_t  type;   /* I/O type                    */
      uint8_t  pad:1;
-- 
1.7.11.7


     -Don Slutz

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

[Jan Beulich]

>>> On 03.10.14 at 21:27, <dslutz@verizon.com> wrote:
> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -85,11 +85,22 @@ struct vmware_ioreq {
>   };
>   typedef struct vmware_ioreq vmware_ioreq_t;
> 
> +union union_ioreq {
> +    ioreq_t oreq;
> +    vmware_ioreq_t vreq;
> +};
> +typedef union union_ioreq union_ioreq_t;
> +
>   struct shared_iopage {
>       struct ioreq vcpu_ioreq[1];
>   };
>   typedef struct shared_iopage shared_iopage_t;
> 
> +struct union_shared_iopage {
> +    union union_ioreq vcpu_ioreq[1];
> +};
> +typedef struct union_shared_iopage union_shared_iopage_t;

I don't think either of these really need to be part of the public
interface. (That said, I'm far from being convinced of this
overloading mechanism, and in fact much of this VMware support
code, in the first place.) 

Jan

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

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 03.10.14 at 21:27, <dslutz@verizon.com> wrote:
> > --- a/xen/include/public/hvm/ioreq.h
> > +++ b/xen/include/public/hvm/ioreq.h
> > @@ -85,11 +85,22 @@ struct vmware_ioreq {
> >   };
> >   typedef struct vmware_ioreq vmware_ioreq_t;
> > 
> > +union union_ioreq {
> > +    ioreq_t oreq;
> > +    vmware_ioreq_t vreq;
> > +};
> > +typedef union union_ioreq union_ioreq_t;
> > +
> >   struct shared_iopage {
> >       struct ioreq vcpu_ioreq[1];
> >   };
> >   typedef struct shared_iopage shared_iopage_t;
> > 
> > +struct union_shared_iopage {
> > +    union union_ioreq vcpu_ioreq[1];
> > +};
> > +typedef struct union_shared_iopage union_shared_iopage_t;
> 
> I don't think either of these really need to be part of the public
> interface. 

Do you prefer this union mechanism or the original code of this patch
1412274977-6098-2-git-send-email-dslutz@verizon.com?

I haven't see the QEMU code for the union approach, but both
alternatives would probably be fine from the QEMU POV.

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

[Jan Beulich]

>>> On 06.10.14 at 11:21, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 03.10.14 at 21:27, <dslutz@verizon.com> wrote:
>> > --- a/xen/include/public/hvm/ioreq.h
>> > +++ b/xen/include/public/hvm/ioreq.h
>> > @@ -85,11 +85,22 @@ struct vmware_ioreq {
>> >   };
>> >   typedef struct vmware_ioreq vmware_ioreq_t;
>> > 
>> > +union union_ioreq {
>> > +    ioreq_t oreq;
>> > +    vmware_ioreq_t vreq;
>> > +};
>> > +typedef union union_ioreq union_ioreq_t;
>> > +
>> >   struct shared_iopage {
>> >       struct ioreq vcpu_ioreq[1];
>> >   };
>> >   typedef struct shared_iopage shared_iopage_t;
>> > 
>> > +struct union_shared_iopage {
>> > +    union union_ioreq vcpu_ioreq[1];
>> > +};
>> > +typedef struct union_shared_iopage union_shared_iopage_t;
>> 
>> I don't think either of these really need to be part of the public
>> interface. 
> 
> Do you prefer this union mechanism or the original code of this patch
> 1412274977-6098-2-git-send-email-dslutz@verizon.com?
> 
> I haven't see the QEMU code for the union approach, but both
> alternatives would probably be fine from the QEMU POV.

I'm fine with the union approach (subject to convincing myself that
all this effort and new code is really worth it in the first place, as
said elsewhere before), just not as part of the public interface
(unless there is a clear need for it to be there).

Jan

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

[Don Slutz]

On 10/06/14 05:39, Jan Beulich wrote:
>>>> On 06.10.14 at 11:21, <stefano.stabellini@eu.citrix.com> wrote:
>> On Mon, 6 Oct 2014, Jan Beulich wrote:
>>>>>> On 03.10.14 at 21:27, <dslutz@verizon.com> wrote:
>>>> --- a/xen/include/public/hvm/ioreq.h
>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>> @@ -85,11 +85,22 @@ struct vmware_ioreq {
>>>>    };
>>>>    typedef struct vmware_ioreq vmware_ioreq_t;
>>>>
>>>> +union union_ioreq {
>>>> +    ioreq_t oreq;
>>>> +    vmware_ioreq_t vreq;
>>>> +};
>>>> +typedef union union_ioreq union_ioreq_t;
>>>> +
>>>>    struct shared_iopage {
>>>>        struct ioreq vcpu_ioreq[1];
>>>>    };
>>>>    typedef struct shared_iopage shared_iopage_t;
>>>>
>>>> +struct union_shared_iopage {
>>>> +    union union_ioreq vcpu_ioreq[1];
>>>> +};
>>>> +typedef struct union_shared_iopage union_shared_iopage_t;
>>> I don't think either of these really need to be part of the public
>>> interface.
>> Do you prefer this union mechanism or the original code of this patch
>> 1412274977-6098-2-git-send-email-dslutz@verizon.com?
>>
>> I haven't see the QEMU code for the union approach, but both
>> alternatives would probably be fine from the QEMU POV.

Adding this union mechanism in QEMU would be making it part
of the public interface.  So I have not yet coded it up.

> I'm fine with the union approach (subject to convincing myself that
> all this effort and new code is really worth it in the first place, as
> said elsewhere before), just not as part of the public interface
> (unless there is a clear need for it to be there).

I am read this as yes, add the union_ioreq_t to Xen but not to QEMU.

I know of at least 1 custom kernel that requires the VMware "rpc" code.
So I think that vmware_port support is just like viridian support. It allows
more guests to be run under Xen.

With the statement "move VMware rpc to QEMU", this patch was needed.

Hope, this helps.

    -Don Slutz

> Jan
>

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

[Jan Beulich]

>>> On 06.10.14 at 21:51, <dslutz@verizon.com> wrote:
> On 10/06/14 05:39, Jan Beulich wrote:
>> I'm fine with the union approach (subject to convincing myself that
>> all this effort and new code is really worth it in the first place, as
>> said elsewhere before), just not as part of the public interface
>> (unless there is a clear need for it to be there).
> 
> I am read this as yes, add the union_ioreq_t to Xen but not to QEMU.

Not exactly: Since the union approach is mechanism, not interface,
both Xen and any consumer of the public interface can choose to
follow this route or use (fragile) casts.

> I know of at least 1 custom kernel that requires the VMware "rpc" code.
> So I think that vmware_port support is just like viridian support. It allows
> more guests to be run under Xen.

Not exactly: You're adding much more code than Viridian support
has accumulated over several years, and quite a bit of that based
on observation rather than a publicly available spec.

Jan

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

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> Sent: 06 October 2014 10:22
> To: Jan Beulich
> Cc: Keir (Xen.org); Ian Campbell; Stefano Stabellini; Andrew Cooper; Don
> Slutz; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [RFC][PATCH v2 1/1] Add
> IOREQ_TYPE_VMWARE_PORT
> 
> On Mon, 6 Oct 2014, Jan Beulich wrote:
> > >>> On 03.10.14 at 21:27, <dslutz@verizon.com> wrote:
> > > --- a/xen/include/public/hvm/ioreq.h
> > > +++ b/xen/include/public/hvm/ioreq.h
> > > @@ -85,11 +85,22 @@ struct vmware_ioreq {
> > >   };
> > >   typedef struct vmware_ioreq vmware_ioreq_t;
> > >
> > > +union union_ioreq {
> > > +    ioreq_t oreq;
> > > +    vmware_ioreq_t vreq;
> > > +};
> > > +typedef union union_ioreq union_ioreq_t;
> > > +
> > >   struct shared_iopage {
> > >       struct ioreq vcpu_ioreq[1];
> > >   };
> > >   typedef struct shared_iopage shared_iopage_t;
> > >
> > > +struct union_shared_iopage {
> > > +    union union_ioreq vcpu_ioreq[1];
> > > +};
> > > +typedef struct union_shared_iopage union_shared_iopage_t;
> >
> > I don't think either of these really need to be part of the public
> > interface.
> 
> Do you prefer this union mechanism or the original code of this patch
> 1412274977-6098-2-git-send-email-dslutz@verizon.com?
> 
> I haven't see the QEMU code for the union approach, but both
> alternatives would probably be fine from the QEMU POV.
> 

Really? To me both seem equally horrible and unnecessary.

  Paul

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

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

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Don Slutz
> Sent: 02 October 2014 19:36
> To: xen-devel@lists.xen.org
> Cc: Don Slutz; Keir (Xen.org); Ian Campbell; Jan Beulich
> Subject: [Xen-devel] [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT
> 
> 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;
> +
> +    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));

Can we not avoid this overloading of the ioreq structure by having the emulator directly modify the vCPU registers? Since the vCPU is paused for emulation, could it not just do a get context/set context to tweak the values?

  Paul

> +
> +    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;
> +    hvmtrace_io_assist(0, &op);
> +
> +    memcpy(&regs->rax, &vio->io_data, vio->io_size);
> +
> +    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->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];
>  };
> --
> 1.8.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Don Slutz]

On 10/03/14 05:29, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Don Slutz
>> Sent: 02 October 2014 19:36
>> To: xen-devel@lists.xen.org
>> Cc: Don Slutz; Keir (Xen.org); Ian Campbell; Jan Beulich
>> Subject: [Xen-devel] [RFC][PATCH v2 1/1] Add IOREQ_TYPE_VMWARE_PORT
>>
>> 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
>> +++++++++++++++++++++++++++++++++++++++

...

>> +
>> +    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));
> Can we not avoid this overloading of the ioreq structure by having the emulator directly modify the vCPU registers?

Yes we can at a high cost of cpu overhead.  The current ways of accessing
registers are mostly way too many registers and other side effects. Using
the debugger interface (which I do not know as well) has a high cost.


> Since the vCPU is paused for emulation, could it not just do a get context/set context to tweak the values?

It is blocked not paused, and while I have not tried it, I would expect 
it to work.
However it does require switching from qemu to the hypervisor and back 2 
times
which is not free.

So I feel that adding a lot of overhead to avoid a new type ioreq_t is 
the wrong
way to go.

    -Don Slutz

>    Paul
>
>

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

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant
> Sent: 03 October 2014 10:29
> To: 'Don Slutz'; xen-devel@lists.xen.org
> Cc: Keir (Xen.org); Ian Campbell; Jan Beulich
> Subject: RE: [Xen-devel] [RFC][PATCH v2 1/1] Add
> IOREQ_TYPE_VMWARE_PORT
> 
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > bounces@lists.xen.org] On Behalf Of Don Slutz
> > Sent: 02 October 2014 19:36
> > To: xen-devel@lists.xen.org
> > Cc: Don Slutz; Keir (Xen.org); Ian Campbell; Jan Beulich
> > Subject: [Xen-devel] [RFC][PATCH v2 1/1] Add
> IOREQ_TYPE_VMWARE_PORT
> >
> > 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;
> > +
> > +    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));
> 
> Can we not avoid this overloading of the ioreq structure by having the
> emulator directly modify the vCPU registers? Since the vCPU is paused for
> emulation, could it not just do a get context/set context to tweak the values?
> 

I should have added...

Or if that doesn't work then surely an extra page of domheap, which can be mapped by the emulator to provide a dedicated channel, is preferable to this mechanism.

>   Paul
> 
> > +
> > +    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;
> > +    hvmtrace_io_assist(0, &op);
> > +
> > +    memcpy(&regs->rax, &vio->io_data, vio->io_size);
> > +
> > +    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->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];
> >  };
> > --
> > 1.8.4
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

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

[Don Slutz]

On 10/03/14 05:46, Paul Durrant wrote:
>> -----Original Message-----
>> From: Paul Durrant
>> Sent: 03 October 2014 10:29
>> To: 'Don Slutz'; xen-devel@lists.xen.org
>> Cc: Keir (Xen.org); Ian Campbell; Jan Beulich
>> Subject: RE: [Xen-devel] [RFC][PATCH v2 1/1] Add
>> IOREQ_TYPE_VMWARE_PORT
>>
>>> -----Original Message-----
>>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>>> bounces@lists.xen.org] On Behalf Of Don Slutz
>>> Sent: 02 October 2014 19:36
>>> To: xen-devel@lists.xen.org
>>> Cc: Don Slutz; Keir (Xen.org); Ian Campbell; Jan Beulich
>>> Subject: [Xen-devel] [RFC][PATCH v2 1/1] Add
>> IOREQ_TYPE_VMWARE_PORT
>>
...
>> Can we not avoid this overloading of the ioreq structure by having the
>> emulator directly modify the vCPU registers? Since the vCPU is paused for
>> emulation, could it not just do a get context/set context to tweak the values?
>>
> I should have added...
>
> Or if that doesn't work then surely an extra page of domheap, which can be mapped by the emulator to provide a dedicated channel, is preferable to this mechanism.

So for a 1 cpu guest adding a page (4096 bytes) to move 24 bytes of data 
is better
for this?

I would still add a new type.  And I would need a slot per cpu just like 
shared_iopage_t
of the same size or smaller (so it can stay 1 page).

I can prototype this out and see how big a change it would be.  It does 
have an impact
on QEMU so cc: qemu-devel


    -Don Slutz


>>    Paul
>>
>>