[Qemu-devel] [PATCH 0/1] Add support for Xen access to vmport

[Qemu-devel] [PATCH 0/1] Add support for Xen access to vmport

[Don Slutz]

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

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

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.

And the Xen RFC patch:

[RFC][PATCH 1/1] Add IOREQ_TYPE_VMWARE_PORT

needs to be done to Xen.

Don Slutz (1):
  xen-hvm.c: Add support for Xen access to vmport

 hw/misc/vmport.c     | 32 ++++++++++++++++++++------------
 include/hw/xen/xen.h |  6 ++++++
 vl.c                 |  1 +
 xen-hvm.c            | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 63 insertions(+), 14 deletions(-)

-- 
1.8.4

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

[Don Slutz]

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

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

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.

And the Xen RFC patch:

[RFC][PATCH 1/1] Add IOREQ_TYPE_VMWARE_PORT

needs to be done to Xen.

Don Slutz (1):
  xen-hvm.c: Add support for Xen access to vmport

 hw/misc/vmport.c     | 32 ++++++++++++++++++++------------
 include/hw/xen/xen.h |  6 ++++++
 vl.c                 |  1 +
 xen-hvm.c            | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 63 insertions(+), 14 deletions(-)

-- 
1.8.4

[Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

This adds synchronisation of the vcpu registers
between Xen and QEMU.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/misc/vmport.c     | 32 ++++++++++++++++++++------------
 include/hw/xen/xen.h |  6 ++++++
 vl.c                 |  1 +
 xen-hvm.c            | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index cd5716a..f984b51 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -26,6 +26,7 @@
 #include "hw/i386/pc.h"
 #include "sysemu/kvm.h"
 #include "hw/qdev.h"
+#include "hw/xen/xen.h"
 
 //#define VMPORT_DEBUG
 
@@ -49,6 +50,16 @@ typedef struct VMPortState
 
 static VMPortState *port_state;
 
+static CPUX86State *vmport_get_env(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    if (xen_enabled()) {
+        return xen_vmport_env();
+    }
+    return &cpu->env;
+}
+
 void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
 {
     if (command >= VMPORT_ENTRIES)
@@ -63,8 +74,7 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
 {
     VMPortState *s = opaque;
     CPUState *cs = current_cpu;
-    X86CPU *cpu = X86_CPU(cs);
-    CPUX86State *env = &cpu->env;
+    CPUX86State *env = vmport_get_env(cs);
     unsigned char command;
     uint32_t eax;
 
@@ -91,32 +101,31 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
 static void vmport_ioport_write(void *opaque, hwaddr addr,
                                 uint64_t val, unsigned size)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = vmport_get_env(current_cpu);
 
-    cpu->env.regs[R_EAX] = vmport_ioport_read(opaque, addr, 4);
+    env->regs[R_EAX] = vmport_ioport_read(opaque, addr, 4);
 }
 
 static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = vmport_get_env(current_cpu);
 
-    cpu->env.regs[R_EBX] = VMPORT_MAGIC;
+    env->regs[R_EBX] = VMPORT_MAGIC;
     return 6;
 }
 
 static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = vmport_get_env(current_cpu);
 
-    cpu->env.regs[R_EBX] = 0x1177;
+    env->regs[R_EBX] = 0x1177;
     return ram_size;
 }
 
 /* vmmouse helpers */
 void vmmouse_get_data(uint32_t *data)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
-    CPUX86State *env = &cpu->env;
+    CPUX86State *env = vmport_get_env(current_cpu);
 
     data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
     data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
@@ -125,8 +134,7 @@ void vmmouse_get_data(uint32_t *data)
 
 void vmmouse_set_data(const uint32_t *data)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
-    CPUX86State *env = &cpu->env;
+    CPUX86State *env = vmport_get_env(current_cpu);
 
     env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
     env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index f71f2d8..8ea328c 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -22,12 +22,18 @@ extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
 
 extern bool xen_allowed;
+extern void *xen_opaque_env;
 
 static inline bool xen_enabled(void)
 {
     return xen_allowed;
 }
 
+static inline void *xen_vmport_env(void)
+{
+    return xen_opaque_env;
+}
+
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
diff --git a/vl.c b/vl.c
index dbdca59..443a9f5 100644
--- a/vl.c
+++ b/vl.c
@@ -215,6 +215,7 @@ static NotifierList machine_init_done_notifiers =
 static bool tcg_allowed = true;
 bool xen_allowed;
 uint32_t xen_domid;
+void *xen_opaque_env;
 enum xen_mode xen_mode = XEN_EMULATE;
 static int tcg_tb_size;
 
diff --git a/xen-hvm.c b/xen-hvm.c
index 05e522c..e1274bb 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
 
     handle_buffered_iopage(state);
     if (req) {
+#ifdef IOREQ_TYPE_VMWARE_PORT
+        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
+            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,
+            };
+            if (!xen_opaque_env) {
+                xen_opaque_env = g_malloc(sizeof(CPUX86State));
+            }
+            env = xen_opaque_env;
+            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
+            env->regs[R_EBX] = (uint32_t)(req->addr);
+            env->regs[R_ECX] = req->count;
+            env->regs[R_EDX] = req->size;
+            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
+            env->regs[R_EDI] = (uint32_t)(req->data);
+            handle_ioreq(&fake_req);
+            req->addr = ((uint64_t)fake_req.data << 32) |
+                (uint32_t)env->regs[R_EBX];
+            req->count = env->regs[R_ECX];
+            req->size = env->regs[R_EDX];
+            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
+                (uint32_t)env->regs[R_EDI];
+        } else {
+            handle_ioreq(req);
+        }
+#else
         handle_ioreq(req);
+#endif
 
         if (req->state != STATE_IOREQ_INPROCESS) {
             fprintf(stderr, "Badness in I/O request ... not in service?!: "
                     "%x, ptr: %x, port: %"PRIx64", "
-                    "data: %"PRIx64", count: %" FMT_ioreq_size ", size: %" FMT_ioreq_size "\n",
+                    "data: %"PRIx64", count: %" FMT_ioreq_size
+                    ", size: %" FMT_ioreq_size ", type: %"FMT_ioreq_size"\n",
                     req->state, req->data_is_ptr, req->addr,
-                    req->data, req->count, req->size);
+                    req->data, req->count, req->size, req->type);
             destroy_hvm_domain(false);
             return;
         }
-- 
1.8.4

[PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

This adds synchronisation of the vcpu registers
between Xen and QEMU.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/misc/vmport.c     | 32 ++++++++++++++++++++------------
 include/hw/xen/xen.h |  6 ++++++
 vl.c                 |  1 +
 xen-hvm.c            | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index cd5716a..f984b51 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -26,6 +26,7 @@
 #include "hw/i386/pc.h"
 #include "sysemu/kvm.h"
 #include "hw/qdev.h"
+#include "hw/xen/xen.h"
 
 //#define VMPORT_DEBUG
 
@@ -49,6 +50,16 @@ typedef struct VMPortState
 
 static VMPortState *port_state;
 
+static CPUX86State *vmport_get_env(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    if (xen_enabled()) {
+        return xen_vmport_env();
+    }
+    return &cpu->env;
+}
+
 void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
 {
     if (command >= VMPORT_ENTRIES)
@@ -63,8 +74,7 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
 {
     VMPortState *s = opaque;
     CPUState *cs = current_cpu;
-    X86CPU *cpu = X86_CPU(cs);
-    CPUX86State *env = &cpu->env;
+    CPUX86State *env = vmport_get_env(cs);
     unsigned char command;
     uint32_t eax;
 
@@ -91,32 +101,31 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
 static void vmport_ioport_write(void *opaque, hwaddr addr,
                                 uint64_t val, unsigned size)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = vmport_get_env(current_cpu);
 
-    cpu->env.regs[R_EAX] = vmport_ioport_read(opaque, addr, 4);
+    env->regs[R_EAX] = vmport_ioport_read(opaque, addr, 4);
 }
 
 static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = vmport_get_env(current_cpu);
 
-    cpu->env.regs[R_EBX] = VMPORT_MAGIC;
+    env->regs[R_EBX] = VMPORT_MAGIC;
     return 6;
 }
 
 static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
+    CPUX86State *env = vmport_get_env(current_cpu);
 
-    cpu->env.regs[R_EBX] = 0x1177;
+    env->regs[R_EBX] = 0x1177;
     return ram_size;
 }
 
 /* vmmouse helpers */
 void vmmouse_get_data(uint32_t *data)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
-    CPUX86State *env = &cpu->env;
+    CPUX86State *env = vmport_get_env(current_cpu);
 
     data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
     data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
