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

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 18.01.2013 18:46, schrieb Eric Blake:
> On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
>> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>>> 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."
>>> 
>>> Interestingly enough, while the Linux syscall uses 'unsigned
>>> long', the POSIX definition of ioctl() uses 'int'; so the Linux
>>> kernel is already constrained to never use an ioctl value that
>>> doesn't fit within 'int',
>> 
>> Really? What about the ioctl()s that get a pointer as argument
>> on architectures where pointers don't fit in an int?
>> 
>> Do you have a pointer to the POSIX definition you are talking
>> about?
>> 
>> Note that I'm talking about the the extra ioctl() argument, not
>> the ioctl() number (that is an unsigned int in the kernel code).
> 
> Okay, now you made me go back and check sources.
> 
> POSIX 2008 says: #include <stropts.h> int ioctl(int fildes, int
> request, ... /* arg */);
> 
> Gnulib says this about a bug that it works around: @item On glibc
> platforms, the second parameter is of type @code{unsigned long} 
> rather than @code{int}.
> 
> But gnulib also suggests using <sys/ioctl.h> instead of the POSIX
> header <stropts.h> for getting ioctl(), because <stropts.h> was
> declared obsolete in POSIX 2008 and was never implemented in
> glibc.
> 
> Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still
> see: extern int ioctl (int __fd, unsigned long int __request, ...)
> __THROW;
> 
> Meanwhile, you are correct that the kernel defines request as 32
> bits: linux.git:include/uapi/asm-generic/ioctl.h /* ioctl command
> encoding: 32 bits total, command in lower 16 bits, * size of the
> parameter structure in the lower 14 bits of the * upper 16 bits. *
> Encoding the size of the parameter structure in the ioctl request *
> is useful for catching programs compiled with old versions * and to
> avoid overwriting user space outside the user buffer area. * The
> highest 2 bits are reserved for indicating the ``access mode''. *
> NOTE: This limits the max parameter size to 16kB -1 ! */
> 
>> 
>>> and glibc is already responsible for ensuring that argument
>>> promotion of an int doesn't change the behavior of ioctl() in
>>> libc when converting it over to the unsigned long syscall
>>> semantics expected by the kernel.
> 
> So a more precise wording of this is:
> 
> 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?

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?

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJQ/T8lAAoJEPou0S0+fgE/gl8QALesZwG5q07W21mp2j4ikL8N
jrBHjG2VZ9Kda+AIGMClVsWntGZSOzHdtriJ4gjxp90D71S/LQfsYAy6bj45FIwS
kPbIQblLlL5Xc6ZiTS5yTzkwyEd7gUpDVouXyv3XxeyUxqhQKwgWxpiP4RftbBRI
Z8wLbVFNpIoIsHfhKoNkT4M/Ucm1iZbChV6y4zqltAfdQhcl6Gq0jzhtkAfmN41t
p3tCJYldRwayiKLsO2Y0BMNrKmCJisKCEGmkCQzye/3cuFoat/WUmpjV/65hLNtm
ruzfn6pCqMTEGPC4YeDdUsxAhVzVX+Sd4mBKHBGItmvhhJMFYUtwTosRwX5bOrAJ
mpVLAj5/XDYTm2/jQUEOJAqpxUr5oAVMQL3sNeWJPmXkk1kNaNWTNVHHDW1iJnRj
ty0YIWOnuNabkwiDEjPCz6ghjfA3wOBWy8Gk3+F21MYgRQwDTFw4JZuroOIzy3iD
6Vs4MmiBUGnoLobSqw2dUZFmjL7a1500AxZG0MwBd+EqnbLHGqD33kvLrbUYT8+F
eW+cqKV+ZXo3ux343rTxD6EFgmN7GmHSkknxJN5m6ldlw5wfFQ8KhdCiKjwSq3EP
X0bVGmryEdIh+6w/RbhL75Vfb/Je0mr/GzhtijtXo+FORkF8ip2mlpVSl46r0AfI
KvsZ0HZqZHsfoaSBC1js
=/cL3
-----END PGP SIGNATURE-----