Re: [patch] remove vcpu_info array v5

Avi Kivity wrote:
> Jes Sorensen wrote:
>> Move contents of struct vcpu_info directly into CPU_COMMON. Rename
>> struct qemu_kvm_work_item to qemu_work_item as it really isn't KVM
>> specific.
> 
> Well, it is actually kvm specific, since qemu doesn't have vccpu threads.

Hi Avi,

Anthony suggested I moved things directly in there, after my previous
patch kept it as a struct.

>> -int kvm_run(kvm_context_t kvm, int vcpu)
>> +int kvm_run(kvm_context_t kvm, int vcpu, void *env)
>>   
> 
> That's a little ugly -- passing two arguments which describe the vcpu.  
> How about passing env to kvm_create_vcpu() instead?

I am all in favor of doing this, but in order to do so, we have to
teach libkvm about CPUState - are you ok with that?

> 
> struct kvm_stuff { ... } and put that in as a member, so it's easier to 
> remember this is a kvm addition.

If we can agree, I really don't care too much. There's already plenty
of stuff in CPU_COMMON that isn't used by specific users, so I didn't
think this was an issue.

>>  
>>  #if defined(TARGET_I386) || defined(TARGET_X86_64)
>> +#ifdef USE_KVM
>> +static CPUState *qemu_kvm_cpu_env(int index)
>> +{
>> +    CPUState *penv;
>> +
>> +    penv = first_cpu;
>> +
>> +    while (penv) {
>> +        if (penv->cpu_index = index)
>> +            return penv;
>> +        penv = (CPUState *)penv->next_cpu;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +#endif
>>   
> 
> This can't be the best way of determining the env pointer from a cpu 
> number.

It's not pretty, but this is only used in the ACPU hotplug code now, so
it's less performance critical.

>> +/* work queue */
>> +struct qemu_work_item {
>> +    struct qemu_kvm_work_item *next;
>>   
> 
> Missed rename here.
 >
>> +    void (*func)(void *data);
>> +    void *data;
>> +    int done;
>> +};
>> +
>>   
> 
> Please keep this in kvm specific files.

Why? There really isn't anything specific about a work queue list
like this. Making it an available functionality for QEMU shouldn't
hurt.

>> -#define bool _Bool
>>   
> 
> why?

It was unused and didn't add any value.

> Please separate into a patch that does the kvm_run int vcpu -> void 
> *vcpu conversion, and then the vcpu_info -> env conversion.

As mentioned above, if I am going to go that way, we need to teach
libkvm about 'env'. I am all in favor, but I am not going to do that
unless I can get an ack that it's acceptable to do so.


Cheers,
Jes