@@ -125,8 +134,7 @@ void vmmouse_get_data(uint32_t *data)
 
 void vmmouse_set_data(const uint32_t *data)
 {
-    X86CPU *cpu = X86_CPU(current_cpu);
-    CPUX86State *env = &cpu->env;
+    CPUX86State *env = vmport_get_env(current_cpu);
 
     env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
     env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index f71f2d8..8ea328c 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -22,12 +22,18 @@ extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
 
 extern bool xen_allowed;
+extern void *xen_opaque_env;
 
 static inline bool xen_enabled(void)
 {
     return xen_allowed;
 }
 
+static inline void *xen_vmport_env(void)
+{
+    return xen_opaque_env;
+}
+
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
diff --git a/vl.c b/vl.c
index dbdca59..443a9f5 100644
--- a/vl.c
+++ b/vl.c
@@ -215,6 +215,7 @@ static NotifierList machine_init_done_notifiers =
 static bool tcg_allowed = true;
 bool xen_allowed;
 uint32_t xen_domid;
+void *xen_opaque_env;
 enum xen_mode xen_mode = XEN_EMULATE;
 static int tcg_tb_size;
 
diff --git a/xen-hvm.c b/xen-hvm.c
index 05e522c..e1274bb 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
 
     handle_buffered_iopage(state);
     if (req) {
+#ifdef IOREQ_TYPE_VMWARE_PORT
+        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
+            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,
+            };
+            if (!xen_opaque_env) {
+                xen_opaque_env = g_malloc(sizeof(CPUX86State));
+            }
+            env = xen_opaque_env;
+            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
+            env->regs[R_EBX] = (uint32_t)(req->addr);
+            env->regs[R_ECX] = req->count;
+            env->regs[R_EDX] = req->size;
+            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
+            env->regs[R_EDI] = (uint32_t)(req->data);
+            handle_ioreq(&fake_req);
+            req->addr = ((uint64_t)fake_req.data << 32) |
+                (uint32_t)env->regs[R_EBX];
+            req->count = env->regs[R_ECX];
+            req->size = env->regs[R_EDX];
+            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
+                (uint32_t)env->regs[R_EDI];
+        } else {
+            handle_ioreq(req);
+        }
+#else
         handle_ioreq(req);
+#endif
 
         if (req->state != STATE_IOREQ_INPROCESS) {
             fprintf(stderr, "Badness in I/O request ... not in service?!: "
                     "%x, ptr: %x, port: %"PRIx64", "
-                    "data: %"PRIx64", count: %" FMT_ioreq_size ", size: %" FMT_ioreq_size "\n",
+                    "data: %"PRIx64", count: %" FMT_ioreq_size
+                    ", size: %" FMT_ioreq_size ", type: %"FMT_ioreq_size"\n",
                     req->state, req->data_is_ptr, req->addr,
-                    req->data, req->count, req->size);
+                    req->data, req->count, req->size, req->type);
             destroy_hvm_domain(false);
             return;
         }
-- 
1.8.4

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Alexander Graf]

On 26.09.14 20:47, Don Slutz wrote:
> This adds synchronisation of the vcpu registers
> between Xen and QEMU.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Could you instead plug into the existing cpu_synchronize_registers()
framework and just implement register synchronization for the Xen side,
just like it's been done for KVM? :)


Alex

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Alexander Graf]

On 26.09.14 20:47, Don Slutz wrote:
> This adds synchronisation of the vcpu registers
> between Xen and QEMU.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Could you instead plug into the existing cpu_synchronize_registers()
framework and just implement register synchronization for the Xen side,
just like it's been done for KVM? :)


Alex

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

On Fri, 26 Sep 2014, Don Slutz wrote:
> This adds synchronisation of the vcpu registers
> between Xen and QEMU.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

[...]

> diff --git a/xen-hvm.c b/xen-hvm.c
> index 05e522c..e1274bb 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>  
>      handle_buffered_iopage(state);
>      if (req) {
> +#ifdef IOREQ_TYPE_VMWARE_PORT

Is there any reason to #ifdef this code?
Couldn't we just always build it in QEMU?


> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {

I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
from within handle_ioreq.


> +            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,
> +            };
> +            if (!xen_opaque_env) {
> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> +            }
> +            env = xen_opaque_env;
> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> +            env->regs[R_EBX] = (uint32_t)(req->addr);
> +            env->regs[R_ECX] = req->count;
> +            env->regs[R_EDX] = req->size;
> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> +            env->regs[R_EDI] = (uint32_t)(req->data);
> +            handle_ioreq(&fake_req);
> +            req->addr = ((uint64_t)fake_req.data << 32) |
> +                (uint32_t)env->regs[R_EBX];
> +            req->count = env->regs[R_ECX];
> +            req->size = env->regs[R_EDX];
> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> +                (uint32_t)env->regs[R_EDI];

This code could be moved to a separate helper function called by
handle_ioreq.


> +        } else {
> +            handle_ioreq(req);
> +        }
> +#else
>          handle_ioreq(req);
> +#endif

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

On Fri, 26 Sep 2014, Don Slutz wrote:
> This adds synchronisation of the vcpu registers
> between Xen and QEMU.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

[...]

> diff --git a/xen-hvm.c b/xen-hvm.c
> index 05e522c..e1274bb 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>  
>      handle_buffered_iopage(state);
>      if (req) {
> +#ifdef IOREQ_TYPE_VMWARE_PORT

Is there any reason to #ifdef this code?
Couldn't we just always build it in QEMU?


> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {

I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
from within handle_ioreq.


> +            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,
> +            };
> +            if (!xen_opaque_env) {
> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> +            }
> +            env = xen_opaque_env;
> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> +            env->regs[R_EBX] = (uint32_t)(req->addr);
> +            env->regs[R_ECX] = req->count;
> +            env->regs[R_EDX] = req->size;
> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> +            env->regs[R_EDI] = (uint32_t)(req->data);
> +            handle_ioreq(&fake_req);
> +            req->addr = ((uint64_t)fake_req.data << 32) |
> +                (uint32_t)env->regs[R_EBX];
> +            req->count = env->regs[R_ECX];
> +            req->size = env->regs[R_EDX];
> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> +                (uint32_t)env->regs[R_EDI];

This code could be moved to a separate helper function called by
handle_ioreq.


> +        } else {
> +            handle_ioreq(req);
> +        }
> +#else
>          handle_ioreq(req);
> +#endif

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

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.
> > 
> > Signed-off-by: Don Slutz <dslutz@verizon.com>
> 
> [...]
> 
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 05e522c..e1274bb 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
> >  
> >      handle_buffered_iopage(state);
> >      if (req) {
> > +#ifdef IOREQ_TYPE_VMWARE_PORT
> 
> Is there any reason to #ifdef this code?
> Couldn't we just always build it in QEMU?
> 
> 
> > +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> 
> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
> from within handle_ioreq.
> 
> 
> > +            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?
Couldn't Xen send the real req instead? If any case you should spend a
few more words on why you are doing this.


> > +            if (!xen_opaque_env) {
> > +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> > +            }
> > +            env = xen_opaque_env;
> > +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> > +            env->regs[R_EBX] = (uint32_t)(req->addr);
> > +            env->regs[R_ECX] = req->count;
> > +            env->regs[R_EDX] = req->size;
> > +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> > +            env->regs[R_EDI] = (uint32_t)(req->data);
> > +            handle_ioreq(&fake_req);
> > +            req->addr = ((uint64_t)fake_req.data << 32) |
> > +                (uint32_t)env->regs[R_EBX];
> > +            req->count = env->regs[R_ECX];
> > +            req->size = env->regs[R_EDX];
> > +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> > +                (uint32_t)env->regs[R_EDI];
> 
> This code could be moved to a separate helper function called by
> handle_ioreq.
> 
> 
> > +        } else {
> > +            handle_ioreq(req);
> > +        }
> > +#else
> >          handle_ioreq(req);
> > +#endif
> 

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

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.
> > 
> > Signed-off-by: Don Slutz <dslutz@verizon.com>
> 
> [...]
> 
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 05e522c..e1274bb 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
> >  
> >      handle_buffered_iopage(state);
> >      if (req) {
> > +#ifdef IOREQ_TYPE_VMWARE_PORT
> 
> Is there any reason to #ifdef this code?
> Couldn't we just always build it in QEMU?
> 
> 
> > +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> 
> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
> from within handle_ioreq.
> 
> 
> > +            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?
Couldn't Xen send the real req instead? If any case you should spend a
few more words on why you are doing this.


> > +            if (!xen_opaque_env) {
> > +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> > +            }
> > +            env = xen_opaque_env;
> > +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> > +            env->regs[R_EBX] = (uint32_t)(req->addr);
> > +            env->regs[R_ECX] = req->count;
> > +            env->regs[R_EDX] = req->size;
> > +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> > +            env->regs[R_EDI] = (uint32_t)(req->data);
> > +            handle_ioreq(&fake_req);
> > +            req->addr = ((uint64_t)fake_req.data << 32) |
> > +                (uint32_t)env->regs[R_EBX];
> > +            req->count = env->regs[R_ECX];
> > +            req->size = env->regs[R_EDX];
> > +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> > +                (uint32_t)env->regs[R_EDI];
> 
> This code could be moved to a separate helper function called by
> handle_ioreq.
> 
> 
> > +        } else {
> > +            handle_ioreq(req);
> > +        }
> > +#else
> >          handle_ioreq(req);
> > +#endif
> 

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 29/09/2014 10:12, Alexander Graf ha scritto:
> Could you instead plug into the existing cpu_synchronize_registers()
> framework and just implement register synchronization for the Xen side,
> just like it's been done for KVM? :)

