Re: [RFC PATCH v2 1/6] kvm: add device control API

[Gleb Natapov <gleb@redhat.com>]

On Wed, Apr 03, 2013 at 07:39:52PM +0200, Alexander Graf wrote:
> 
> On 03.04.2013, at 19:37, Scott Wood wrote:
> 
> > On 04/03/2013 08:22:37 AM, Alexander Graf wrote:
> >> On 03.04.2013, at 04:17, Paul Mackerras wrote:
> >> > On Tue, Apr 02, 2013 at 08:19:56PM -0500, Scott Wood wrote:
> >> >> On 04/02/2013 08:02:39 PM, Paul Mackerras wrote:
> >> >>> On Mon, Apr 01, 2013 at 05:47:48PM -0500, Scott Wood wrote:
> >> >>>> +4.79 KVM_CREATE_DEVICE
> >> >>>> +
> >> >>>> +Capability: KVM_CAP_DEVICE_CTRL
> >> >>>
> >> >>> I notice this patch doesn't add this capability;
> >> >>
> >> >> Yes, it does (see below).
> >> >>
> >> >>> you add it in a later patch.
> >> >>
> >> >> Maybe you're thinking of KVM_CAP_IRQ_MPIC?
> >> >
> >> > No, I was referring to the addition to kvm_dev_ioctl_check_extension()
> >> > of a KVM_CAP_DEVICE_CTRL case.  Since this patch adds the code to handle
> >> > KVM_CREATE_DEVICE ioctl it should also add the code to return 1 if
> >> > userspace queries the KVM_CAP_DEVICE_CTRL capability.
> >> >
> >> >>>> +/* ioctl for vm fd */
> >> >>>> +#define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct
> >> >>> kvm_create_device)
> >> >>>
> >> >>> This define should go with the other VM ioctls, otherwise the next
> >> >>> person to add a VM ioctl will probably miss it and reuse the 0xe0
> >> >>> code.
> >> >>
> >> >> That's actually why I moved it to a new section, with device control
> >> >> ioctls getting their own range, as the legacy "device model" and
> >> >> some other things did.  0xe0 is not the next ioctl that would be
> >> >> used for either vm or vcpu.  The ioctl numbering is actually already
> >> >> a mess, with sometimes care being taken to keep vcpu and vm ioctls
> >> >> from overlapping, but on other places overlapping does happen.  I'm
> >> >> not sure what exactly I should do here.
> >> >
> >> > Well, even if you are using a new range, I still think that
> >> > KVM_CREATE_DEVICE, being a VM ioctl, should go next to the other VM
> >> > ioctls.  I guess it's ultimately up to the maintainers.
> >> I agree. Things get confusing for VM ioctls otherwise.
> > 
> > Things are already confusing. :-)
> > 
> > I can move KVM_CREATE_DEVICE back with the other VM ioctls, but what number should it get?  The last VM ioctl is 0xab (which is also a VCPU ioctl).  Should I use 0xac (which is also a VCPU ioctl)?  Or should I try to avoid a conflict, as was sometimes done in the past -- in which case, which number should I use?
> 
> Gleb, Marcelo?
> 
> 
Yes, ioctls number assignments are a little bit of a mess :( There are 14
numbers that are used twice and some of them are used twice for the same
type of fd, but with different direction bits and there is one, 0x9b,
that is used twice and have the same IO direction and even, potentially,
same parameter size (just checked it has the same parameter size)! Good that
the second use is only on IA64 which is almost dead.

Although ioctls numbers between different types of fds should not conflict
lets try and make them unique. Scott, please put KVM_CREATE_DEVICE near
device model ioctls and give it a unique number.

--
			Gleb.