Re: [patch] fold struct vcpu_info into CPUState

Glauber Costa
2008-10-13T22:24:23Z

> Index: kvm-userspace.git/qemu/qemu-kvm-vcpu.h
> ===================================================================
> --- /dev/null
> +++ kvm-userspace.git/qemu/qemu-kvm-vcpu.h
can't it be just qemu-kvm.h ?

> 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"
>
> +__thread struct CPUState *current_env;
>
> static int qemu_system_ready;
>
> #define SIG_IPI (SIGRTMIN+4)
>

> 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;
> }

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)

> 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.

> - 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?

> 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.

The rest seems pretty straightforward.

--
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."