Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops

[Paolo Bonzini <pbonzini@redhat.com>]

Il 30/06/2014 11:21, Cornelia Huck ha scritto:
> On Thu, 26 Jun 2014 18:30:16 +0100
> Will Deacon <will.deacon@arm.com> wrote:
>
>> kvm_ioctl_create_device currently has knowledge of all the device types
>> and their associated ops. This is fairly inflexible when adding support
>> for new in-kernel device emulations, so move what we currently have out
>> into a table, which can support dynamic registration of ops by new
>> drivers for virtual hardware.
>>
>> I didn't try to port all current drivers over, as it's not always clear
>> which initialisation hook the ops should be registered from.
>
> I like the general idea of registering the ops dynamically, some
> comments below.
>
>>
>> Cc: Gleb Natapov <gleb@kernel.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>
>> Hi guys,
>>
>> I've just started writing a virtual IOMMU for the ARM SMMU and figured a
>> registration mechanism for kvm_device_ops would be a nice cleanup for
>> that.
>>
>> Will
>>
>>  include/linux/kvm_host.h |  1 +
>>  include/uapi/linux/kvm.h |  1 +
>>  virt/kvm/kvm_main.c      | 65 ++++++++++++++++++++++++++++--------------------
>>  3 files changed, 40 insertions(+), 27 deletions(-)
>>
>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index e11d8f170a62..3b368166286f 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>>  #define   KVM_DEV_VFIO_GROUP_DEL			2
>>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
>>  #define KVM_DEV_TYPE_FLIC		6
>> +#define KVM_DEV_TYPE_MAX		7
>
> This means we always need to move this value once we introduce a new
> kvm device type. Can't you keep it in a dynamic list instead of a
> table? We just need to do the lookup during device creation anyway.

There's also this wonderful thing called enum. ;)

It would let Will keep the simpler code with an array, and autogenerate 
KVM_DEV_TYPE_MAX.

>>
>>  /*
>>   * ioctls for VM fds
>
>> +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
>> +{
>> +	if (type >= KVM_DEV_TYPE_MAX)

... then you can make this ARRAY_SIZE, which makes the code quite nice & 
obvious.

Paolo

>> +		return -ENOSPC;
>> +
>> +	if (kvm_device_ops_table[type] != NULL)
>> +		return -EEXIST;
>
> Checking for type collisions would be a bit more expensive with a list,
> but I don't think it matters.
>
>> +
>> +	kvm_device_ops_table[type] = ops;
>> +	return 0;
>> +}
>