[Qemu-devel] [HACK] make vmmouse work with KVM

[Qemu-devel] [HACK] make vmmouse work with KVM

[Reimar Döffinger]

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

Hello,
vmmouse uses a giant hack: it uses io ports (in instruction) but passes
data via registers.
This currently does not work since the qemu CPU registers are
(understandably) not kept in sync with the real KVM registers for this
operation.
Attached patch detects access to the vmmouse port and loads/stores CPU
registers into/from the QEMU state.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

[-- Attachment #2: kvm_vmmouse.diff --]
[-- Type: text/plain, Size: 1003 bytes --]

diff --git a/kvm-all.c b/kvm-all.c
index f669c3a..207378b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -611,11 +611,21 @@ int kvm_cpu_exec(CPUState *env)
         switch (run->exit_reason) {
         case KVM_EXIT_IO:
             dprintf("handle_io\n");
+#if defined(TARGET_I386) || defined(TARGET_X86_64)
+            // HACK to make vmport/vmmouse work
+            if (run->io.port == 0x5658)
+                kvm_arch_get_registers(env);
+#endif
             ret = kvm_handle_io(env, run->io.port,
                                 (uint8_t *)run + run->io.data_offset,
                                 run->io.direction,
                                 run->io.size,
                                 run->io.count);
+#if defined(TARGET_I386) || defined(TARGET_X86_64)
+            // HACK to make vmport/vmmouse work
+            if (run->io.port == 0x5658)
+                kvm_arch_put_registers(env);
+#endif
             break;
         case KVM_EXIT_MMIO:
             dprintf("handle_mmio\n");

Re: [Qemu-devel] [HACK] make vmmouse work with KVM

[Anthony Liguori]

Reimar Döffinger wrote:
> Hello,
> vmmouse uses a giant hack: it uses io ports (in instruction) but passes
> data via registers.
> This currently does not work since the qemu CPU registers are
> (understandably) not kept in sync with the real KVM registers for this
> operation.
> Attached patch detects access to the vmmouse port and loads/stores CPU
> registers into/from the QEMU state.
>   

Should use cpu_synchronize_state() in vmport.c

> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
>   

Regards,

Anthony Liguori

Re: [Qemu-devel] [HACK] make vmmouse work with KVM

[Reimar Döffinger]

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

On Mon, Aug 17, 2009 at 04:47:54PM +0200, Reimar Döffinger wrote:
> Hello,
> vmmouse uses a giant hack: it uses io ports (in instruction) but passes
> data via registers.
> This currently does not work since the qemu CPU registers are
> (understandably) not kept in sync with the real KVM registers for this
> operation.
> Attached patch detects access to the vmmouse port and loads/stores CPU
> registers into/from the QEMU state.
> 
> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

Slightly simplified since TARGET_X86_64 implies TARGET_I386.

[-- Attachment #2: kvm_vmmouse.diff --]
[-- Type: text/plain, Size: 939 bytes --]

diff --git a/kvm-all.c b/kvm-all.c
index f669c3a..639bd77 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -611,11 +611,21 @@ int kvm_cpu_exec(CPUState *env)
         switch (run->exit_reason) {
         case KVM_EXIT_IO:
             dprintf("handle_io\n");
+#ifdef TARGET_I386
+            // HACK to make vmport/vmmouse work
+            if (run->io.port == 0x5658)
+                kvm_arch_get_registers(env);
+#endif
             ret = kvm_handle_io(env, run->io.port,
                                 (uint8_t *)run + run->io.data_offset,
                                 run->io.direction,
                                 run->io.size,
                                 run->io.count);
+#ifdef TARGET_I386
+            // HACK to make vmport/vmmouse work
+            if (run->io.port == 0x5658)
+                kvm_arch_put_registers(env);
+#endif
             break;
         case KVM_EXIT_MMIO:
             dprintf("handle_mmio\n");

Re: [Qemu-devel] [HACK] make vmmouse work with KVM

[Reimar Döffinger]

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

On Mon, Aug 17, 2009 at 10:11:11AM -0500, Anthony Liguori wrote:
> Reimar Döffinger wrote:
> > Hello,
> > vmmouse uses a giant hack: it uses io ports (in instruction) but passes
> > data via registers.
> > This currently does not work since the qemu CPU registers are
> > (understandably) not kept in sync with the real KVM registers for this
> > operation.
> > Attached patch detects access to the vmmouse port and loads/stores CPU
> > registers into/from the QEMU state.
> >   
> 
> Should use cpu_synchronize_state() in vmport.c

Ah, missed that function...
Does attached patch look good?

[-- Attachment #2: kvm_vmmouse.diff --]
[-- Type: text/plain, Size: 1125 bytes --]

diff --git a/hw/vmport.c b/hw/vmport.c
index 884af3f..9dc94a3 100644
--- a/hw/vmport.c
+++ b/hw/vmport.c
@@ -25,6 +25,7 @@
 #include "isa.h"
 #include "pc.h"
 #include "sysemu.h"
+#include "kvm.h"
 
 //#define VMPORT_DEBUG
 
@@ -57,6 +58,9 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
     CPUState *env = cpu_single_env;
     unsigned char command;
     uint32_t eax;
+    uint32_t result;
+
+    cpu_synchronize_state(env, 0);
 
     eax = env->regs[R_EAX];
     if (eax != VMPORT_MAGIC)
@@ -73,14 +77,19 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
         return eax;
     }
 
-    return s->func[command](s->opaque[command], addr);
+    result = s->func[command](s->opaque[command], addr);
+    cpu_synchronize_state(env, 1);
+
+    return result;
 }
 
 static void vmport_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
     CPUState *env = cpu_single_env;
 
+    cpu_synchronize_state(env, 0);
     env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
+    cpu_synchronize_state(env, 1);
 }
 
 static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)

[Qemu-devel] Re: [HACK] make vmmouse work with KVM

[Paolo Bonzini]

On 08/17/2009 05:45 PM, Reimar Döffinger wrote:
> +    cpu_synchronize_state(env, 0);
>       env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
> +    cpu_synchronize_state(env, 1);

This is not needed because the sync is done in vmport_ioport_read, isn't it?

Paolo

[Qemu-devel] Re: [HACK] make vmmouse work with KVM

[Reimar Döffinger]

On Mon, Aug 17, 2009 at 06:44:11PM +0200, Paolo Bonzini wrote:
> On 08/17/2009 05:45 PM, Reimar Döffinger wrote:
> > +    cpu_synchronize_state(env, 0);
> >       env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
> > +    cpu_synchronize_state(env, 1);
> 
> This is not needed because the sync is done in vmport_ioport_read, isn't it?

Well... The cpu_synchronize_state could be dropped you are right, but
here we write R_EAX so the cpu_synchronize_state(env, 1) is necessary.
Want me to remove the cpu_synchronize_state(env, 0)?
It all seems a bit messy, because despite the "synchronize" name of the
function any change to the registers before the call to
vmport_ioport_read would be overwritten by the cpu_synchronize_state in
there.
It might be slightly cleaner to rename the vmport_ioport_read (any name
suggestions?) and add a wrapper for register_ioport_read that does the
cpu_synchronize_state (so it looks similar to vmport_ioport_write).

Greetings,
Reimar Döffinger

[Qemu-devel] Re: [HACK] make vmmouse work with KVM

[Paolo Bonzini]

On 08/17/2009 07:00 PM, Reimar Döffinger wrote:
> On Mon, Aug 17, 2009 at 06:44:11PM +0200, Paolo Bonzini wrote:
>> On 08/17/2009 05:45 PM, Reimar Döffinger wrote:
>>> +    cpu_synchronize_state(env, 0);
>>>        env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
>>> +    cpu_synchronize_state(env, 1);
>>
>> This is not needed because the sync is done in vmport_ioport_read, isn't it?
>
> Well... The cpu_synchronize_state could be dropped you are right, but
> here we write R_EAX so the cpu_synchronize_state(env, 1) is necessary.
> It might be slightly cleaner to rename the vmport_ioport_read (any name
> suggestions?) and add a wrapper for register_ioport_read that does the
> cpu_synchronize_state (so it looks similar to vmport_ioport_write).

Yes, that would look best and wouldn't have fooled me. Could 
vmport_ioport_trigger be a decent name?

Paolo

Paolo

[Qemu-devel] Re: [HACK] make vmmouse work with KVM

[Reimar Döffinger]

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

On Mon, Aug 17, 2009 at 07:16:49PM +0200, Paolo Bonzini wrote:
> On 08/17/2009 07:00 PM, Reimar Döffinger wrote:
> > On Mon, Aug 17, 2009 at 06:44:11PM +0200, Paolo Bonzini wrote:
> >> On 08/17/2009 05:45 PM, Reimar Döffinger wrote:
> >>> +    cpu_synchronize_state(env, 0);
> >>>        env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
> >>> +    cpu_synchronize_state(env, 1);
> >>
> >> This is not needed because the sync is done in vmport_ioport_read, isn't it?
> >
> > Well... The cpu_synchronize_state could be dropped you are right, but
> > here we write R_EAX so the cpu_synchronize_state(env, 1) is necessary.
> > It might be slightly cleaner to rename the vmport_ioport_read (any name
> > suggestions?) and add a wrapper for register_ioport_read that does the
> > cpu_synchronize_state (so it looks similar to vmport_ioport_write).
> 
> Yes, that would look best and wouldn't have fooled me. Could 
> vmport_ioport_trigger be a decent name?

Since what that function actually does is execute a specific command it
gets from ecx I went with vmport_ioport_command, but I'll happily leave
the naming to whoever applies it :-)