No, because here it's Xen that sends out the register contents.  With
KVM, it's QEMU that requests the register contents.

Paolo

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 29/09/2014 10:12, Alexander Graf ha scritto:
> Could you instead plug into the existing cpu_synchronize_registers()
> framework and just implement register synchronization for the Xen side,
> just like it's been done for KVM? :)

No, because here it's Xen that sends out the register contents.  With
KVM, it's QEMU that requests the register contents.

Paolo

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Alexander Graf]

On 29.09.14 13:10, Paolo Bonzini wrote:
> Il 29/09/2014 10:12, Alexander Graf ha scritto:
>> Could you instead plug into the existing cpu_synchronize_registers()
>> framework and just implement register synchronization for the Xen side,
>> just like it's been done for KVM? :)
> 
> No, because here it's Xen that sends out the register contents.  With
> KVM, it's QEMU that requests the register contents.

So? We could still reuse the same infrastructure:


  cpu_handle_ioreq()
  {
    ...

    if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
        cpu->xen_vcpu_dirty = true;
        synchronize_xen_to_env(xenptr, cpu);
    }

    handle_ioreq();

    if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
        cpu->xen_vcpu_dirty = false;
        synchronize_env_to_xen(xenptr, cpu);
    }

    ...
  }

  void xen_cpu_synchronize_state(CPUState *cpu)
  {
    assert(cpu->xen_vcpu_dirty);
  }

Then no changes to the vmport code would be necessary and this problems
where some code path wants to do direct access to registers
automatically tells us that things are broken.


Alex

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Alexander Graf]

On 29.09.14 13:10, Paolo Bonzini wrote:
> Il 29/09/2014 10:12, Alexander Graf ha scritto:
>> Could you instead plug into the existing cpu_synchronize_registers()
>> framework and just implement register synchronization for the Xen side,
>> just like it's been done for KVM? :)
> 
> No, because here it's Xen that sends out the register contents.  With
> KVM, it's QEMU that requests the register contents.

So? We could still reuse the same infrastructure:


  cpu_handle_ioreq()
  {
    ...

    if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
        cpu->xen_vcpu_dirty = true;
        synchronize_xen_to_env(xenptr, cpu);
    }

    handle_ioreq();

    if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
        cpu->xen_vcpu_dirty = false;
        synchronize_env_to_xen(xenptr, cpu);
    }

    ...
  }

  void xen_cpu_synchronize_state(CPUState *cpu)
  {
    assert(cpu->xen_vcpu_dirty);
  }

Then no changes to the vmport code would be necessary and this problems
where some code path wants to do direct access to registers
automatically tells us that things are broken.


Alex

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 29/09/2014 13:53, Alexander Graf ha scritto:
> 
>   cpu_handle_ioreq()
>   {
>     ...
> 
>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>         cpu->xen_vcpu_dirty = true;
>         synchronize_xen_to_env(xenptr, cpu);
>     }
> 
>     handle_ioreq();
> 
>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>         cpu->xen_vcpu_dirty = false;
>         synchronize_env_to_xen(xenptr, cpu);
>     }
> 
>     ...
>   }
> 
>   void xen_cpu_synchronize_state(CPUState *cpu)
>   {
>     assert(cpu->xen_vcpu_dirty);
>   }
> 
> Then no changes to the vmport code would be necessary and this problems
> where some code path wants to do direct access to registers
> automatically tells us that things are broken.

Yeah, that would be possible.  You do not even need synchronize_state,
it seems to me that it introduces more complication for little gain.

Paolo

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 29/09/2014 13:53, Alexander Graf ha scritto:
> 
>   cpu_handle_ioreq()
>   {
>     ...
> 
>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>         cpu->xen_vcpu_dirty = true;
>         synchronize_xen_to_env(xenptr, cpu);
>     }
> 
>     handle_ioreq();
> 
>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>         cpu->xen_vcpu_dirty = false;
>         synchronize_env_to_xen(xenptr, cpu);
>     }
> 
>     ...
>   }
> 
>   void xen_cpu_synchronize_state(CPUState *cpu)
>   {
>     assert(cpu->xen_vcpu_dirty);
>   }
> 
> Then no changes to the vmport code would be necessary and this problems
> where some code path wants to do direct access to registers
> automatically tells us that things are broken.

Yeah, that would be possible.  You do not even need synchronize_state,
it seems to me that it introduces more complication for little gain.

Paolo

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Alexander Graf]

On 29.09.14 14:21, Paolo Bonzini wrote:
> Il 29/09/2014 13:53, Alexander Graf ha scritto:
>>
>>   cpu_handle_ioreq()
>>   {
>>     ...
>>
>>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>>         cpu->xen_vcpu_dirty = true;
>>         synchronize_xen_to_env(xenptr, cpu);
>>     }
>>
>>     handle_ioreq();
>>
>>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>>         cpu->xen_vcpu_dirty = false;
>>         synchronize_env_to_xen(xenptr, cpu);
>>     }
>>
>>     ...
>>   }
>>
>>   void xen_cpu_synchronize_state(CPUState *cpu)
>>   {
>>     assert(cpu->xen_vcpu_dirty);
>>   }
>>
>> Then no changes to the vmport code would be necessary and this problems
>> where some code path wants to do direct access to registers
>> automatically tells us that things are broken.
> 
> Yeah, that would be possible.  You do not even need synchronize_state,
> it seems to me that it introduces more complication for little gain.

Well, it makes all accels behave the same and keep information always at
a single entity (the env struct). I don't think the vmport code should
have knowledge of a xen env struct.


Alex

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Alexander Graf]

On 29.09.14 14:21, Paolo Bonzini wrote:
> Il 29/09/2014 13:53, Alexander Graf ha scritto:
>>
>>   cpu_handle_ioreq()
>>   {
>>     ...
>>
>>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>>         cpu->xen_vcpu_dirty = true;
>>         synchronize_xen_to_env(xenptr, cpu);
>>     }
>>
>>     handle_ioreq();
>>
>>     if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>>         cpu->xen_vcpu_dirty = false;
>>         synchronize_env_to_xen(xenptr, cpu);
>>     }
>>
>>     ...
>>   }
>>
>>   void xen_cpu_synchronize_state(CPUState *cpu)
>>   {
>>     assert(cpu->xen_vcpu_dirty);
>>   }
>>
>> Then no changes to the vmport code would be necessary and this problems
>> where some code path wants to do direct access to registers
>> automatically tells us that things are broken.
> 
> Yeah, that would be possible.  You do not even need synchronize_state,
> it seems to me that it introduces more complication for little gain.

Well, it makes all accels behave the same and keep information always at
a single entity (the env struct). I don't think the vmport code should
have knowledge of a xen env struct.


Alex

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 29/09/2014 14:57, Alexander Graf ha scritto:
>> > Yeah, that would be possible.  You do not even need synchronize_state,
>> > it seems to me that it introduces more complication for little gain.
> Well, it makes all accels behave the same and keep information always at
> a single entity (the env struct). I don't think the vmport code should
> have knowledge of a xen env struct.

Yes, we agree on that.  In fact the right CPU is already stored in
send_vcpu of XenIOState.

Paolo

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 29/09/2014 14:57, Alexander Graf ha scritto:
>> > Yeah, that would be possible.  You do not even need synchronize_state,
>> > it seems to me that it introduces more complication for little gain.
> Well, it makes all accels behave the same and keep information always at
> a single entity (the env struct). I don't think the vmport code should
> have knowledge of a xen env struct.

