From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function Date: Fri, 18 Jan 2013 12:20:14 -0200 Message-ID: <20130118142013.GX10683@otherpad.lan.raisama.net> 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> <50F9480D.3040400@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Igor Mammedov , Gleb Natapov , qemu-devel@nongnu.org, Anthony Liguori , "kvm@vger.kernel.org list" To: Andreas =?iso-8859-1?Q?F=E4rber?= Return-path: Content-Disposition: inline In-Reply-To: <50F9480D.3040400@suse.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org On Fri, Jan 18, 2013 at 02:03:09PM +0100, Andreas F=E4rber wrote: > 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 woul= d 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() get= s > > 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 = you > > add it before committing, or should I resubmit? >=20 > Could you suggest a text for me to add please? "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int works on most or all architectures supporting KVM, but it is safer to use an appropriate 'unsigned long' parameter." To find out if 'int' breaks on any architecture, I would need to check the ABI specification for each architecture. I didn't do that, but I am sure we should pass an unsigned long instead, if that's the type expected by the kernel. --=20 Eduardo From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44399) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TwCmM-0004ew-C0 for qemu-devel@nongnu.org; Fri, 18 Jan 2013 09:18:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TwCmH-0004pg-6Q for qemu-devel@nongnu.org; Fri, 18 Jan 2013 09:18:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51881) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TwCmG-0004pK-TR for qemu-devel@nongnu.org; Fri, 18 Jan 2013 09:18:41 -0500 Date: Fri, 18 Jan 2013 12:20:14 -0200 From: Eduardo Habkost Message-ID: <20130118142013.GX10683@otherpad.lan.raisama.net> 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> <50F9480D.3040400@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <50F9480D.3040400@suse.de> 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: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Igor Mammedov , Gleb Natapov , qemu-devel@nongnu.org, Anthony Liguori , "kvm@vger.kernel.org list" On Fri, Jan 18, 2013 at 02:03:09PM +0100, Andreas F=E4rber wrote: > 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 woul= d 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() get= s > > 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 = you > > add it before committing, or should I resubmit? >=20 > Could you suggest a text for me to add please? "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int works on most or all architectures supporting KVM, but it is safer to use an appropriate 'unsigned long' parameter." To find out if 'int' breaks on any architecture, I would need to check the ABI specification for each architecture. I didn't do that, but I am sure we should pass an unsigned long instead, if that's the type expected by the kernel. --=20 Eduardo