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

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 07:35:22AM -0700, Eric Blake wrote:
> On 01/21/2013 06:14 AM, Andreas Färber wrote:
> >> glibc is already responsible from converting the 'unsigned long
> >> int' of the user declaration back into the 'unsigned int' that the
> >> kernel expects for the second argument.  The third argument (when
> >> present), is generally treated as a pointer (of size appropriate
> >> for the architecture).  Although there _might_ be an ioctl that
> >> uses it directly as an integer instead of dereferencing it as a
> >> pointer, those would be the exceptions to the rule.
> > 
> > So ... do we have a conclusion what to put into the commit message? :)
> > 
> > It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
> > unsigned long but maybe uintptr_t would be more correct then?
> 
> uintptr_t feels more correct - the 3rd (vararg) argument through the
> ioctl() syscall is always retrieved using the same size as void*.

Actually, sys_ioctl() always retrieve it using "unsigned long", but
nothing prevents the arch-specific syscall entry code to from
translating something from a different type to "unsigned long" before
calling sys_ioctl().

So I guess the only guarantee we have is the Linux ioctl(2) man page,
that says: "The third argument is an untyped pointer to memory. It's
traditionally char *argp (from the days before void * was valid C), and
will be so named for this discussion."

That said, I plan to change the code to cast the argument to (void*) in
the next version.

> 
> > 
> > Or should kvm_vm_ioctl() be fixed to use something else instead?
> > Eric's int would be a semantic change for the 64-bit platforms, no?
> 
> My discussion about 'int' vs. 'unsigned long' was in regards to the
> second argument KVM_CREATE_VCPU, which your patch does not change
> (perhaps my fault for jumping in on a conversation mid-thread without
> actually reading your original patch, which I have now done).  That is,
> KVM_CREATE_VCPU as a constant is always 32 bits (kernel constraint),
> widened out to unsigned long when passed to the glibc function (due to
> the glibc signature disagreeing with POSIX), then narrowed back down to
> 32 bits when forwarded to the kernel syscall.
> 
> Meanwhile, your patch is fixing the third argument from 'int' to a wider
> type, which is necessary for passing that value through varargs when the
> receiving end will retrieve the same argument via a void* variable.

I am confident that "unsigned long" will work properly on all
architectures we care about today, but I also don't know if this is
documented and guaranteed to work on all architectures. Passing an
argument of the documented type (void*) sounds like the right thing to
do.

-- 
Eduardo