From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops Date: Mon, 30 Jun 2014 19:22:55 +0200 Message-ID: <53B19CEF.6080702@redhat.com> References: <1403803817-22140-1-git-send-email-will.deacon@arm.com> <20140630112114.2f185502.cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Gleb Natapov , Marc Zyngier , Christoffer Dall To: Cornelia Huck , Will Deacon Return-path: Received: from mx1.redhat.com ([209.132.183.28]:5334 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751184AbaF3RXR (ORCPT ); Mon, 30 Jun 2014 13:23:17 -0400 In-Reply-To: <20140630112114.2f185502.cornelia.huck@de.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 30/06/2014 11:21, Cornelia Huck ha scritto: > On Thu, 26 Jun 2014 18:30:16 +0100 > Will Deacon 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 >> Cc: Paolo Bonzini >> Cc: Marc Zyngier >> Cc: Christoffer Dall >> Signed-off-by: Will Deacon >> --- >> >> 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; >> +} >