Re: [patch] fold struct vcpu_info into CPUState

[Jes Sorensen <jes@sgi.com>]

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

Glauber Costa wrote:
>> @@ -93,7 +74,20 @@
>>
>>  CPUState *qemu_kvm_cpu_env(int index)
>>  {
[snip]
>>  }
> 
> This doesn't seem right. This function exists because we used to have
> a vcpu and and env structs, that were separated but
> should be tied together in some uses.
> Now, there's absolutely nothing in here that is not qemu-specific.
> This is just a function to return and env given a cpu number.
> You'll lose the current_env optimization that probably matters a lot
> in your case, but I'm afraid you will just have to live with that:
> it sucks for qemu too, and when it is fixed, it will be fixed for both
> (means getting rid of the ugly cpu_single_env)

Hi Glauber,

I see this function as a temporary transition type thing, it wasn't
my intention for it to stay permanently, but it's not really called from
a lot of performance critical places for now.

>>     if (env) {
>> -        if (!vcpu)
>> +        if (!current_env->vcpu_info.created)
>>             signal = 1;
> 
> !vcpu is probably meant to catch the case in witch the vcpu tls
> variable is not yet assigned. By dereferencing current_env here,
> you are probably doing an invalid access. So unless you can prove this
> is not an issue, should add another test.

Hmmm I think the test got lost because I moved this over in multiple
steps. It was a thread variable 'cpu_info' prior, but we have no
guarantee here that current_env is valid. Thanks.

>> -        if (vcpu && env != vcpu->env &&
>> !vcpu_info[env->cpu_index].signalled)
>> +        /*
>> +         * Testing for vcpu_info.created here is really redundant
>> +         */
>> +        if (current_env->vcpu_info.created &&
>> +            env != current_env && env->vcpu_info.signalled)
> 
> should be !env->vcpu_info.signalled instead?

Good catch!

>>  static void flush_queued_work(CPUState *env)
>>  {
>> -    struct vcpu_info *vi = &vcpu_info[env->cpu_index];
>> +    struct vcpu_info *vi = &env->vcpu_info;
>>     struct qemu_kvm_work_item *wi;
> 
> "vi" is not a good name, since emacs users will likely complain.
> vcpu_info is a much better name.

I'm an Emacs user! It's just a temporary variable, I really don't think
this matters. We could name it 'ed' just to be different, if you prefer.

Here's an updated version of that patch.

Cheers,
Jes

PS: I am gone through Wednesday, so should give you a few days to
     review :)

[-- Attachment #2: 0010-qemu-kvm-vcpu_info-v2.patch --]
[-- Type: text/plain, Size: 11375 bytes --]

Merge vcpu_info into CPUState.

Moves definition of vcpu related structs to new header qemu-kvm-vcpu.h
and declares this struct in i386/ia64/ppc CPUState structs if USE_KVM
is defined. In addition conver qemu-kvm.c to pull vcpu_info out of
CPUState.

This eliminates ugly static sized array of struct vcpu_info.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 qemu/qemu-kvm-vcpu.h   |   34 +++++++++++
 qemu/qemu-kvm.c        |  143 ++++++++++++++++++++++++++-----------------------
 qemu/target-i386/cpu.h |    4 +
 qemu/target-ia64/cpu.h |    5 +
 qemu/target-ppc/cpu.h  |    5 +
 5 files changed, 126 insertions(+), 65 deletions(-)

Index: kvm-userspace.git/qemu/qemu-kvm-vcpu.h
===================================================================
--- /dev/null
+++ kvm-userspace.git/qemu/qemu-kvm-vcpu.h
@@ -0,0 +1,34 @@
+/*
+ * qemu/kvm vcpu definitions
+ *
+ * Copyright (C) 2006-2008 Qumranet Technologies
+ *
+ * Licensed under the terms of the GNU GPL version 2 or higher.
+ */
+#ifndef QEMU_KVM_VCPU_H
+#define QEMU_KVM_VCPU_H
+
+#include <pthread.h>
+
+struct qemu_kvm_work_item {
+    struct qemu_kvm_work_item *next;
+    void (*func)(void *data);
+    void *data;
+    int done;
+};
+
+/*
+ * KVM vcpu struct
+ */
+struct vcpu_info {
+    int sipi_needed;
+    int init;
+    pthread_t thread;
+    int signalled;
+    int stop;
+    int stopped;
+    int created;
+    struct qemu_kvm_work_item *queued_work_first, *queued_work_last;
+};
+
+#endif
Index: kvm-userspace.git/qemu/qemu-kvm.c
===================================================================
--- kvm-userspace.git.orig/qemu/qemu-kvm.c
+++ kvm-userspace.git/qemu/qemu-kvm.c
@@ -22,13 +22,13 @@
 #include "compatfd.h"
 
 #include "qemu-kvm.h"
+#include "qemu-kvm-vcpu.h"
 #include <libkvm.h>
 #include <pthread.h>
 #include <sys/utsname.h>
 #include <sys/syscall.h>
 #include <sys/mman.h>
 
-#define bool _Bool
 #define false 0
 #define true 1
 
@@ -43,31 +43,12 @@
 pthread_cond_t qemu_system_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t qemu_pause_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t qemu_work_cond = PTHREAD_COND_INITIALIZER;
-__thread struct vcpu_info *vcpu;
+__thread struct CPUState *current_env;
 
 static int qemu_system_ready;
 
 #define SIG_IPI (SIGRTMIN+4)
 
-struct qemu_kvm_work_item {
-    struct qemu_kvm_work_item *next;
-    void (*func)(void *data);
-    void *data;
-    bool done;
-};
-
-struct vcpu_info {
-    CPUState *env;
-    int sipi_needed;
-    int init;
-    pthread_t thread;
-    int signalled;
-    int stop;
-    int stopped;
-    int created;
-    struct qemu_kvm_work_item *queued_work_first, *queued_work_last;
-} vcpu_info[256];
-
 pthread_t io_thread;
 static int io_thread_fd = -1;
 static int io_thread_sigfd = -1;
@@ -93,7 +74,20 @@
 
 CPUState *qemu_kvm_cpu_env(int index)
 {
-    return vcpu_info[index].env;
+    CPUState *penv;
+
+    if (current_env->cpu_index == index)
+        return current_env;
+
+    penv = first_cpu;
+
+    while (penv) {
+        if (penv->cpu_index == index)
+            return penv;
+        penv = (CPUState *)penv->next_cpu;
+    }
+
+    return NULL;
 }
 
 static void sig_ipi_handler(int n)
@@ -102,10 +96,10 @@
 
 static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
 {
-    struct vcpu_info *vi = &vcpu_info[env->cpu_index];
+    struct vcpu_info *vi = &env->vcpu_info;
     struct qemu_kvm_work_item wi;
 
-    if (vi == vcpu) {
+    if (env == current_env) {
         func(data);
         return;
     }
@@ -127,7 +121,7 @@
 
 static void inject_interrupt(void *data)
 {
-    cpu_interrupt(vcpu->env, (int)data);
+    cpu_interrupt(current_env, (int)data);
 }
 
 void kvm_inject_interrupt(CPUState *env, int mask)
@@ -140,29 +134,33 @@
     int signal = 0;
 
     if (env) {
-        if (!vcpu)
+        if (current_env && !current_env->vcpu_info.created)
             signal = 1;
-        if (vcpu && env != vcpu->env && !vcpu_info[env->cpu_index].signalled)
+        /*
+         * Testing for vcpu_info.created here is really redundant
+         */
+        if (current_env->vcpu_info.created &&
+            env != current_env && !env->vcpu_info.signalled)
             signal = 1;
 
         if (signal) {
-            vcpu_info[env->cpu_index].signalled = 1;
-                if (vcpu_info[env->cpu_index].thread)
-                    pthread_kill(vcpu_info[env->cpu_index].thread, SIG_IPI);
+            env->vcpu_info.signalled = 1;
+            if (env->vcpu_info.thread)
+                pthread_kill(env->vcpu_info.thread, SIG_IPI);
         }
     }
 }
 
 void kvm_update_after_sipi(CPUState *env)
 {
-    vcpu_info[env->cpu_index].sipi_needed = 1;
+    env->vcpu_info.sipi_needed = 1;
     kvm_update_interrupt_request(env);
 }
 
 void kvm_apic_init(CPUState *env)
 {
     if (env->cpu_index != 0)
-	vcpu_info[env->cpu_index].init = 1;
+	env->vcpu_info.init = 1;
     kvm_update_interrupt_request(env);
 }
 
@@ -240,7 +238,7 @@
 
 static int has_work(CPUState *env)
 {
-    if (!vm_running || (env && vcpu_info[env->cpu_index].stopped))
+    if (!vm_running || (env && env->vcpu_info.stopped))
 	return 0;
     if (!env->halted)
 	return 1;
@@ -249,7 +247,7 @@
 
 static void flush_queued_work(CPUState *env)
 {
-    struct vcpu_info *vi = &vcpu_info[env->cpu_index];
+    struct vcpu_info *vi = &env->vcpu_info;
     struct qemu_kvm_work_item *wi;
 
     if (!vi->queued_work_first)
@@ -266,6 +264,7 @@
 
 static void kvm_main_loop_wait(CPUState *env, int timeout)
 {
+    struct vcpu_info *vi = &env->vcpu_info;
     struct timespec ts;
     int r, e;
     siginfo_t siginfo;
@@ -291,49 +290,55 @@
     cpu_single_env = env;
     flush_queued_work(env);
 
-    if (vcpu_info[env->cpu_index].stop) {
-	vcpu_info[env->cpu_index].stop = 0;
-	vcpu_info[env->cpu_index].stopped = 1;
+    if (vi->stop) {
+	vi->stop = 0;
+	vi->stopped = 1;
 	pthread_cond_signal(&qemu_pause_cond);
     }
 
-    vcpu_info[env->cpu_index].signalled = 0;
+    vi->signalled = 0;
 }
 
 static int all_threads_paused(void)
 {
-    int i;
+    CPUState *penv = first_cpu;
+
+    while (penv) {
+        if (penv->vcpu_info.stop)
+            return 0;
+        penv = (CPUState *)penv->next_cpu;
+    }
 
-    for (i = 0; i < smp_cpus; ++i)
-	if (vcpu_info[i].stop)
-	    return 0;
     return 1;
 }
 
 static void pause_all_threads(void)
 {
-    int i;
+    CPUState *penv = first_cpu;
 
     assert(!cpu_single_env);
 
-    for (i = 0; i < smp_cpus; ++i) {
-	vcpu_info[i].stop = 1;
-	pthread_kill(vcpu_info[i].thread, SIG_IPI);
+    while (penv) {
+        penv->vcpu_info.stop = 1;
+        pthread_kill(penv->vcpu_info.thread, SIG_IPI);
+        penv = (CPUState *)penv->next_cpu;
     }
+
     while (!all_threads_paused())
 	qemu_cond_wait(&qemu_pause_cond);
 }
 
 static void resume_all_threads(void)
 {
-    int i;
+    CPUState *penv = first_cpu;
 
     assert(!cpu_single_env);
 
-    for (i = 0; i < smp_cpus; ++i) {
-	vcpu_info[i].stop = 0;
-	vcpu_info[i].stopped = 0;
-	pthread_kill(vcpu_info[i].thread, SIG_IPI);
+    while (penv) {
+        penv->vcpu_info.stop = 0;
+        penv->vcpu_info.stopped = 0;
+        pthread_kill(penv->vcpu_info.thread, SIG_IPI);
+        penv = (CPUState *)penv->next_cpu;
     }
 }
 
@@ -348,8 +353,8 @@
 static void update_regs_for_sipi(CPUState *env)
 {
     kvm_arch_update_regs_for_sipi(env);
-    vcpu_info[env->cpu_index].sipi_needed = 0;
-    vcpu_info[env->cpu_index].init = 0;
+    env->vcpu_info.sipi_needed = 0;
+    env->vcpu_info.init = 0;
 }
 
 static void update_regs_for_init(CPUState *env)
@@ -376,21 +381,23 @@
 
 void qemu_kvm_system_reset(void)
 {
-    int i;
+    CPUState *penv = first_cpu;
 
     pause_all_threads();
 
     qemu_system_reset();
 
-    for (i = 0; i < smp_cpus; ++i)
-	kvm_arch_cpu_reset(vcpu_info[i].env);
+    while (penv) {
+        kvm_arch_cpu_reset(penv);
+        penv = (CPUState *)penv->next_cpu;
+    }
 
     resume_all_threads();
 }
 
 static int kvm_main_loop_cpu(CPUState *env)
 {
-    struct vcpu_info *info = &vcpu_info[env->cpu_index];
+    struct vcpu_info *info = &env->vcpu_info;
 
     setup_kernel_sigmask(env);
 
@@ -429,9 +436,8 @@
     CPUState *env = _env;
     sigset_t signals;
 
-    vcpu = &vcpu_info[env->cpu_index];
-    vcpu->env = env;
-    vcpu->env->thread_id = kvm_get_thread_id();
+    current_env = env;
+    env->thread_id = kvm_get_thread_id();
     sigfillset(&signals);
     sigprocmask(SIG_BLOCK, &signals, NULL);
     kvm_create_vcpu(kvm_context, env->cpu_index);
@@ -439,7 +445,7 @@
 
     /* signal VCPU creation */
     pthread_mutex_lock(&qemu_mutex);
-    vcpu->created = 1;
+    current_env->vcpu_info.created = 1;
     pthread_cond_signal(&qemu_vcpu_cond);
 
     /* and wait for machine initialization */
@@ -453,9 +459,9 @@
 
 void kvm_init_new_ap(int cpu, CPUState *env)
 {
-    pthread_create(&vcpu_info[cpu].thread, NULL, ap_main_loop, env);
+    pthread_create(&env->vcpu_info.thread, NULL, ap_main_loop, env);
 
-    while (vcpu_info[cpu].created == 0)
+    while (env->vcpu_info.created == 0)
 	qemu_cond_wait(&qemu_vcpu_cond);
 }
 
@@ -613,8 +619,12 @@
 
 static int kvm_debug(void *opaque, int vcpu)
 {
+    CPUState *env;
+
+    env = qemu_kvm_cpu_env(vcpu);
+
     kvm_debug_stop_requested = 1;
-    vcpu_info[vcpu].stopped = 1;
+    env->vcpu_info.stopped = 1;
     return 1;
 }
 
@@ -710,8 +720,11 @@
 
 static int kvm_shutdown(void *opaque, int vcpu)
 {
+    CPUState *env;
+
+    env = qemu_kvm_cpu_env(cpu_single_env->cpu_index);
     /* stop the current vcpu from going back to guest mode */
-    vcpu_info[cpu_single_env->cpu_index].stopped = 1;
+    env->vcpu_info.stopped = 1;
 
     qemu_system_reset_request();
     return 1;
Index: kvm-userspace.git/qemu/target-i386/cpu.h
===================================================================
--- kvm-userspace.git.orig/qemu/target-i386/cpu.h
+++ kvm-userspace.git/qemu/target-i386/cpu.h
@@ -45,6 +45,7 @@
 #include "cpu-defs.h"
 
 #include "softfloat.h"
+#include "qemu-kvm-vcpu.h"
 
 #define R_EAX 0
 #define R_ECX 1
@@ -622,6 +623,9 @@
 #define NR_IRQ_WORDS (256/ BITS_PER_LONG)
     uint32_t kvm_interrupt_bitmap[NR_IRQ_WORDS];
 
+#ifdef USE_KVM
+    struct vcpu_info vcpu_info;
+#endif
     /* in order to simplify APIC support, we leave this pointer to the
        user */
     struct APICState *apic_state;
Index: kvm-userspace.git/qemu/target-ia64/cpu.h
===================================================================
--- kvm-userspace.git.orig/qemu/target-ia64/cpu.h
+++ kvm-userspace.git/qemu/target-ia64/cpu.h
@@ -40,10 +40,15 @@
 #include "cpu-defs.h"
 
 #include "softfloat.h"
+#include "qemu-kvm-vcpu.h"
+
 typedef struct CPUIA64State {
     CPU_COMMON;
     uint32_t hflags;
     int mp_state;
+#ifdef USE_KVM
+    struct vcpu_info vcpu_info;
+#endif
 } CPUIA64State;
 
 #define CPUState CPUIA64State
Index: kvm-userspace.git/qemu/target-ppc/cpu.h
===================================================================
--- kvm-userspace.git.orig/qemu/target-ppc/cpu.h
+++ kvm-userspace.git/qemu/target-ppc/cpu.h
@@ -22,6 +22,7 @@
 
 #include "config.h"
 #include <inttypes.h>
+#include "qemu-kvm-vcpu.h"
 
 //#define PPC_EMULATE_32BITS_HYPV
 
@@ -578,6 +579,10 @@
 
     CPU_COMMON
 
+#ifdef USE_KVM
+    struct vcpu_info vcpu_info;
+#endif
+
     int access_type; /* when a memory exception occurs, the access
                         type is stored here */