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

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

Hello,

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

Great, thanks.

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

> > +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
> > +{
> > +	if (type >= KVM_DEV_TYPE_MAX)
> > +		return -ENOSPC;
> > +
> > +	if (kvm_device_ops_table[type] != NULL)
> > +		return -EEXIST;
> 
> Checking for type collisions would be a bit more expensive with a list,
> but I don't think it matters.

I'd be *really* surprised if this was a fast-path in KVM!

Will