[Qemu-devel] [PATCH 0/4] Presenting accelerators for better abstraction

[Qemu-devel] [PATCH 0/4] Presenting accelerators for better abstraction

[Glauber Costa]

Hey guys,

This was already discussed, mostly on kvm-devel (but qemu list always copied, if you remember),
and here is the pre work I've done. I don't feel it to be mergeable yet, but with your
comments, I expect to get it in a good shape soon. This is not extensibly tested (but it will).

These patches applies ontop of raw qemu, so they are not kvm-specific _at all_. I've choosen to start
by abstracting kqemu first, since this is what we have already in the tree. 
The abstraction of kqemu is also, not complete. It cover mostly areas that are common to kvm, i.e., provide
functions that kvm could implement as-is.

Comments are welcome, rants not so.
Have fun.

[Qemu-devel] [PATCH 1/4] [PATCH] introduce QEMUAccel and fill it with interrupt specific driver

[Glauber Costa]

This patch introduces QEMUAccel, a placeholder for function pointers
that aims at helping qemu to abstract accelerators such as kqemu and
kvm (actually, the 'accelerator' name was proposed by avi kivity, since
he loves referring to kvm that way).

To begin with, the accelerator is given the opportunity to register a
cpu_interrupt function, to be called after the raw cpu_interrupt.
This has the side effect of, for the kqemu accelerator, calling kqemu_cpu_interrupt
everytime, which didn't use to happen. But looking at the code, this seems safe to me.

This patch applies on raw qemu.

Signed-off-by: Glauber Costa <gcosta@redhat.com>
---
 block-raw-posix.c |    5 -----
 exec-all.h        |   18 +++++++++++++++++-
 exec.c            |    2 ++
 kqemu.c           |   26 ++++++++++++++++----------
 vl.c              |    6 +-----
 5 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 6b0009e..61c23ba 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -250,11 +250,6 @@ static void aio_signal_handler(int signum)
     if (env) {
         /* stop the currently executing cpu because a timer occured */
         cpu_interrupt(env, CPU_INTERRUPT_EXIT);
-#ifdef USE_KQEMU
-        if (env->kqemu_enabled) {
-            kqemu_cpu_interrupt(env);
-        }
-#endif
     }
 #endif
 }
diff --git a/exec-all.h b/exec-all.h
index 3ce8242..5162307 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -574,6 +574,23 @@ static inline target_ulong get_phys_addr_code(CPUState *env, target_ulong addr)
 }
 #endif
 
+typedef struct QEMUAccel {
+    void (*cpu_interrupt)(CPUState *env);
+} QEMUAccel;
+
+extern QEMUAccel *current_accel;
+
+static inline void register_qemu_accel(QEMUAccel *accel)
+{
+    current_accel = accel;
+}
+
+static inline void accel_cpu_interrupt(CPUState *env)
+{
+    if (current_accel && current_accel->cpu_interrupt)
+        current_accel->cpu_interrupt(env);
+}
+
 #ifdef USE_KQEMU
 #define KQEMU_MODIFY_PAGE_MASK (0xff & ~(VGA_DIRTY_FLAG | CODE_DIRTY_FLAG))
 
@@ -583,7 +600,6 @@ void kqemu_flush_page(CPUState *env, target_ulong addr);
 void kqemu_flush(CPUState *env, int global);
 void kqemu_set_notdirty(CPUState *env, ram_addr_t ram_addr);
 void kqemu_modify_page(CPUState *env, ram_addr_t ram_addr);
-void kqemu_cpu_interrupt(CPUState *env);
 void kqemu_record_dump(void);
 
 static inline int kqemu_is_ok(CPUState *env)
diff --git a/exec.c b/exec.c
index 48dabd6..93d9b01 100644
--- a/exec.c
+++ b/exec.c
@@ -1226,6 +1226,8 @@ void cpu_interrupt(CPUState *env, int mask)
         tb_reset_jump_recursive(tb);
         resetlock(&interrupt_lock);
     }
