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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon, 30 Jun 2014 19:22:55 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 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.

Or this :)

It means we statically grow the table with each new device type, but we
probably don't want bazillions of kvm device types anyway.