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

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

On Mon, Jun 30, 2014 at 11:25:01AM +0100, Cornelia Huck wrote:
> On Mon, 30 Jun 2014 10:36:19 +0100
> Will Deacon <will.deacon@arm.com> 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 <will.deacon@arm.com> 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