+
+    accel_cpu_interrupt(env);
 }
 
 void cpu_reset_interrupt(CPUState *env, int mask)
diff --git a/kqemu.c b/kqemu.c
index 148a52f..c46698c 100644
--- a/kqemu.c
+++ b/kqemu.c
@@ -158,6 +158,19 @@ static void kqemu_update_cpuid(CPUState *env)
        accelerated code */
 }
 
+void kqemu_cpu_interrupt(CPUState *env)
+{
+#if defined(_WIN32) && KQEMU_VERSION >= 0x010101
+    /* cancelling the I/O request causes KQEMU to finish executing the
+       current block and successfully returning. */
+    CancelIo(kqemu_fd);
+#endif
+}
+
+QEMUAccel kqemu_accel = {
+    .cpu_interrupt = kqemu_cpu_interrupt,
+};
+
 int kqemu_init(CPUState *env)
 {
     struct kqemu_init init;
@@ -239,6 +252,9 @@ int kqemu_init(CPUState *env)
     }
     kqemu_update_cpuid(env);
     env->kqemu_enabled = kqemu_allowed;
+    if (env->kqemu_enabled)
+        register_qemu_accel(&kqemu_accel);
+
     nb_pages_to_flush = 0;
     nb_ram_pages_to_update = 0;
     return 0;
@@ -901,14 +917,4 @@ int kqemu_cpu_exec(CPUState *env)
     }
     return 0;
 }
-
-void kqemu_cpu_interrupt(CPUState *env)
-{
-#if defined(_WIN32) && KQEMU_VERSION >= 0x010101
-    /* cancelling the I/O request causes KQEMU to finish executing the
-       current block and successfully returning. */
-    CancelIo(kqemu_fd);
-#endif
-}
-
 #endif
diff --git a/vl.c b/vl.c
index 9289982..9272541 100644
--- a/vl.c
+++ b/vl.c
@@ -240,6 +240,7 @@ struct drive_opt {
 static CPUState *cur_cpu;
 static CPUState *next_cpu;
 static int event_pending = 1;
+QEMUAccel *current_accel;
 
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
 
@@ -1200,11 +1201,6 @@ static void host_alarm_handler(int host_signum)
         if (env) {
             /* stop the currently executing cpu because a timer occured */
             cpu_interrupt(env, CPU_INTERRUPT_EXIT);
-#ifdef USE_KQEMU
-            if (env->kqemu_enabled) {
-                kqemu_cpu_interrupt(env);
-            }
-#endif
         }
         event_pending = 1;
     }
-- 
1.5.0.6

[Qemu-devel] [PATCH 2/4] [PATCH] exec interrupt made abstract by accel_yyy()

[Glauber Costa]

---
 cpu-exec.c |   29 ++++++++++++++++++-----------
 exec-all.h |   10 ++++++++++
 kqemu.c    |    3 +++
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 3246264..79da619 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -35,6 +35,22 @@
 #include <sys/ucontext.h>
 #endif
 
+/* FIXME: This does not belong here */
+int kqemu_exec_interrupt(CPUState *env)
+{
+    int ret = 0;
+    if (kqemu_is_ok(env) && env->interrupt_request == 0) {
+        env->eflags = env->eflags | cc_table[CC_OP].compute_all() | (DF & DF_MASK);
+        ret = kqemu_cpu_exec(env);
+        /* put eflags in CPU temporary format */
+        CC_SRC = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
+        DF = 1 - (2 * ((env->eflags >> 10) & 1));
+        CC_OP = CC_OP_EFLAGS;
+        env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
+    }
+    return ret;
+}
+
 int tb_invalidated_flag;
 
 //#define DEBUG_EXEC
@@ -388,16 +404,8 @@ int cpu_exec(CPUState *env1)
                 }
                 env->exception_index = -1;
             }