Yes, we agree on that.  In fact the right CPU is already stored in
send_vcpu of XenIOState.

Paolo

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

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.
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> [...]
>>
>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>> index 05e522c..e1274bb 100644
>>> --- a/xen-hvm.c
>>> +++ b/xen-hvm.c
>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>   
>>>       handle_buffered_iopage(state);
>>>       if (req) {
>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>> Is there any reason to #ifdef this code?
>> Couldn't we just always build it in QEMU?

This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
or earlier installed; and work.

>>
>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>> from within handle_ioreq.
>>

Ok, I can move it.


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

>   If any case you should spend a
> few more words on why you are doing this.
>

Sure.  Will add some form of the above as part of the commit message.

>>> +            if (!xen_opaque_env) {
>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>> +            }
>>> +            env = xen_opaque_env;
>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>> +            env->regs[R_ECX] = req->count;
>>> +            env->regs[R_EDX] = req->size;
>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>> +            handle_ioreq(&fake_req);
>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>> +                (uint32_t)env->regs[R_EBX];
>>> +            req->count = env->regs[R_ECX];
>>> +            req->size = env->regs[R_EDX];
>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>> +                (uint32_t)env->regs[R_EDI];
>> This code could be moved to a separate helper function called by
>> handle_ioreq.
>>

Ok.

    -Don Slutz

>>> +        } else {
>>> +            handle_ioreq(req);
>>> +        }
>>> +#else
>>>           handle_ioreq(req);
>>> +#endif

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

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.
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> [...]
>>
>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>> index 05e522c..e1274bb 100644
>>> --- a/xen-hvm.c
>>> +++ b/xen-hvm.c
>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>   
>>>       handle_buffered_iopage(state);
>>>       if (req) {
>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>> Is there any reason to #ifdef this code?
>> Couldn't we just always build it in QEMU?

This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
or earlier installed; and work.

>>
>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>> from within handle_ioreq.
>>

Ok, I can move it.


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

>   If any case you should spend a
> few more words on why you are doing this.
>

Sure.  Will add some form of the above as part of the commit message.

>>> +            if (!xen_opaque_env) {
>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>> +            }
>>> +            env = xen_opaque_env;
>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>> +            env->regs[R_ECX] = req->count;
>>> +            env->regs[R_EDX] = req->size;
>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>> +            handle_ioreq(&fake_req);
>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>> +                (uint32_t)env->regs[R_EBX];
>>> +            req->count = env->regs[R_ECX];
>>> +            req->size = env->regs[R_EDX];
>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>> +                (uint32_t)env->regs[R_EDI];
>> This code could be moved to a separate helper function called by
>> handle_ioreq.
>>

Ok.

    -Don Slutz

>>> +        } else {
>>> +            handle_ioreq(req);
>>> +        }
>>> +#else
>>>           handle_ioreq(req);
>>> +#endif

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

On 09/29/14 07:53, Alexander Graf wrote:
>
> On 29.09.14 13:10, Paolo Bonzini wrote:
>> Il 29/09/2014 10:12, Alexander Graf ha scritto:
>>> Could you instead plug into the existing cpu_synchronize_registers()
>>> framework and just implement register synchronization for the Xen side,
>>> just like it's been done for KVM? :)
>> No, because here it's Xen that sends out the register contents.  With
>> KVM, it's QEMU that requests the register contents.
> So? We could still reuse the same infrastructure:
>
>
>    cpu_handle_ioreq()
>    {
>      ...
>
>      if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>          cpu->xen_vcpu_dirty = true;

I am assuming that you mean something like:

     X86CPU *cpu = X86_CPU(current_cpu);
     cpu->xen_vcpu_dirty = true;

because cpu is not defined here.  And this has a major issue
because current_cpu is NULL for Xen, so you have added a SIGSEGV.

If you are trying to say "lets build a CPUState state object here
and set current_cpu to it, and then set the xen_vcpu_dirty" then
yes that could be done.  Adding all the code to build a CPUState state
object and X86CPUClass object correctly just to use the code in
vmport.c looked to me to be a lot of work for little benefit.


If you want to go with not adding the objects, I could change
to having a "xen X86CPU" that I set current_cpu to and
then back to NULL when done.  That (I think) would drop
all changes to vmport.c

>          synchronize_xen_to_env(xenptr, cpu);
>      }
>
>      handle_ioreq();
>
>      if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>          cpu->xen_vcpu_dirty = false;
>          synchronize_env_to_xen(xenptr, cpu);
>      }
>
>      ...
>    }
>
>    void xen_cpu_synchronize_state(CPUState *cpu)
>    {
>      assert(cpu->xen_vcpu_dirty);
>    }
>
> Then no changes to the vmport code would be necessary and this problems
> where some code path wants to do direct access to registers
> automatically tells us that things are broken.

The SIGSEGV already does this.

     -Don Slutz

>
> Alex

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

On 09/29/14 07:53, Alexander Graf wrote:
>
> On 29.09.14 13:10, Paolo Bonzini wrote:
>> Il 29/09/2014 10:12, Alexander Graf ha scritto:
>>> Could you instead plug into the existing cpu_synchronize_registers()
>>> framework and just implement register synchronization for the Xen side,
>>> just like it's been done for KVM? :)
>> No, because here it's Xen that sends out the register contents.  With
>> KVM, it's QEMU that requests the register contents.
> So? We could still reuse the same infrastructure:
>
>
>    cpu_handle_ioreq()
>    {
>      ...
>
>      if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>          cpu->xen_vcpu_dirty = true;

I am assuming that you mean something like:

     X86CPU *cpu = X86_CPU(current_cpu);
     cpu->xen_vcpu_dirty = true;

because cpu is not defined here.  And this has a major issue
because current_cpu is NULL for Xen, so you have added a SIGSEGV.

If you are trying to say "lets build a CPUState state object here
and set current_cpu to it, and then set the xen_vcpu_dirty" then
yes that could be done.  Adding all the code to build a CPUState state
object and X86CPUClass object correctly just to use the code in
vmport.c looked to me to be a lot of work for little benefit.


If you want to go with not adding the objects, I could change
to having a "xen X86CPU" that I set current_cpu to and
then back to NULL when done.  That (I think) would drop
all changes to vmport.c

>          synchronize_xen_to_env(xenptr, cpu);
>      }
>
>      handle_ioreq();
>
>      if (req->type == IOPORT_REGISTERS_SYNCHRONIZED) {
>          cpu->xen_vcpu_dirty = false;
>          synchronize_env_to_xen(xenptr, cpu);
>      }
>
>      ...
>    }
>
>    void xen_cpu_synchronize_state(CPUState *cpu)
>    {
>      assert(cpu->xen_vcpu_dirty);
>    }
>
> Then no changes to the vmport code would be necessary and this problems
> where some code path wants to do direct access to registers
> automatically tells us that things are broken.

The SIGSEGV already does this.

     -Don Slutz

>
> Alex

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

On 09/29/14 09:14, Paolo Bonzini wrote:
> Il 29/09/2014 14:57, Alexander Graf ha scritto:
>>>> Yeah, that would be possible.  You do not even need synchronize_state,
>>>> it seems to me that it introduces more complication for little gain.
>> Well, it makes all accels behave the same and keep information always at
>> a single entity (the env struct). I don't think the vmport code should
>> have knowledge of a xen env struct.
> Yes, we agree on that.  In fact the right CPU is already stored in
> send_vcpu of XenIOState.

I did reply up the thread because the info is missing here.

Basically current_cpu is NULL here, and I think that no CPU objects
have been created for Xen.  So there is no VCPU object to select.

     -Don Slutz

> Paolo

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

On 09/29/14 09:14, Paolo Bonzini wrote:
> Il 29/09/2014 14:57, Alexander Graf ha scritto:
>>>> Yeah, that would be possible.  You do not even need synchronize_state,
>>>> it seems to me that it introduces more complication for little gain.
>> Well, it makes all accels behave the same and keep information always at
>> a single entity (the env struct). I don't think the vmport code should
>> have knowledge of a xen env struct.
> Yes, we agree on that.  In fact the right CPU is already stored in
> send_vcpu of XenIOState.

I did reply up the thread because the info is missing here.

Basically current_cpu is NULL here, and I think that no CPU objects
have been created for Xen.  So there is no VCPU object to select.

     -Don Slutz

> Paolo

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 30/09/2014 03:05, Don Slutz ha scritto:
> 
> Basically current_cpu is NULL here, and I think that no CPU objects
> have been created for Xen.  So there is no VCPU object to select.

CPU objects are created for Xen HVM, though not for PV.

You would have to visit the list of CPUs and fill in an array in
XenIOState (e.g. "X86CPU **cpu_by_ioreq_id" array in XenIOState).  Then
you can set

    current_cpu = state->cpu_by_ioreq_id[state->send_vcpu];
    handle_ioreq(req);
    current_cpu = NULL;

in cpu_handle_ioreq.

Paolo

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paolo Bonzini]

Il 30/09/2014 03:05, Don Slutz ha scritto:
> 
> Basically current_cpu is NULL here, and I think that no CPU objects
> have been created for Xen.  So there is no VCPU object to select.

CPU objects are created for Xen HVM, though not for PV.

You would have to visit the list of CPUs and fill in an array in
XenIOState (e.g. "X86CPU **cpu_by_ioreq_id" array in XenIOState).  Then
you can set

    current_cpu = state->cpu_by_ioreq_id[state->send_vcpu];
    handle_ioreq(req);
    current_cpu = NULL;

in cpu_handle_ioreq.

Paolo

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

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.
> > > > 
> > > > Signed-off-by: Don Slutz <dslutz@verizon.com>
> > > [...]
> > > 
> > > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > > index 05e522c..e1274bb 100644
> > > > --- a/xen-hvm.c
> > > > +++ b/xen-hvm.c
> > > > @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
> > > >         handle_buffered_iopage(state);
> > > >       if (req) {
> > > > +#ifdef IOREQ_TYPE_VMWARE_PORT
> > > Is there any reason to #ifdef this code?
> > > Couldn't we just always build it in QEMU?
> 
> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
> or earlier installed; and work.

I would rather remove the #ifdef from here and add any necessary
compatibility stuff to include/hw/xen/xen_common.h.


> > > > +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> > > I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
> > > from within handle_ioreq.
> > > 
> 
> Ok, I can move it.
> 
> 
> > > > +            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.


> >   If any case you should spend a
> > few more words on why you are doing this.
> > 
> 
> Sure.  Will add some form of the above as part of the commit message.
> 
> > > > +            if (!xen_opaque_env) {
> > > > +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> > > > +            }
> > > > +            env = xen_opaque_env;
> > > > +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> > > > +            env->regs[R_EBX] = (uint32_t)(req->addr);
> > > > +            env->regs[R_ECX] = req->count;
> > > > +            env->regs[R_EDX] = req->size;
> > > > +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> > > > +            env->regs[R_EDI] = (uint32_t)(req->data);
> > > > +            handle_ioreq(&fake_req);
> > > > +            req->addr = ((uint64_t)fake_req.data << 32) |
> > > > +                (uint32_t)env->regs[R_EBX];
> > > > +            req->count = env->regs[R_ECX];
> > > > +            req->size = env->regs[R_EDX];
> > > > +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> > > > +                (uint32_t)env->regs[R_EDI];
> > > This code could be moved to a separate helper function called by
> > > handle_ioreq.
> > > 
> 
> Ok.
> 
>    -Don Slutz
> 
> > > > +        } else {
> > > > +            handle_ioreq(req);
> > > > +        }
> > > > +#else
> > > >           handle_ioreq(req);
> > > > +#endif
> 

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

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.
> > > > 
> > > > Signed-off-by: Don Slutz <dslutz@verizon.com>
> > > [...]
> > > 
> > > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > > index 05e522c..e1274bb 100644
> > > > --- a/xen-hvm.c
> > > > +++ b/xen-hvm.c
> > > > @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
> > > >         handle_buffered_iopage(state);
> > > >       if (req) {
> > > > +#ifdef IOREQ_TYPE_VMWARE_PORT
> > > Is there any reason to #ifdef this code?
> > > Couldn't we just always build it in QEMU?
> 
> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
> or earlier installed; and work.

I would rather remove the #ifdef from here and add any necessary
compatibility stuff to include/hw/xen/xen_common.h.


> > > > +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> > > I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
> > > from within handle_ioreq.
> > > 
> 
> Ok, I can move it.
> 
> 
> > > > +            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.


> >   If any case you should spend a
> > few more words on why you are doing this.
> > 
> 
> Sure.  Will add some form of the above as part of the commit message.
> 
> > > > +            if (!xen_opaque_env) {
> > > > +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> > > > +            }
> > > > +            env = xen_opaque_env;
> > > > +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> > > > +            env->regs[R_EBX] = (uint32_t)(req->addr);
> > > > +            env->regs[R_ECX] = req->count;
> > > > +            env->regs[R_EDX] = req->size;
> > > > +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> > > > +            env->regs[R_EDI] = (uint32_t)(req->data);
> > > > +            handle_ioreq(&fake_req);
> > > > +            req->addr = ((uint64_t)fake_req.data << 32) |
> > > > +                (uint32_t)env->regs[R_EBX];
> > > > +            req->count = env->regs[R_ECX];
> > > > +            req->size = env->regs[R_EDX];
> > > > +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> > > > +                (uint32_t)env->regs[R_EDI];
> > > This code could be moved to a separate helper function called by
> > > handle_ioreq.
> > > 
> 
> Ok.
> 
>    -Don Slutz
> 
> > > > +        } else {
> > > > +            handle_ioreq(req);
> > > > +        }
> > > > +#else
> > > >           handle_ioreq(req);
> > > > +#endif
> 

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Slutz, Donald Christopher]

On 09/30/14 06:35, Stefano Stabellini wrote:
> 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.
>>>>>
>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>> [...]
>>>>
>>>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>>>> index 05e522c..e1274bb 100644
>>>>> --- a/xen-hvm.c
>>>>> +++ b/xen-hvm.c
>>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>>>          handle_buffered_iopage(state);
>>>>>        if (req) {
>>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>>>> Is there any reason to #ifdef this code?
>>>> Couldn't we just always build it in QEMU?
>> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
>> or earlier installed; and work.
> I would rather remove the #ifdef from here and add any necessary
> compatibility stuff to include/hw/xen/xen_common.h.
>

Ok, will do.

>>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>>>> from within handle_ioreq.
>>>>
>> Ok, I can move it.
>>
>>
>>>>> +            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 :-)

None taken.

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

I can, it is just slower.  This would require 2 new types.  1 for regs to
QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).

     -Don Slutz

>>>    If any case you should spend a
>>> few more words on why you are doing this.
>>>
>> Sure.  Will add some form of the above as part of the commit message.
>>
>>>>> +            if (!xen_opaque_env) {
>>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>>>> +            }
>>>>> +            env = xen_opaque_env;
>>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>>>> +            env->regs[R_ECX] = req->count;
>>>>> +            env->regs[R_EDX] = req->size;
>>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>>>> +            handle_ioreq(&fake_req);
>>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>>>> +                (uint32_t)env->regs[R_EBX];
>>>>> +            req->count = env->regs[R_ECX];
>>>>> +            req->size = env->regs[R_EDX];
>>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>>>> +                (uint32_t)env->regs[R_EDI];
>>>> This code could be moved to a separate helper function called by
>>>> handle_ioreq.
>>>>
>> Ok.
>>
>>     -Don Slutz
>>
>>>>> +        } else {
>>>>> +            handle_ioreq(req);
>>>>> +        }
>>>>> +#else
>>>>>            handle_ioreq(req);
>>>>> +#endif

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Slutz, Donald Christopher]

On 09/30/14 06:35, Stefano Stabellini wrote:
> 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.
>>>>>
>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>> [...]
>>>>
>>>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>>>> index 05e522c..e1274bb 100644
>>>>> --- a/xen-hvm.c
>>>>> +++ b/xen-hvm.c
>>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>>>          handle_buffered_iopage(state);
>>>>>        if (req) {
>>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>>>> Is there any reason to #ifdef this code?
>>>> Couldn't we just always build it in QEMU?
>> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
>> or earlier installed; and work.
> I would rather remove the #ifdef from here and add any necessary
> compatibility stuff to include/hw/xen/xen_common.h.
>

Ok, will do.

>>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>>>> from within handle_ioreq.
>>>>
>> Ok, I can move it.
>>
>>
>>>>> +            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 :-)

None taken.

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

I can, it is just slower.  This would require 2 new types.  1 for regs to
QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).

     -Don Slutz

