Re: [patch] remove vcpu_info array v5

Jes Sorensen
2008-11-10T13:24:23Z

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