| Message ID | 1404225918-8903-1-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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). > > include/linux/kvm_host.h | 1 + > include/uapi/linux/kvm.h | 22 +++++++++++----- > virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++-------------------- > 3 files changed, 55 insertions(+), 33 deletions(-) > > 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. > +}; > > /* > * ioctls for VM fds -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2 Jul 2014 10:32:41 +0100 Will Deacon <will.deacon@arm.com> wrote: > 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: > > > 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? I don't really have a strong opinion on that, so Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 01, 2014 at 03:45:15PM +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. > > 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> > --- > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ec4e3bd83d47..b75faaf0d76d 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1083,6 +1083,7 @@ struct kvm_device_ops { void kvm_device_get(struct kvm_device *dev); void kvm_device_put(struct kvm_device *dev); struct kvm_device *kvm_device_from_filp(struct file *filp); +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type); extern struct kvm_device_ops kvm_mpic_ops; extern struct kvm_device_ops kvm_xics_ops; 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, +}; /* * ioctls for VM fds diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4b6c01b477f9..a8539ae10247 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2257,44 +2257,55 @@ struct kvm_device *kvm_device_from_filp(struct file *filp) return filp->private_data; } -static int kvm_ioctl_create_device(struct kvm *kvm, - struct kvm_create_device *cd) -{ - struct kvm_device_ops *ops = NULL; - struct kvm_device *dev; - bool test = cd->flags & KVM_CREATE_DEVICE_TEST; - int ret; - - switch (cd->type) { +static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = { #ifdef CONFIG_KVM_MPIC - case KVM_DEV_TYPE_FSL_MPIC_20: - case KVM_DEV_TYPE_FSL_MPIC_42: - ops = &kvm_mpic_ops; - break; + [KVM_DEV_TYPE_FSL_MPIC_20] = &kvm_mpic_ops, + [KVM_DEV_TYPE_FSL_MPIC_42] = &kvm_mpic_ops, #endif + #ifdef CONFIG_KVM_XICS - case KVM_DEV_TYPE_XICS: - ops = &kvm_xics_ops; - break; + [KVM_DEV_TYPE_XICS] = &kvm_xics_ops, #endif + #ifdef CONFIG_KVM_VFIO - case KVM_DEV_TYPE_VFIO: - ops = &kvm_vfio_ops; - break; + [KVM_DEV_TYPE_VFIO] = &kvm_vfio_ops, #endif + #ifdef CONFIG_KVM_ARM_VGIC - case KVM_DEV_TYPE_ARM_VGIC_V2: - ops = &kvm_arm_vgic_v2_ops; - break; + [KVM_DEV_TYPE_ARM_VGIC_V2] = &kvm_arm_vgic_v2_ops, #endif + #ifdef CONFIG_S390 - case KVM_DEV_TYPE_FLIC: - ops = &kvm_flic_ops; - break; + [KVM_DEV_TYPE_FLIC] = &kvm_flic_ops, #endif - default: +}; + +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type) +{ + if (type >= ARRAY_SIZE(kvm_device_ops_table)) + return -ENOSPC; + + if (kvm_device_ops_table[type] != NULL) + return -EEXIST; + + kvm_device_ops_table[type] = ops; + return 0; +} + +static int kvm_ioctl_create_device(struct kvm *kvm, + struct kvm_create_device *cd) +{ + struct kvm_device_ops *ops = NULL; + struct kvm_device *dev; + bool test = cd->flags & KVM_CREATE_DEVICE_TEST; + int ret; + + if (cd->type >= ARRAY_SIZE(kvm_device_ops_table)) + return -ENODEV; + + ops = kvm_device_ops_table[cd->type]; + if (ops == NULL) return -ENODEV; - } if (test) return 0;
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. 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. include/linux/kvm_host.h | 1 + include/uapi/linux/kvm.h | 22 +++++++++++----- virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++-------------------- 3 files changed, 55 insertions(+), 33 deletions(-)