Greetings,
Reimar Döffinger

[-- Attachment #2: kvm_vmmouse2.diff --]
[-- Type: text/plain, Size: 1378 bytes --]

diff --git a/hw/vmport.c b/hw/vmport.c
index 884af3f..7ba6e2e 100644
--- a/hw/vmport.c
+++ b/hw/vmport.c
@@ -25,6 +25,7 @@
 #include "isa.h"
 #include "pc.h"
 #include "sysemu.h"
+#include "kvm.h"
 
 //#define VMPORT_DEBUG
 
@@ -51,7 +52,7 @@ void vmport_register(unsigned char command, IOPortReadFunc *func, void *opaque)
     port_state.opaque[command] = opaque;
 }
 
-static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
+static uint32_t vmport_ioport_command(void *opaque, uint32_t addr)
 {
     VMPortState *s = opaque;
     CPUState *env = cpu_single_env;
@@ -76,11 +77,24 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
     return s->func[command](s->opaque[command], addr);
 }
 
+static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
+{
+    CPUState *env = cpu_single_env;
+    uint32_t result;
+
+    cpu_synchronize_state(env, 0);
+    result = vmport_ioport_command(opaque, addr);
+    cpu_synchronize_state(env, 1);
+    return result;
+}
+
 static void vmport_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
     CPUState *env = cpu_single_env;
 
