diff mbox

[HACK] make vmmouse work with KVM

Message ID 20090817154557.GB365@1und1.de
State Superseded
Headers show

Commit Message

Reimar Döffinger Aug. 17, 2009, 3:45 p.m. UTC 
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?

Comments

Paolo Bonzini Aug. 17, 2009, 4:44 p.m. UTC | #1 
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
Reimar Döffinger Aug. 17, 2009, 5 p.m. UTC | #2 
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
Paolo Bonzini Aug. 17, 2009, 5:16 p.m. UTC | #3 
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
diff mbox

Patch

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)