Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 02:03:09PM +0100, Andreas Färber wrote:
> Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> > On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber 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);
> >>>  
> >>>  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)
> >>>  
> >>>      DPRINTF("kvm_init_vcpu\n");
> >>>  
> >>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> >>> +    ret = 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.
> > 
> > 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.
> > 
> > I agree this should be mentioned in the commit message, though. Will you
> > add it before committing, or should I resubmit?
> 
> 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.

-- 
Eduardo