-#ifdef USE_KQEMU
-            if (kqemu_is_ok(env) && env->interrupt_request == 0) {
-                int ret;
-                env->eflags = env->eflags | cc_table[CC_OP].compute_all() | (DF & DF_MASK);
-                ret = kqemu_cpu_exec(env);
-                /* put eflags in CPU temporary format */
-                CC_SRC = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
-                DF = 1 - (2 * ((env->eflags >> 10) & 1));
-                CC_OP = CC_OP_EFLAGS;
-                env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
+
+            if (accel_exec_interrupt(env, &ret)) {
                 if (ret == 1) {
                     /* exception */
                     longjmp(env->jmp_env, 1);
@@ -412,7 +420,6 @@ int cpu_exec(CPUState *env1)
                     }
                 }
             }
-#endif
 
             T0 = 0; /* force lookup of first TB */
             for(;;) {
diff --git a/exec-all.h b/exec-all.h
index 5162307..621e1ca 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -576,6 +576,7 @@ static inline target_ulong get_phys_addr_code(CPUState *env, target_ulong addr)
 
 typedef struct QEMUAccel {
     void (*cpu_interrupt)(CPUState *env);
+    int (*exec_interrupt)(CPUState *env);
 } QEMUAccel;
 
 extern QEMUAccel *current_accel;
@@ -591,6 +592,15 @@ static inline void accel_cpu_interrupt(CPUState *env)
         current_accel->cpu_interrupt(env);
 }
 
+static inline int accel_exec_interrupt(CPUState *env, int *ret)
+{
+    if (current_accel && current_accel->exec_interrupt) {
+        *ret = current_accel->exec_interrupt(env);
+        return 1;
+    }
+    return 0;
+}
+
 #ifdef USE_KQEMU
 #define KQEMU_MODIFY_PAGE_MASK (0xff & ~(VGA_DIRTY_FLAG | CODE_DIRTY_FLAG))
 
diff --git a/kqemu.c b/kqemu.c
index c46698c..08622ad 100644
--- a/kqemu.c
+++ b/kqemu.c
@@ -167,8 +167,11 @@ void kqemu_cpu_interrupt(CPUState *env)
 #endif
 }
 
+extern int kqemu_exec_interrupt(CPUState *env);
+
 QEMUAccel kqemu_accel = {
     .cpu_interrupt = kqemu_cpu_interrupt,
+    .exec_interrupt = kqemu_exec_interrupt,
 };
 
 int kqemu_init(CPUState *env)
-- 
1.5.0.6

[Qemu-devel] [PATCH 3/4] [PATCH] init env made accel driver

[Glauber Costa]

---
 exec-all.h            |    8 +++++++-
 kqemu.c               |    1 +
 target-i386/helper2.c |    4 +---
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index 621e1ca..784d0ac 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -577,6 +577,7 @@ static inline target_ulong get_phys_addr_code(CPUState *env, target_ulong addr)
 typedef struct QEMUAccel {
     void (*cpu_interrupt)(CPUState *env);
     int (*exec_interrupt)(CPUState *env);
+    void (*init_env)(CPUState *env);
 } QEMUAccel;
 
 extern QEMUAccel *current_accel;
@@ -601,10 +602,15 @@ static inline int accel_exec_interrupt(CPUState *env, int *ret)
     return 0;
 }
 
+static inline void accel_init_env(CPUState *env)
+{
+    if (current_accel && current_accel->init_env)
+        current_accel->init_env(env);
+}
+
 #ifdef USE_KQEMU
 #define KQEMU_MODIFY_PAGE_MASK (0xff & ~(VGA_DIRTY_FLAG | CODE_DIRTY_FLAG))
 
-int kqemu_init(CPUState *env);
 int kqemu_cpu_exec(CPUState *env);
 void kqemu_flush_page(CPUState *env, target_ulong addr);
 void kqemu_flush(CPUState *env, int global);
