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

Alexander Graf
2013-04-03T13:22:37Z

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.

Alex