-    env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
+    cpu_synchronize_state(env, 0);
+    env->regs[R_EAX] = vmport_ioport_command(opaque, addr);
+    cpu_synchronize_state(env, 1);
 }
 
 static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)

Re: [Qemu-devel] Re: [HACK] make vmmouse work with KVM

[Reimar Döffinger]

On Mon, Aug 17, 2009 at 07:32:32PM +0200, Reimar Döffinger wrote:
> On Mon, Aug 17, 2009 at 07:16:49PM +0200, Paolo Bonzini wrote:
> > On 08/17/2009 07:00 PM, Reimar Döffinger wrote:
> > > On Mon, Aug 17, 2009 at 06:44:11PM +0200, Paolo Bonzini wrote:
> > >> On 08/17/2009 05:45 PM, Reimar Döffinger wrote:
> > >>> +    cpu_synchronize_state(env, 0);
> > >>>        env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
> > >>> +    cpu_synchronize_state(env, 1);
> > >>
> > >> This is not needed because the sync is done in vmport_ioport_read, isn't it?
> > >
> > > Well... The cpu_synchronize_state could be dropped you are right, but
> > > here we write R_EAX so the cpu_synchronize_state(env, 1) is necessary.
> > > It might be slightly cleaner to rename the vmport_ioport_read (any name
> > > suggestions?) and add a wrapper for register_ioport_read that does the
> > > cpu_synchronize_state (so it looks similar to vmport_ioport_write).
> > 
> > Yes, that would look best and wouldn't have fooled me. Could 
> > vmport_ioport_trigger be a decent name?
> 
> Since what that function actually does is execute a specific command it
> gets from ecx I went with vmport_ioport_command, but I'll happily leave
> the naming to whoever applies it :-)

Simpler version due to new cpu_synchronize_state behaviour.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

---
diff --git a/hw/vmport.c b/hw/vmport.c
index 884af3f..9a942ee 100644
--- a/hw/vmport.c
+++ b/hw/vmport.c
@@ -25,6 +25,7 @@
 #include "isa.h"
 #include "pc.h"
 #include "sysemu.h"
+#include "kvm.h"
 
 //#define VMPORT_DEBUG
 
@@ -58,6 +59,7 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
     unsigned char command;
     uint32_t eax;
 
+    cpu_synchronize_state(env);
     eax = env->regs[R_EAX];
     if (eax != VMPORT_MAGIC)
         return eax;

Re: [Qemu-devel] Re: [HACK] make vmmouse work with KVM

[Anthony Liguori]

Reimar Döffinger wrote:
> On Mon, Aug 17, 2009 at 07:32:32PM +0200, Reimar Döffinger wrote:
>   
>> On Mon, Aug 17, 2009 at 07:16:49PM +0200, Paolo Bonzini wrote:
>>     
>>> On 08/17/2009 07:00 PM, Reimar Döffinger wrote:
>>>       
>>>> On Mon, Aug 17, 2009 at 06:44:11PM +0200, Paolo Bonzini wrote:
>>>>         
>>>>> On 08/17/2009 05:45 PM, Reimar Döffinger wrote:
>>>>>           
>>>>>> +    cpu_synchronize_state(env, 0);
>>>>>>        env->regs[R_EAX] = vmport_ioport_read(opaque, addr);
>>>>>> +    cpu_synchronize_state(env, 1);
>>>>>>             
>>>>> This is not needed because the sync is done in vmport_ioport_read, isn't it?
>>>>>           
>>>> Well... The cpu_synchronize_state could be dropped you are right, but
>>>> here we write R_EAX so the cpu_synchronize_state(env, 1) is necessary.
>>>> It might be slightly cleaner to rename the vmport_ioport_read (any name
>>>> suggestions?) and add a wrapper for register_ioport_read that does the
>>>> cpu_synchronize_state (so it looks similar to vmport_ioport_write).
>>>>         
>>> Yes, that would look best and wouldn't have fooled me. Could 
>>> vmport_ioport_trigger be a decent name?
>>>       
>> Since what that function actually does is execute a specific command it
>> gets from ecx I went with vmport_ioport_command, but I'll happily leave
>> the naming to whoever applies it :-)
>>     
>
> Simpler version due to new cpu_synchronize_state behaviour.
>   

Please top post patches.

Regards,

Anthony Liguori