diff --git a/kqemu.c b/kqemu.c
index 08622ad..95d8a94 100644
--- a/kqemu.c
+++ b/kqemu.c
@@ -172,6 +172,7 @@ extern int kqemu_exec_interrupt(CPUState *env);
 QEMUAccel kqemu_accel = {
     .cpu_interrupt = kqemu_cpu_interrupt,
     .exec_interrupt = kqemu_exec_interrupt,
+    .init_env = kqemu_init,
 };
 
 int kqemu_init(CPUState *env)
diff --git a/target-i386/helper2.c b/target-i386/helper2.c
index 551a0d8..b11bc22 100644
--- a/target-i386/helper2.c
+++ b/target-i386/helper2.c
@@ -111,9 +111,7 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
     cpu_reset(env);
-#ifdef USE_KQEMU
-    kqemu_init(env);
-#endif
+    accel_init_env(env);
     return env;
 }
 
-- 
1.5.0.6

[Qemu-devel] [PATCH 4/4] [PATCH] wrap cache flushing functions into accel drivers

[Glauber Costa]

---
 exec-all.h |   16 ++++++++++++++--
 exec.c     |   12 ++----------
 kqemu.c    |    2 ++
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index 784d0ac..9925a80 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -578,6 +578,8 @@ typedef struct QEMUAccel {
     void (*cpu_interrupt)(CPUState *env);
     int (*exec_interrupt)(CPUState *env);
     void (*init_env)(CPUState *env);
+    void (*flush_cache)(CPUState *env, int global);
+    void (*flush_page)(CPUState *env, target_ulong addr);
 } QEMUAccel;
 
 extern QEMUAccel *current_accel;
@@ -608,12 +610,22 @@ static inline void accel_init_env(CPUState *env)
         current_accel->init_env(env);
 }
 
+static inline void accel_flush_cache(CPUState *env, int global)
+{
+    if (current_accel && current_accel->flush_cache)
+        current_accel->flush_cache(env, global);
+}
+
+static inline void accel_flush_page(CPUState *env, target_ulong addr)
+{
+    if (current_accel && current_accel->flush_page)
+        current_accel->flush_page(env, addr);
+}
+
 #ifdef USE_KQEMU
 #define KQEMU_MODIFY_PAGE_MASK (0xff & ~(VGA_DIRTY_FLAG | CODE_DIRTY_FLAG))
 
 int kqemu_cpu_exec(CPUState *env);
-void kqemu_flush_page(CPUState *env, target_ulong addr);
-void kqemu_flush(CPUState *env, int global);
 void kqemu_set_notdirty(CPUState *env, ram_addr_t ram_addr);
 void kqemu_modify_page(CPUState *env, ram_addr_t ram_addr);
 void kqemu_record_dump(void);
diff --git a/exec.c b/exec.c
index 93d9b01..b0f50e3 100644
--- a/exec.c
+++ b/exec.c
@@ -1393,11 +1393,7 @@ void tlb_flush(CPUState *env, int flush_global)
 #if !defined(CONFIG_SOFTMMU)
     munmap((void *)MMAP_AREA_START, MMAP_AREA_END - MMAP_AREA_START);
 #endif
-#ifdef USE_KQEMU
-    if (env->kqemu_enabled) {
-        kqemu_flush(env, flush_global);
-    }
-#endif
+    accel_flush_cache(env, flush_global);
     tlb_flush_count++;
 }
 
@@ -1450,11 +1446,7 @@ void tlb_flush_page(CPUState *env, target_ulong addr)
     if (addr < MMAP_AREA_END)
         munmap((void *)addr, TARGET_PAGE_SIZE);
 #endif
