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 :)