>>>    If any case you should spend a
>>> few more words on why you are doing this.
>>>
>> Sure.  Will add some form of the above as part of the commit message.
>>
>>>>> +            if (!xen_opaque_env) {
>>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>>>> +            }
>>>>> +            env = xen_opaque_env;
>>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>>>> +            env->regs[R_ECX] = req->count;
>>>>> +            env->regs[R_EDX] = req->size;
>>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>>>> +            handle_ioreq(&fake_req);
>>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>>>> +                (uint32_t)env->regs[R_EBX];
>>>>> +            req->count = env->regs[R_ECX];
>>>>> +            req->size = env->regs[R_EDX];
>>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>>>> +                (uint32_t)env->regs[R_EDI];
>>>> This code could be moved to a separate helper function called by
>>>> handle_ioreq.
>>>>
>> Ok.
>>
>>     -Don Slutz
>>
>>>>> +        } else {
>>>>> +            handle_ioreq(req);
>>>>> +        }
>>>>> +#else
>>>>>            handle_ioreq(req);
>>>>> +#endif

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
> On 09/30/14 06:35, Stefano Stabellini wrote:
> > 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.
> >>>>>
> >>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> >>>> [...]
> >>>>
> >>>>> diff --git a/xen-hvm.c b/xen-hvm.c
> >>>>> index 05e522c..e1274bb 100644
> >>>>> --- a/xen-hvm.c
> >>>>> +++ b/xen-hvm.c
> >>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
> >>>>>          handle_buffered_iopage(state);
> >>>>>        if (req) {
> >>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
> >>>> Is there any reason to #ifdef this code?
> >>>> Couldn't we just always build it in QEMU?
> >> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
> >> or earlier installed; and work.
> > I would rather remove the #ifdef from here and add any necessary
> > compatibility stuff to include/hw/xen/xen_common.h.
> >
> 
> Ok, will do.
> 
> >>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> >>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
> >>>> from within handle_ioreq.
> >>>>
> >> Ok, I can move it.
> >>
> >>
> >>>>> +            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 :-)
> 
> None taken.
> 
> > 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.
> >
> 
> I can, it is just slower.  This would require 2 new types.  1 for regs to
> QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
> to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
> QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).

This is not an high performance device, is it?

In any case I agree it would be better to avoid it.
I wonder if we could send both ioreqs at once from Xen and back from
QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
the size of ioreq_t only for this ioreq type.
Another maybe simpler possibility would be introducing a union in
ioreq_t with the registers.
Anything would be OK for me but I would like to see the registers being
passes explicitely by Xen rather than "hiding" them into other ioreq
fields.


> >>>    If any case you should spend a
> >>> few more words on why you are doing this.
> >>>
> >> Sure.  Will add some form of the above as part of the commit message.
> >>
> >>>>> +            if (!xen_opaque_env) {
> >>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> >>>>> +            }
> >>>>> +            env = xen_opaque_env;
> >>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> >>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
> >>>>> +            env->regs[R_ECX] = req->count;
> >>>>> +            env->regs[R_EDX] = req->size;
> >>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> >>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
> >>>>> +            handle_ioreq(&fake_req);
> >>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
> >>>>> +                (uint32_t)env->regs[R_EBX];
> >>>>> +            req->count = env->regs[R_ECX];
> >>>>> +            req->size = env->regs[R_EDX];
> >>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> >>>>> +                (uint32_t)env->regs[R_EDI];
> >>>> This code could be moved to a separate helper function called by
> >>>> handle_ioreq.
> >>>>
> >> Ok.
> >>
> >>     -Don Slutz
> >>
> >>>>> +        } else {
> >>>>> +            handle_ioreq(req);
> >>>>> +        }
> >>>>> +#else
> >>>>>            handle_ioreq(req);
> >>>>> +#endif
> 

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Stefano Stabellini]

On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
> On 09/30/14 06:35, Stefano Stabellini wrote:
> > 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.
> >>>>>
> >>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> >>>> [...]
> >>>>
> >>>>> diff --git a/xen-hvm.c b/xen-hvm.c
> >>>>> index 05e522c..e1274bb 100644
> >>>>> --- a/xen-hvm.c
> >>>>> +++ b/xen-hvm.c
> >>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
> >>>>>          handle_buffered_iopage(state);
> >>>>>        if (req) {
> >>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
> >>>> Is there any reason to #ifdef this code?
> >>>> Couldn't we just always build it in QEMU?
> >> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
> >> or earlier installed; and work.
> > I would rather remove the #ifdef from here and add any necessary
> > compatibility stuff to include/hw/xen/xen_common.h.
> >
> 
> Ok, will do.
> 
> >>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> >>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
> >>>> from within handle_ioreq.
> >>>>
> >> Ok, I can move it.
> >>
> >>
> >>>>> +            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 :-)
> 
> None taken.
> 
> > 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.
> >
> 
> I can, it is just slower.  This would require 2 new types.  1 for regs to
> QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
> to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
> QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).

This is not an high performance device, is it?

In any case I agree it would be better to avoid it.
I wonder if we could send both ioreqs at once from Xen and back from
QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
the size of ioreq_t only for this ioreq type.
Another maybe simpler possibility would be introducing a union in
ioreq_t with the registers.
Anything would be OK for me but I would like to see the registers being
passes explicitely by Xen rather than "hiding" them into other ioreq
fields.


> >>>    If any case you should spend a
> >>> few more words on why you are doing this.
> >>>
> >> Sure.  Will add some form of the above as part of the commit message.
> >>
> >>>>> +            if (!xen_opaque_env) {
> >>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> >>>>> +            }
> >>>>> +            env = xen_opaque_env;
> >>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> >>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
> >>>>> +            env->regs[R_ECX] = req->count;
> >>>>> +            env->regs[R_EDX] = req->size;
> >>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> >>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
> >>>>> +            handle_ioreq(&fake_req);
> >>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
> >>>>> +                (uint32_t)env->regs[R_EBX];
> >>>>> +            req->count = env->regs[R_ECX];
> >>>>> +            req->size = env->regs[R_EDX];
> >>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> >>>>> +                (uint32_t)env->regs[R_EDI];
> >>>> This code could be moved to a separate helper function called by
> >>>> handle_ioreq.
> >>>>
> >> Ok.
> >>
> >>     -Don Slutz
> >>
> >>>>> +        } else {
> >>>>> +            handle_ioreq(req);
> >>>>> +        }
> >>>>> +#else
> >>>>>            handle_ioreq(req);
> >>>>> +#endif
> 

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

On 10/01/14 05:20, Stefano Stabellini wrote:
> On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
>> On 09/30/14 06:35, Stefano Stabellini wrote:
>>> 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.
>>>>>>>
>>>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>>>>>> index 05e522c..e1274bb 100644
>>>>>>> --- a/xen-hvm.c
>>>>>>> +++ b/xen-hvm.c
>>>>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>>>>>           handle_buffered_iopage(state);
>>>>>>>         if (req) {
>>>>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>>>>>> Is there any reason to #ifdef this code?
>>>>>> Couldn't we just always build it in QEMU?
>>>> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
>>>> or earlier installed; and work.
>>> I would rather remove the #ifdef from here and add any necessary
>>> compatibility stuff to include/hw/xen/xen_common.h.
>>>
>> Ok, will do.
>>
>>>>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>>>>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>>>>>> from within handle_ioreq.
>>>>>>
>>>> Ok, I can move it.
>>>>
>>>>
>>>>>>> +            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 :-)
>> None taken.
>>
>>> 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.
>>>
>> I can, it is just slower.  This would require 2 new types.  1 for regs to
>> QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
>> to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
>> QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).
> This is not an high performance device, is it?

Depends on how you view mouse device speed.

> In any case I agree it would be better to avoid it.
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
> the size of ioreq_t only for this ioreq type.

No idea how do do this since this is an array in a shared page.

> Another maybe simpler possibility would be introducing a union in
> ioreq_t with the registers.
> Anything would be OK for me but I would like to see the registers being
> passes explicitely by Xen rather than "hiding" them into other ioreq
> fields.
>

I considered a union, but this can be a issue with older QEMU
and newer Xen.  Since it appears that you care, I will add a new
struct for this.  The problem is that it is a "union" and so must
match on the common fields.

    -Don Slutz



>>>>>     If any case you should spend a
>>>>> few more words on why you are doing this.
>>>>>
>>>> Sure.  Will add some form of the above as part of the commit message.
>>>>
>>>>>>> +            if (!xen_opaque_env) {
>>>>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>>>>>> +            }
>>>>>>> +            env = xen_opaque_env;
>>>>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>>>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>>>>>> +            env->regs[R_ECX] = req->count;
>>>>>>> +            env->regs[R_EDX] = req->size;
>>>>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>>>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>>>>>> +            handle_ioreq(&fake_req);
>>>>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>>>>>> +                (uint32_t)env->regs[R_EBX];
>>>>>>> +            req->count = env->regs[R_ECX];
>>>>>>> +            req->size = env->regs[R_EDX];
>>>>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>>>>>> +                (uint32_t)env->regs[R_EDI];
>>>>>> This code could be moved to a separate helper function called by
>>>>>> handle_ioreq.
>>>>>>
>>>> Ok.
>>>>
>>>>      -Don Slutz
>>>>
>>>>>>> +        } else {
>>>>>>> +            handle_ioreq(req);
>>>>>>> +        }
>>>>>>> +#else
>>>>>>>             handle_ioreq(req);
>>>>>>> +#endif

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Don Slutz]