-#ifdef USE_KQEMU
-    if (env->kqemu_enabled) {
-        kqemu_flush_page(env, addr);
-    }
-#endif
+    accel_flush_page(env, addr);
 }
 
 /* update the TLBs so that writes to code in the virtual page 'addr'
diff --git a/kqemu.c b/kqemu.c
index 95d8a94..f92b60d 100644
--- a/kqemu.c
+++ b/kqemu.c
@@ -173,6 +173,8 @@ QEMUAccel kqemu_accel = {
     .cpu_interrupt = kqemu_cpu_interrupt,
     .exec_interrupt = kqemu_exec_interrupt,
     .init_env = kqemu_init,
+    .flush_cache = kqemu_flush,
+    .flush_page = kqemu_flush_page,
 };
 
 int kqemu_init(CPUState *env)
-- 
1.5.0.6

Re: [Qemu-devel] [PATCH 1/4] [PATCH] introduce QEMUAccel and fill it with interrupt specific driver

[Jamie Lokier]

Glauber Costa wrote:
> This patch introduces QEMUAccel, a placeholder for function pointers
> that aims at helping qemu to abstract accelerators such as kqemu and
> kvm (actually, the 'accelerator' name was proposed by avi kivity, since
> he loves referring to kvm that way).

Just a little thought...

Maybe 'VCPU' would be a clearer name?  QEMU provides its own VCPU, and
KQEMU+QEMU also provide one toegether.  While KVM provides essentially
one or more whole VCPUs by itself and uses QEMU's drivers only doesn't
it?

-- Jamie

Re: [Qemu-devel] [PATCH 1/4] [PATCH] introduce QEMUAccel and fill it with interrupt specific driver

[Glauber Costa]

Jamie Lokier wrote:
> Glauber Costa wrote:
>> This patch introduces QEMUAccel, a placeholder for function pointers
>> that aims at helping qemu to abstract accelerators such as kqemu and
>> kvm (actually, the 'accelerator' name was proposed by avi kivity, since
>> he loves referring to kvm that way).
> 
> Just a little thought...
> 
> Maybe 'VCPU' would be a clearer name?  QEMU provides its own VCPU, and
> KQEMU+QEMU also provide one toegether.  While KVM provides essentially
> one or more whole VCPUs by itself and uses QEMU's drivers only doesn't
> it?
> 
> -- Jamie
VCPU is rather confusing with the vcpus themselves. KVM, for instance,
has its own structures called "vcpu".

If it is preferred, however, we can name the structure VCPUOperations,
and change the function names that involves accel_yyy to vcpu_op_yyy()

What do you think?

Re: [Qemu-devel] [PATCH 1/4] [PATCH] introduce QEMUAccel and fill it with interrupt specific driver

[Paul Brook]

> > Maybe 'VCPU' would be a clearer name?  QEMU provides its own VCPU, and
> > KQEMU+QEMU also provide one toegether.  While KVM provides essentially
> > one or more whole VCPUs by itself and uses QEMU's drivers only doesn't
> > it?
> >
> > -- Jamie
>
> VCPU is rather confusing with the vcpus themselves. KVM, for instance,
> has its own structures called "vcpu".
>
> If it is preferred, however, we can name the structure VCPUOperations,
> and change the function names that involves accel_yyy to vcpu_op_yyy()

kvm wants to hook into more than just the CPU doesn't it?

Paul

Re: [Qemu-devel] [PATCH 1/4] [PATCH] introduce QEMUAccel and fill it with interrupt specific driver

[Glauber Costa]

Paul Brook wrote:
>>> Maybe 'VCPU' would be a clearer name?  QEMU provides its own VCPU, and
>>> KQEMU+QEMU also provide one toegether.  While KVM provides essentially
>>> one or more whole VCPUs by itself and uses QEMU's drivers only doesn't
>>> it?
>>>
>>> -- Jamie
>> VCPU is rather confusing with the vcpus themselves. KVM, for instance,
>> has its own structures called "vcpu".
>>
>> If it is preferred, however, we can name the structure VCPUOperations,
>> and change the function names that involves accel_yyy to vcpu_op_yyy()
> 
> kvm wants to hook into more than just the CPU doesn't it?
But so far, the structure I proposed only touches cpu-related things.
We can have two structures for clarity. Like MEMOperations, and so far.
We do it this way for paravirt_ops in linux, with good results in code 
clarity.