Re: [patch] remove vcpu_info array v5

On Tue, Nov 4, 2008 at 9:54 AM, Avi Kivity <[email protected]> wrote:
> Jes Sorensen wrote:
>>
>> So here we go, trying to catch up on the PCI passthrough patch revision
>> number, here's v5 of the struct vcpu_info patch.
>>
>> In the end I decided to merge the contents of struct vcpu_info directly
>> into CPU_COMMON as it was silly to create new files just to remove them
>> in the next patch again.
>>
>> This boots for me on ia64 and builds on x86_64, obviously it is 100%
>> perfect <tm>!
>>
>> Comments, bug reports, or upstream merge, welcome :-)
>> Merge vcpu_info into CPUState.
>>
>> 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.
>
>> -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?
>
>>  #define CPU_TEMP_BUF_NLONGS 128
>>  #define CPU_COMMON                                                      \
>>     struct TranslationBlock *current_tb; /* currently executing TB  */  \
>> @@ -200,6 +204,15 @@
>>     /* user data */                                                     \
>>     void *opaque;                                                       \
>>                                                                         \
>> -    const char *cpu_model_str;
>> +    const char *cpu_model_str;                                          \
>> +                                                                        \
>> +    int sipi_needed;                                                    \
>> +    int init;                                                           \
>> +    pthread_t thread;                                                   \
>> +    int signalled;                                                      \
>> +    int stop;                                                           \
>> +    int stopped;                                                        \
>> +    int created;                                                        \
>> +    struct qemu_work_item *queued_work_first, *queued_work_last;
>>
>
> struct kvm_stuff { ... } and put that in as a member, so it's easier to
> remember this is a kvm addition.
>
>>   #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.
By far it is not. But at least it does not depend on any kvm-specific
data, and works
for both qemu and kvm. So it's better to be this way.

If we change this scheme to a better one, like a hash table, then
it'll improve both qemu and kvm.

>> @@ -143,4 +143,12 @@
>>  void cpu_save(QEMUFile *f, void *opaque);
>>  int cpu_load(QEMUFile *f, void *opaque, int version_id);
>>  +/* 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.
>
>
>> @@ -28,7 +28,6 @@
>>  #include <sys/syscall.h>
>>  #include <sys/mman.h>
>>  -#define bool _Bool
>>
>
> why?
>
> Please separate into a patch that does the kvm_run int vcpu -> void *vcpu
> conversion, and then the vcpu_info -> env conversion.
>
> --
> error compiling committee.c: too many arguments to function
>
>



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

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