From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53300) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TwBbI-0002Pb-5f for qemu-devel@nongnu.org; Fri, 18 Jan 2013 08:03:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TwBbG-0003ZB-Jr for qemu-devel@nongnu.org; Fri, 18 Jan 2013 08:03:16 -0500 Received: from cantor2.suse.de ([195.135.220.15]:37389 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TwBbG-0003Yl-9P for qemu-devel@nongnu.org; Fri, 18 Jan 2013 08:03:14 -0500 Message-ID: <50F9480D.3040400@suse.de> Date: Fri, 18 Jan 2013 14:03:09 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1358456378-29248-1-git-send-email-ehabkost@redhat.com> <1358456378-29248-5-git-send-email-ehabkost@redhat.com> <50F92DE1.7040107@suse.de> <20130118125340.GV10683@otherpad.lan.raisama.net> In-Reply-To: <20130118125340.GV10683@otherpad.lan.raisama.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Igor Mammedov , Gleb Natapov , qemu-devel@nongnu.org, Anthony Liguori , "kvm@vger.kernel.org list" Am 18.01.2013 13:53, schrieb Eduardo Habkost: > On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas F=E4rber wrote: > [...] >>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */ >>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu); >>> + >>> void kvm_arch_reset_vcpu(CPUState *cpu); >>> =20 >>> int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr); >>> diff --git a/kvm-all.c b/kvm-all.c >>> index 6278d61..995220d 100644 >>> --- a/kvm-all.c >>> +++ b/kvm-all.c >>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu) >>> =20 >>> DPRINTF("kvm_init_vcpu\n"); >>> =20 >>> - ret =3D kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index); >>> + ret =3D kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu)); >>> if (ret < 0) { >>> DPRINTF("kvm_create_vcpu failed\n"); >>> goto err; >> >> This is changing the vararg from int to unsigned long. I have no >> insights yet on how this is handled and whether that is okay; I would = at >> least expect this change to be mentioned in the commit message. >=20 > It was an unexpected change (I didn't notice that cpu_index was int), > but strictly speaking the previous code was incorrect (as ioctl() gets > an unsigned long argument, not int). I doubt there are cases where it > would really break, but it is a good thing to fix it. >=20 > I agree this should be mentioned in the commit message, though. Will yo= u > add it before committing, or should I resubmit? Could you suggest a text for me to add please? >>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >>> index 3acff40..5f3f789 100644 >>> --- a/target-i386/kvm.c >>> +++ b/target-i386/kvm.c >>> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int r= unning, RunState state) >>> } >>> } >>> =20 >>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu) >>> +{ >>> + return cpu->cpu_index; >>> +} >>> + >>> int kvm_arch_init_vcpu(CPUState *cs) >>> { >>> struct { >> >> Minor nit: If you change this to CPUState *cs you spare the renaming i= n >> 05/12. Alternatively use x86_cpu there (not much code affected so you >> can just ignore this, no need to respin just for that). >> >> Otherwise looks okay to me. >=20 > I actually wanted to rename the variable only when necessary, otherwise > this patch would be confusing if all architectures used 'cpu' and i386 > used 'cs'. It's inconsistent anyway, 'cs' is relatively new and I see no reason to use it in the prototype. But OK, once 03/12 gets an ack I'll start applying. >=20 > (And I like using "cpu" for the more specific CPU type in the function > [e.g. CPUState or X86CPUState depending on the case] and abbreviations > [like 'cs'] for the more generic types. I believe I have seen this styl= e > used in other parts of the code.) Yes, I chose to use "cpu" for the more frequently used type. I don't really like "cs" but it seemed better than "base_cpu" or so. When changing something over from "env" something with three letters looks nicer though, easier to review. Can't have everything. ;) Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg