From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jes Sorensen Subject: Re: [patch] remove vcpu_info array v5 Date: Mon, 10 Nov 2008 14:24:15 +0100 Message-ID: <491835FF.9030400@sgi.com> References: <4909C00F.8050704@sgi.com> <49103812.1070104@redhat.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: Avi Kivity Return-path: Received: from relay1.sgi.com ([192.48.179.29]:57122 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754577AbYKJNYX (ORCPT ); Mon, 10 Nov 2008 08:24:23 -0500 In-Reply-To: <49103812.1070104@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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