On 10/01/14 05:20, Stefano Stabellini wrote:
> On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
>> On 09/30/14 06:35, Stefano Stabellini wrote:
>>> 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.
>>>>>>>
>>>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>>>>>> index 05e522c..e1274bb 100644
>>>>>>> --- a/xen-hvm.c
>>>>>>> +++ b/xen-hvm.c
>>>>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void *opaque)
>>>>>>>           handle_buffered_iopage(state);
>>>>>>>         if (req) {
>>>>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
>>>>>> Is there any reason to #ifdef this code?
>>>>>> Couldn't we just always build it in QEMU?
>>>> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
>>>> or earlier installed; and work.
>>> I would rather remove the #ifdef from here and add any necessary
>>> compatibility stuff to include/hw/xen/xen_common.h.
>>>
>> Ok, will do.
>>
>>>>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
>>>>>> I think it would make more sense to check for IOREQ_TYPE_VMWARE_PORT
>>>>>> from within handle_ioreq.
>>>>>>
>>>> Ok, I can move it.
>>>>
>>>>
>>>>>>> +            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 :-)
>> None taken.
>>
>>> 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.
>>>
>> I can, it is just slower.  This would require 2 new types.  1 for regs to
>> QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
>> to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
>> QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).
> This is not an high performance device, is it?

Depends on how you view mouse device speed.

> In any case I agree it would be better to avoid it.
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
> the size of ioreq_t only for this ioreq type.

No idea how do do this since this is an array in a shared page.

> Another maybe simpler possibility would be introducing a union in
> ioreq_t with the registers.
> Anything would be OK for me but I would like to see the registers being
> passes explicitely by Xen rather than "hiding" them into other ioreq
> fields.
>

I considered a union, but this can be a issue with older QEMU
and newer Xen.  Since it appears that you care, I will add a new
struct for this.  The problem is that it is a "union" and so must
match on the common fields.

    -Don Slutz



>>>>>     If any case you should spend a
>>>>> few more words on why you are doing this.
>>>>>
>>>> Sure.  Will add some form of the above as part of the commit message.
>>>>
>>>>>>> +            if (!xen_opaque_env) {
>>>>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
>>>>>>> +            }
>>>>>>> +            env = xen_opaque_env;
>>>>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
>>>>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
>>>>>>> +            env->regs[R_ECX] = req->count;
>>>>>>> +            env->regs[R_EDX] = req->size;
>>>>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
>>>>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
>>>>>>> +            handle_ioreq(&fake_req);
>>>>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
>>>>>>> +                (uint32_t)env->regs[R_EBX];
>>>>>>> +            req->count = env->regs[R_ECX];
>>>>>>> +            req->size = env->regs[R_EDX];
>>>>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
>>>>>>> +                (uint32_t)env->regs[R_EDI];
>>>>>> This code could be moved to a separate helper function called by
>>>>>> handle_ioreq.
>>>>>>
>>>> Ok.
>>>>
>>>>      -Don Slutz
>>>>
>>>>>>> +        } else {
>>>>>>> +            handle_ioreq(req);
>>>>>>> +        }
>>>>>>> +#else
>>>>>>>             handle_ioreq(req);
>>>>>>> +#endif

Re: [Qemu-devel] [Xen-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Ian Campbell]

On Wed, 2014-10-01 at 10:20 +0100, Stefano Stabellini wrote:
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
> the size of ioreq_t only for this ioreq type.

Random idea: Why new add a IOREQ_TYPE_FULL_STATE which would be issued
for these ports and let qemu decode the fact that it is vmware
internally? That might be a more generically useful interface in the
future?

WRT to fitting all the register state in the current sized request, you
could declare that this new thing takes multiple slots.

Also, I may be wrong, but I thought most IOREQs were synchronous so only
one slot was ever used? The buffered ioreq stuff has a separate ring (or
uses a different part of the page, or something). I might be talking
nonsense here though ;-)

Ian.

Re: [Xen-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Ian Campbell]

On Wed, 2014-10-01 at 10:20 +0100, Stefano Stabellini wrote:
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
> the size of ioreq_t only for this ioreq type.

Random idea: Why new add a IOREQ_TYPE_FULL_STATE which would be issued
for these ports and let qemu decode the fact that it is vmware
internally? That might be a more generically useful interface in the
future?

WRT to fitting all the register state in the current sized request, you
could declare that this new thing takes multiple slots.

Also, I may be wrong, but I thought most IOREQs were synchronous so only
one slot was ever used? The buffered ioreq stuff has a separate ring (or
uses a different part of the page, or something). I might be talking
nonsense here though ;-)

Ian.

Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paul Durrant]

> -----Original Message-----
> From: qemu-devel-bounces+paul.durrant=citrix.com@nongnu.org
> [mailto:qemu-devel-bounces+paul.durrant=citrix.com@nongnu.org] On
> Behalf Of Stefano Stabellini
> Sent: 01 October 2014 10:20
> To: Slutz, Donald Christopher
> Cc: xen-devel@lists.xensource.com; Stefano Stabellini; Markus Armbruster;
> Marcel Apfelbaum; Alexander Graf; qemu-devel@nongnu.org; Michael S.
> Tsirkin; Anthony Liguori; Andreas Färber
> Subject: Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen
> access to vmport
> 
> On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
> > On 09/30/14 06:35, Stefano Stabellini wrote:
> > > 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.
> > >>>>>
> > >>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> > >>>> [...]
> > >>>>
> > >>>>> diff --git a/xen-hvm.c b/xen-hvm.c
> > >>>>> index 05e522c..e1274bb 100644
> > >>>>> --- a/xen-hvm.c
> > >>>>> +++ b/xen-hvm.c
> > >>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void
> *opaque)
> > >>>>>          handle_buffered_iopage(state);
> > >>>>>        if (req) {
> > >>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
> > >>>> Is there any reason to #ifdef this code?
> > >>>> Couldn't we just always build it in QEMU?
> > >> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
> > >> or earlier installed; and work.
> > > I would rather remove the #ifdef from here and add any necessary
> > > compatibility stuff to include/hw/xen/xen_common.h.
> > >
> >
> > Ok, will do.
> >
> > >>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> > >>>> I think it would make more sense to check for
> IOREQ_TYPE_VMWARE_PORT
> > >>>> from within handle_ioreq.
> > >>>>
> > >> Ok, I can move it.
> > >>
> > >>
> > >>>>> +            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 :-)
> >
> > None taken.
> >
> > > 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.
> > >
> >
> > I can, it is just slower.  This would require 2 new types.  1 for regs to
> > QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
> > to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
> > QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).
> 
> This is not an high performance device, is it?
> 
> In any case I agree it would be better to avoid it.
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT,
> changing
> the size of ioreq_t only for this ioreq type.
> Another maybe simpler possibility would be introducing a union in
> ioreq_t with the registers.
> Anything would be OK for me but I would like to see the registers being
> passes explicitely by Xen rather than "hiding" them into other ioreq
> fields.
> 

Changing the size of ioreq_t would be a bit of a nightmare: The structs are laid out in an array indexed by vcpu and shared by QEMU and Xen.

  Paul

> 
> > >>>    If any case you should spend a
> > >>> few more words on why you are doing this.
> > >>>
> > >> Sure.  Will add some form of the above as part of the commit message.
> > >>
> > >>>>> +            if (!xen_opaque_env) {
> > >>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> > >>>>> +            }
> > >>>>> +            env = xen_opaque_env;
> > >>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> > >>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
> > >>>>> +            env->regs[R_ECX] = req->count;
> > >>>>> +            env->regs[R_EDX] = req->size;
> > >>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> > >>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
> > >>>>> +            handle_ioreq(&fake_req);
> > >>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
> > >>>>> +                (uint32_t)env->regs[R_EBX];
> > >>>>> +            req->count = env->regs[R_ECX];
> > >>>>> +            req->size = env->regs[R_EDX];
> > >>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> > >>>>> +                (uint32_t)env->regs[R_EDI];
> > >>>> This code could be moved to a separate helper function called by
> > >>>> handle_ioreq.
> > >>>>
> > >> Ok.
> > >>
> > >>     -Don Slutz
> > >>
> > >>>>> +        } else {
> > >>>>> +            handle_ioreq(req);
> > >>>>> +        }
> > >>>>> +#else
> > >>>>>            handle_ioreq(req);
> > >>>>> +#endif
> >

