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

[Will Deacon <will.deacon@arm.com>]

On Wed, Jul 02, 2014 at 10:17:20AM +0100, Cornelia Huck wrote:
> On Tue,  1 Jul 2014 15:45:15 +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 think that last paragraph should rather go into a cover letter :)
> 
> > 
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Alex Williamson <Alex.Williamson@redhat.com>
> > Cc: Alex Graf <agraf@suse.de>
> > 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>
> > ---
> > 
> > v1 -> v2: Added enum for KVM_DEV_TYPE* IDs, changed limits to ARRAY_SIZE,
> >           removed stray semicolon, had a crack at porting VFIO, included
> > 	  Cornelia's s390 FLIC patch.
> 
> ...and the changelog as well (or keep changelogs for individual
> patches).

Yeah... this has grown to be bigger than one patch now. I can include that
for v3.

> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index e11d8f170a62..6875cc225dff 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -940,15 +940,25 @@ struct kvm_device_attr {
> >  	__u64	addr;		/* userspace address of attr data */
> >  };
> > 
> > -#define KVM_DEV_TYPE_FSL_MPIC_20	1
> > -#define KVM_DEV_TYPE_FSL_MPIC_42	2
> > -#define KVM_DEV_TYPE_XICS		3
> > -#define KVM_DEV_TYPE_VFIO		4
> >  #define  KVM_DEV_VFIO_GROUP			1
> >  #define   KVM_DEV_VFIO_GROUP_ADD			1
> >  #define   KVM_DEV_VFIO_GROUP_DEL			2
> > -#define KVM_DEV_TYPE_ARM_VGIC_V2	5
> > -#define KVM_DEV_TYPE_FLIC		6
> > +
> > +enum kvm_device_type {
> > +	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> > +#define KVM_DEV_TYPE_FSL_MPIC_20	KVM_DEV_TYPE_FSL_MPIC_20
> > +	KVM_DEV_TYPE_FSL_MPIC_42,
> > +#define KVM_DEV_TYPE_FSL_MPIC_42	KVM_DEV_TYPE_FSL_MPIC_42
> > +	KVM_DEV_TYPE_XICS,
> > +#define KVM_DEV_TYPE_XICS		KVM_DEV_TYPE_XICS
> > +	KVM_DEV_TYPE_VFIO,
> > +#define KVM_DEV_TYPE_VFIO		KVM_DEV_TYPE_VFIO
> > +	KVM_DEV_TYPE_ARM_VGIC_V2,
> > +#define KVM_DEV_TYPE_ARM_VGIC_V2	KVM_DEV_TYPE_ARM_VGIC_V2
> > +	KVM_DEV_TYPE_FLIC,
> > +#define KVM_DEV_TYPE_FLIC		KVM_DEV_TYPE_FLIC
> > +	KVM_DEV_TYPE_MAX,
> 
> Do you want to add the individual values to the enum? A removal of a
> type would be an API break (so we should be safe against renumbering),
> but it's sometimes helpful if one can get the numeric value at a glance.

Could do, but then I think the advantage of the enum is questionable over
the #defines, since you could just as easily have two entries in the enum
with the same ID value as forgetting to update a KVM_DEV_TYPE_MAX #define
(which was the reason for the enum in the first place).

So I'd be inclined to keep the patch as-is, unless you have really strong
objections?

Will