On Fri, Oct 17, 2008 at 1:28 PM, Jes Sorensen <jes@xxxxxxx> wrote:
> 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.
One more reason to provide a general purpose qemu function, instead of
a kvm specific one.
As for the patch itself, I'll re-review it later. stay tuned
>>> 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 :)
>
> 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@xxxxxxx>
>
> ---
> 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 */
>
>
>
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
--
To unsubscribe from this list: send the line "unsubscribe kvm-ia64" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
