From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch] remove vcpu_info array v5 Date: Mon, 10 Nov 2008 15:48:40 +0200 Message-ID: <49183BB8.7080502@redhat.com> References: <4909C00F.8050704@sgi.com> <49103812.1070104@redhat.com> <491835FF.9030400@sgi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Anthony Liguori , Hollis Blanchard , "kvm@vger.kernel.org" , "kvm-ia64@vger.kernel.org" , Glauber de Oliveira Costa To: Jes Sorensen Return-path: Received: from mx2.redhat.com ([66.187.237.31]:46620 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754439AbYKJNsr (ORCPT ); Mon, 10 Nov 2008 08:48:47 -0500 In-Reply-To: <491835FF.9030400@sgi.com> Sender: kvm-owner@vger.kernel.org List-ID: Jes Sorensen wrote: > 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. > The movement is fine, just keep it named kvm so we know it is kvm specific. >>> -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? No, libkvm is independent of qemu. We can make kvm_create_vcpu(kvm_context_t kvm, int vcpu, void *env), and pass 'env' as the opaque to all the callbacks, instead of the vcpu number. > >> >> 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. > It's more of a maintenance issue so that I can keep track of what is in qemu upstream and what is local. Merges are already painful enough. >>> >> >> 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. > It's only relevant if you have per-vcpu threads, which qemu doesn't have. >>> -#define bool _Bool >>> >> >> why? > > It was unused and didn't add any value. > So separate patch. >> 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. How's my suggestion above? -- error compiling committee.c: too many arguments to function