| Message ID | 1403803817-22140-1-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 26/06/2014 19:30, Will Deacon ha scritto: > 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. Conny, Alex (Graf & Williamson), can you help Will here? The idea looks sane, but I'd rather merge it with all devices converted. Paolo -- 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 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. > > 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> > --- > > Hi guys, > > I've just started writing a virtual IOMMU for the ARM SMMU and figured a > registration mechanism for kvm_device_ops would be a nice cleanup for > that. > > Will > > include/linux/kvm_host.h | 1 + > include/uapi/linux/kvm.h | 1 + > virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++-------------------- > 3 files changed, 40 insertions(+), 27 deletions(-) > > 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. > > /* > * ioctls for VM fds > +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. > + > + kvm_device_ops_table[type] = ops; > + return 0; > +} -- 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
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 -- 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 Mon, 30 Jun 2014 10:36:19 +0100 Will Deacon <will.deacon@arm.com> wrote: > 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: > > > 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 :) 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. -- 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 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 -- 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 Fri, Jun 27, 2014 at 03:17:57PM +0100, Paolo Bonzini wrote: > Il 26/06/2014 19:30, Will Deacon ha scritto: > > 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. > > Conny, Alex (Graf & Williamson), > > can you help Will here? The idea looks sane, but I'd rather merge it > with all devices converted. Also, if we want to use an IDR instead of an array then dynamic registration will be required, since I can't initialise the former statically. I can convert kvm_vfio_ops with an initcall, but the others (mpic, xics and flic) should probably hang off the host irqchip initialisation, no? All help appreciated! I'm happy to collate diffs :) Failing that, I'll have a crack at it myself, but I've got no way of testing the result. 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
Il 30/06/2014 11:21, Cornelia Huck ha scritto: > 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. > >> >> 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> >> --- >> >> Hi guys, >> >> I've just started writing a virtual IOMMU for the ARM SMMU and figured a >> registration mechanism for kvm_device_ops would be a nice cleanup for >> that. >> >> Will >> >> include/linux/kvm_host.h | 1 + >> include/uapi/linux/kvm.h | 1 + >> virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++-------------------- >> 3 files changed, 40 insertions(+), 27 deletions(-) >> > >> 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. There's also this wonderful thing called enum. ;) It would let Will keep the simpler code with an array, and autogenerate KVM_DEV_TYPE_MAX. >> >> /* >> * ioctls for VM fds > >> +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type) >> +{ >> + if (type >= KVM_DEV_TYPE_MAX) ... then you can make this ARRAY_SIZE, which makes the code quite nice & obvious. Paolo >> + 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. > >> + >> + kvm_device_ops_table[type] = ops; >> + return 0; >> +} > -- 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 Mon, 30 Jun 2014 19:22:55 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 30/06/2014 11:21, Cornelia Huck ha scritto: > > 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. > > > >> > >> 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> > >> --- > >> > >> Hi guys, > >> > >> I've just started writing a virtual IOMMU for the ARM SMMU and figured a > >> registration mechanism for kvm_device_ops would be a nice cleanup for > >> that. > >> > >> Will > >> > >> include/linux/kvm_host.h | 1 + > >> include/uapi/linux/kvm.h | 1 + > >> virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++-------------------- > >> 3 files changed, 40 insertions(+), 27 deletions(-) > >> > > > >> 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. > > There's also this wonderful thing called enum. ;) > > It would let Will keep the simpler code with an array, and autogenerate > KVM_DEV_TYPE_MAX. Or this :) It means we statically grow the table with each new device type, but we probably don't want bazillions of kvm device types anyway. -- 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 Mon, Jun 30, 2014 at 06:22:55PM +0100, Paolo Bonzini wrote: > Il 30/06/2014 11:21, Cornelia Huck ha scritto: > > 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. > > There's also this wonderful thing called enum. ;) > > It would let Will keep the simpler code with an array, and autogenerate > KVM_DEV_TYPE_MAX. Although this is uapi, so we may need to #define the symbols anyway to avoid breaking userspace #ifndef tests. What do you reckon; is this an ABI break? 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
Il 30/06/2014 19:31, Will Deacon ha scritto: >> > It would let Will keep the simpler code with an array, and autogenerate >> > KVM_DEV_TYPE_MAX. > Although this is uapi, so we may need to #define the symbols anyway to avoid > breaking userspace #ifndef tests. > > What do you reckon; is this an ABI break? Yes, it would be an API break. But you can also do this: #define FOO FOO I think there are other occurrences of this in uapi/. Paolo -- 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..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 /* * ioctls for VM fds diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4b6c01b477f9..a0c6cc31a1f6 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 >= KVM_DEV_TYPE_MAX) + 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 >= KVM_DEV_TYPE_MAX) + 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: 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> --- Hi guys, I've just started writing a virtual IOMMU for the ARM SMMU and figured a registration mechanism for kvm_device_ops would be a nice cleanup for that. Will include/linux/kvm_host.h | 1 + include/uapi/linux/kvm.h | 1 + virt/kvm/kvm_main.c | 65 ++++++++++++++++++++++++++++-------------------- 3 files changed, 40 insertions(+), 27 deletions(-)