From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH v2 1/4] KVM: device: add simple registration mechanism for kvm_device_ops Date: Wed, 2 Jul 2014 10:32:41 +0100 Message-ID: <20140702093241.GD18731@arm.com> References: <1404225918-8903-1-git-send-email-will.deacon@arm.com> <20140702111720.28506538.cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "kvm@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , "Alex.Williamson@redhat.com" , "agraf@suse.de" , "gleb@kernel.org" , "pbonzini@redhat.com" , Marc Zyngier , "christoffer.dall@linaro.org" To: Cornelia Huck Return-path: Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:33020 "EHLO cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751205AbaGBJcv (ORCPT ); Wed, 2 Jul 2014 05:32:51 -0400 Content-Disposition: inline In-Reply-To: <20140702111720.28506538.cornelia.huck@de.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jul 02, 2014 at 10:17:20AM +0100, Cornelia Huck wrote: > On Tue, 1 Jul 2014 15:45:15 +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 think that last paragraph should rather go into a cover letter :) > > > > > Cc: Cornelia Huck > > Cc: Alex Williamson > > Cc: Alex Graf > > Cc: Gleb Natapov > > Cc: Paolo Bonzini > > Cc: Marc Zyngier > > Cc: Christoffer Dall > > Signed-off-by: Will Deacon > > --- > > > > 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