From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jes Sorensen Subject: Re: [patch] fold struct vcpu_info into CPUState Date: Fri, 17 Oct 2008 17:28:00 +0200 Message-ID: <48F8AF00.1050304@sgi.com> References: <48E0F318.7050303@sgi.com> <5d6222a80810131524s7ec55bfyb296085c6c2ac4af@mail.gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------020804030009050806050903" Cc: kvm@vger.kernel.org, kvm-ia64@vger.kernel.org To: Glauber Costa Return-path: Received: from relay1.sgi.com ([192.48.171.29]:33642 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754697AbYJQP2G (ORCPT ); Fri, 17 Oct 2008 11:28:06 -0400 In-Reply-To: <5d6222a80810131524s7ec55bfyb296085c6c2ac4af@mail.gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: This is a multi-part message in MIME format. --------------020804030009050806050903 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 :) --------------020804030009050806050903 Content-Type: text/plain; name="0010-qemu-kvm-vcpu_info-v2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0010-qemu-kvm-vcpu_info-v2.patch" 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 --- 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 + +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 #include #include #include #include -#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 +#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 */ --------------020804030009050806050903--