From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH 1/2] KVM: device: add simple registration mechanism for kvm_device_ops Date: Mon, 30 Jun 2014 11:26:48 +0100 Message-ID: <20140630102648.GC25779@arm.com> References: <1403803817-22140-1-git-send-email-will.deacon@arm.com> <20140630112114.2f185502.cornelia.huck@de.ibm.com> <20140630093619.GF24879@arm.com> <20140630122501.509dac09.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" , Gleb Natapov , Paolo Bonzini , Marc Zyngier , Christoffer Dall To: Cornelia Huck Return-path: Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:63263 "EHLO cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753301AbaF3K0z (ORCPT ); Mon, 30 Jun 2014 06:26:55 -0400 Content-Disposition: inline In-Reply-To: <20140630122501.509dac09.cornelia.huck@de.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jun 30, 2014 at 11:25:01AM +0100, Cornelia Huck wrote: > On Mon, 30 Jun 2014 10:36:19 +0100 > Will Deacon wrote: > > On Mon, Jun 30, 2014 at 10:21:14AM +0100, Cornelia Huck wrote: > > > On Thu, 26 Jun 2014 18:30:16 +0100 > > > Will Deacon wrote: > > > > 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. > > > > Well, we do need the fixed IDs in order for userspace to create these > > devices via the ioctl. If it's the fixed size you're worried about, the > > easiest thing is to replace the array with an idr. I actually started off > > with that, but it felt a bit overkill (since we never need dynamic ID > > allocation). I can bring it back if you prefer? > > > > At the end of the day, we can't get around the fact that the IDs need to > > added with some caution (e.g. not assigning an ID twice). > > Ah, just to make this clear, I was only worried about the _MAX value; > the other ids obviously need to be known by userspace :) Agreed. > So what this basically boils down to is an internal implementation > detail: Do we keep a static table that needs to be grown each time we > add a new device type, or do we keep a dynamic list that can have a > variable number of entries? The dynamic list appeals to me, but it is > slightly more complex code, and we probably won't be adding new device > types left and right anyway. So if your idr code is short and sweet, > I'd say go for it, but not if we end up with a mountain of code. Okey doke, I'll take a look for v2 (I need to remove a stray semi-colon from the patch anyway). Thanks for the feedback, Will