Re: [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Paul Durrant]

> -----Original Message-----
> From: qemu-devel-bounces+paul.durrant=citrix.com@nongnu.org
> [mailto:qemu-devel-bounces+paul.durrant=citrix.com@nongnu.org] On
> Behalf Of Stefano Stabellini
> Sent: 01 October 2014 10:20
> To: Slutz, Donald Christopher
> Cc: xen-devel@lists.xensource.com; Stefano Stabellini; Markus Armbruster;
> Marcel Apfelbaum; Alexander Graf; qemu-devel@nongnu.org; Michael S.
> Tsirkin; Anthony Liguori; Andreas Färber
> Subject: Re: [Qemu-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen
> access to vmport
> 
> On Wed, 1 Oct 2014, Slutz, Donald Christopher wrote:
> > On 09/30/14 06:35, Stefano Stabellini wrote:
> > > 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.
> > >>>>>
> > >>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> > >>>> [...]
> > >>>>
> > >>>>> diff --git a/xen-hvm.c b/xen-hvm.c
> > >>>>> index 05e522c..e1274bb 100644
> > >>>>> --- a/xen-hvm.c
> > >>>>> +++ b/xen-hvm.c
> > >>>>> @@ -857,14 +857,48 @@ static void cpu_handle_ioreq(void
> *opaque)
> > >>>>>          handle_buffered_iopage(state);
> > >>>>>        if (req) {
> > >>>>> +#ifdef IOREQ_TYPE_VMWARE_PORT
> > >>>> Is there any reason to #ifdef this code?
> > >>>> Couldn't we just always build it in QEMU?
> > >> This allows QEMU 2.2 (or later) to build on a system that has Xen 4.5
> > >> or earlier installed; and work.
> > > I would rather remove the #ifdef from here and add any necessary
> > > compatibility stuff to include/hw/xen/xen_common.h.
> > >
> >
> > Ok, will do.
> >
> > >>>>> +        if (req->type == IOREQ_TYPE_VMWARE_PORT) {
> > >>>> I think it would make more sense to check for
> IOREQ_TYPE_VMWARE_PORT
> > >>>> from within handle_ioreq.
> > >>>>
> > >> Ok, I can move it.
> > >>
> > >>
> > >>>>> +            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 :-)
> >
> > None taken.
> >
> > > 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.
> > >
> >
> > I can, it is just slower.  This would require 2 new types.  1 for regs to
> > QEMU, 1 for regs from QEMU.  So instead of 1 round trip (Xen to QEMU
> > to Xen), you now have 3 (Xen to QEMU (regs to QEMU) to Xen, Xen to
> > QEMU (PIO) to Xen, Xen to QEMU (regs from QEMU) to Xen).
> 
> This is not an high performance device, is it?
> 
> In any case I agree it would be better to avoid it.
> I wonder if we could send both ioreqs at once from Xen and back from
> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT,
> changing
> the size of ioreq_t only for this ioreq type.
> Another maybe simpler possibility would be introducing a union in
> ioreq_t with the registers.
> Anything would be OK for me but I would like to see the registers being
> passes explicitely by Xen rather than "hiding" them into other ioreq
> fields.
> 

Changing the size of ioreq_t would be a bit of a nightmare: The structs are laid out in an array indexed by vcpu and shared by QEMU and Xen.

  Paul

> 
> > >>>    If any case you should spend a
> > >>> few more words on why you are doing this.
> > >>>
> > >> Sure.  Will add some form of the above as part of the commit message.
> > >>
> > >>>>> +            if (!xen_opaque_env) {
> > >>>>> +                xen_opaque_env = g_malloc(sizeof(CPUX86State));
> > >>>>> +            }
> > >>>>> +            env = xen_opaque_env;
> > >>>>> +            env->regs[R_EAX] = (uint32_t)(req->addr >> 32);
> > >>>>> +            env->regs[R_EBX] = (uint32_t)(req->addr);
> > >>>>> +            env->regs[R_ECX] = req->count;
> > >>>>> +            env->regs[R_EDX] = req->size;
> > >>>>> +            env->regs[R_ESI] = (uint32_t)(req->data >> 32);
> > >>>>> +            env->regs[R_EDI] = (uint32_t)(req->data);
> > >>>>> +            handle_ioreq(&fake_req);
> > >>>>> +            req->addr = ((uint64_t)fake_req.data << 32) |
> > >>>>> +                (uint32_t)env->regs[R_EBX];
> > >>>>> +            req->count = env->regs[R_ECX];
> > >>>>> +            req->size = env->regs[R_EDX];
> > >>>>> +            req->data = ((uint64_t)env->regs[R_ESI] << 32) |
> > >>>>> +                (uint32_t)env->regs[R_EDI];
> > >>>> This code could be moved to a separate helper function called by
> > >>>> handle_ioreq.
> > >>>>
> > >> Ok.
> > >>
> > >>     -Don Slutz
> > >>
> > >>>>> +        } else {
> > >>>>> +            handle_ioreq(req);
> > >>>>> +        }
> > >>>>> +#else
> > >>>>>            handle_ioreq(req);
> > >>>>> +#endif
> >

Re: [Qemu-devel] [Xen-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Anthony Liguori]

On Wed, Oct 1, 2014 at 7:44 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-10-01 at 10:20 +0100, Stefano Stabellini wrote:
>> I wonder if we could send both ioreqs at once from Xen and back from
>> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
>> the size of ioreq_t only for this ioreq type.
>
> Random idea: Why new add a IOREQ_TYPE_FULL_STATE which would be issued
> for these ports and let qemu decode the fact that it is vmware
> internally? That might be a more generically useful interface in the
> future?
>
> WRT to fitting all the register state in the current sized request, you
> could declare that this new thing takes multiple slots.
>
> Also, I may be wrong, but I thought most IOREQs were synchronous so only
> one slot was ever used? The buffered ioreq stuff has a separate ring (or
> uses a different part of the page, or something). I might be talking
> nonsense here though ;-)

There really isn't a concept of "CPU associated with an IOREQ" outside
of two very special cases, LAPIC emulation and vmport.  LAPIC
emulation really belongs closer to the CPU and given V-APIC, it's
gotten moved into hardware anyway.  vmport is just a hack VMware made.

I think it's better to think of it as a VMware specific hypercall and
terminate the IOREQ within the hypervisor.  Passing a decoded version
of the request to QEMU is fine but passing the full CPU state as part
of an IOREQ_TYPE_FULL_STATE is not very useful.  It's just an
IOREQ_TYPE_VMPORT with more information than is needed.

Regards,

Anthony Liguori

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

Re: [Xen-devel] [PATCH 1/1] xen-hvm.c: Add support for Xen access to vmport

[Anthony Liguori]

On Wed, Oct 1, 2014 at 7:44 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-10-01 at 10:20 +0100, Stefano Stabellini wrote:
>> I wonder if we could send both ioreqs at once from Xen and back from
>> QEMU. Or maybe append the registers to IOREQ_TYPE_VMWARE_PORT, changing
>> the size of ioreq_t only for this ioreq type.
>
> Random idea: Why new add a IOREQ_TYPE_FULL_STATE which would be issued
> for these ports and let qemu decode the fact that it is vmware
> internally? That might be a more generically useful interface in the
> future?
>
> WRT to fitting all the register state in the current sized request, you
> could declare that this new thing takes multiple slots.
>
> Also, I may be wrong, but I thought most IOREQs were synchronous so only
> one slot was ever used? The buffered ioreq stuff has a separate ring (or
> uses a different part of the page, or something). I might be talking
> nonsense here though ;-)

There really isn't a concept of "CPU associated with an IOREQ" outside
of two very special cases, LAPIC emulation and vmport.  LAPIC
emulation really belongs closer to the CPU and given V-APIC, it's
gotten moved into hardware anyway.  vmport is just a hack VMware made.

I think it's better to think of it as a VMware specific hypercall and
terminate the IOREQ within the hypervisor.  Passing a decoded version
of the request to QEMU is fine but passing the full CPU state as part
of an IOREQ_TYPE_FULL_STATE is not very useful.  It's just an
IOREQ_TYPE_VMPORT with more information than is needed.

Regards,

Anthony Liguori

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