Message ID | 1477895706-22824-5-git-send-email-jike.song@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, 31 Oct 2016 14:35:05 +0800 Jike Song <jike.song@intel.com> wrote: Patch title is "set/put" but there is no "put". > A vfio_group may be or may not be attached to a KVM instance, > if it is, the user of vfio_group might also want to know which > KVM instance it is attached to, to utilize features provided > by KVM. In VFIO there are already external APIs for KVM to > get/put the vfio_group, by extending that, KVM can set or clear > itself to/from the vfio_group, for external users to use. > > Signed-off-by: Jike Song <jike.song@intel.com> > --- > drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++ > include/linux/vfio.h | 4 ++++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index e3e58e3..41611cc 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -34,6 +34,7 @@ > #include <linux/uaccess.h> > #include <linux/vfio.h> > #include <linux/wait.h> > +#include <linux/kvm_host.h> > > #define DRIVER_VERSION "0.3" > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" > @@ -86,6 +87,10 @@ struct vfio_group { > struct mutex unbound_lock; > atomic_t opened; > bool noiommu; > + struct { > + struct kvm *kvm; > + struct mutex lock; > + } udata; > }; > > struct vfio_device { > @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) > mutex_init(&group->device_lock); > INIT_LIST_HEAD(&group->unbound_list); > mutex_init(&group->unbound_lock); > + mutex_init(&group->udata.lock); > atomic_set(&group->container_users, 0); > atomic_set(&group->opened, 0); > group->iommu_group = iommu_group; > @@ -1739,6 +1745,30 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > } > EXPORT_SYMBOL_GPL(vfio_external_check_extension); > > +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) > +{ > + mutex_lock(&group->udata.lock); > + group->udata.kvm = kvm; > + mutex_unlock(&group->udata.lock); > +} > +EXPORT_SYMBOL_GPL(vfio_group_set_kvm); > + > +struct kvm *vfio_group_get_kvm(struct vfio_group *group) > +{ > + struct kvm *kvm = NULL; Unnecessary initialization. > + > + mutex_lock(&group->udata.lock); > + > + kvm = group->udata.kvm; > + if (kvm) > + kvm_get_kvm(kvm); > + > + mutex_unlock(&group->udata.lock); > + > + return kvm; > +} > +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); > + How are kvm references acquired through vfio_group_get_kvm() ever released? Can the reference become invalid? The caller may still hold a kvm references, but couldn't the group be detached from one kvm instance and re-attached to another? This seems like an ad-hoc reference that doesn't impose any usage semantics on the caller or release mechanism. Thanks, Alex > /** > * Sub-module support > */ > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index ad9b857..3abd690 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -95,6 +95,10 @@ extern long vfio_external_check_extension(struct vfio_group *group, > extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); > extern void vfio_group_put(struct vfio_group *group); > > +struct kvm; > +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm); > +extern struct kvm *vfio_group_get_kvm(struct vfio_group *group); > + > /* > * Sub-module helpers > */ -- 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 07/11/2016 19:04, Alex Williamson wrote: >> > +struct kvm *vfio_group_get_kvm(struct vfio_group *group) >> > +{ >> > + struct kvm *kvm = NULL; > Unnecessary initialization. > >> > + >> > + mutex_lock(&group->udata.lock); >> > + >> > + kvm = group->udata.kvm; >> > + if (kvm) >> > + kvm_get_kvm(kvm); >> > + >> > + mutex_unlock(&group->udata.lock); >> > + >> > + return kvm; >> > +} >> > +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); > > How are kvm references acquired through vfio_group_get_kvm() ever > released? They are released with kvm_put_kvm, but it's done in the vendor driver so that VFIO core doesn't have a dependency on kvm.ko. > Can the reference become invalid? No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which probably should be renamed...). > The caller may still hold > a kvm references, but couldn't the group be detached from one kvm > instance and re-attached to another? Can this be handled by the vendor driver? Does it get a callback when it's detached from a KVM instance? 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 Mon, 7 Nov 2016 19:10:37 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/11/2016 19:04, Alex Williamson wrote: > >> > +struct kvm *vfio_group_get_kvm(struct vfio_group *group) > >> > +{ > >> > + struct kvm *kvm = NULL; > > Unnecessary initialization. > > > >> > + > >> > + mutex_lock(&group->udata.lock); > >> > + > >> > + kvm = group->udata.kvm; > >> > + if (kvm) > >> > + kvm_get_kvm(kvm); > >> > + > >> > + mutex_unlock(&group->udata.lock); > >> > + > >> > + return kvm; > >> > +} > >> > +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); > > > > How are kvm references acquired through vfio_group_get_kvm() ever > > released? > > They are released with kvm_put_kvm, but it's done in the vendor driver > so that VFIO core doesn't have a dependency on kvm.ko. We could do a symbol_get() to avoid that so we could have a balanced get/put through one interface. > > Can the reference become invalid? > > No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which > probably should be renamed...). The caller gets a reference to kvm, but there's no guarantee that the association of that kvm reference to the group stays valid. Once we're outside of that mutex, we might as well consider that kvm:group association stale. > > The caller may still hold > > a kvm references, but couldn't the group be detached from one kvm > > instance and re-attached to another? > > Can this be handled by the vendor driver? Does it get a callback when > it's detached from a KVM instance? The only release callback through vfio is when the user closes the device, the code in this series is the full extent of vfio awareness of kvm. Thanks, Alex -- 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 07/11/2016 19:28, Alex Williamson wrote: > > > Can the reference become invalid? > > > > No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which > > probably should be renamed...). > > The caller gets a reference to kvm, but there's no guarantee that the > association of that kvm reference to the group stays valid. Once we're > outside of that mutex, we might as well consider that kvm:group > association stale. > > > > The caller may still hold > > > a kvm references, but couldn't the group be detached from one kvm > > > instance and re-attached to another? > > > > Can this be handled by the vendor driver? Does it get a callback when > > it's detached from a KVM instance? > > The only release callback through vfio is when the user closes the > device, the code in this series is the full extent of vfio awareness of > kvm. Thanks, Maybe there should be an mdev callback at the point of association and deassociation between VFIO and KVM. Then the vendor driver can just use the same mutex for association, deassociation and usage. I'm not even sure that these patches are necessary once you have that callback. 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 11/08/2016 02:28 AM, Alex Williamson wrote: > On Mon, 7 Nov 2016 19:10:37 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 07/11/2016 19:04, Alex Williamson wrote: >>>>> +struct kvm *vfio_group_get_kvm(struct vfio_group *group) >>>>> +{ >>>>> + struct kvm *kvm = NULL; >>> Unnecessary initialization. >>> >>>>> + >>>>> + mutex_lock(&group->udata.lock); >>>>> + >>>>> + kvm = group->udata.kvm; >>>>> + if (kvm) >>>>> + kvm_get_kvm(kvm); >>>>> + >>>>> + mutex_unlock(&group->udata.lock); >>>>> + >>>>> + return kvm; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); >>> >>> How are kvm references acquired through vfio_group_get_kvm() ever >>> released? >> >> They are released with kvm_put_kvm, but it's done in the vendor driver >> so that VFIO core doesn't have a dependency on kvm.ko. > > We could do a symbol_get() to avoid that so we could have a balanced > get/put through one interface. > >>> Can the reference become invalid? >> >> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >> probably should be renamed...). > > The caller gets a reference to kvm, but there's no guarantee that the > association of that kvm reference to the group stays valid. Once we're > outside of that mutex, we might as well consider that kvm:group > association stale. > >>> The caller may still hold >>> a kvm references, but couldn't the group be detached from one kvm >>> instance and re-attached to another? >> >> Can this be handled by the vendor driver? Does it get a callback when >> it's detached from a KVM instance? > > The only release callback through vfio is when the user closes the > device, the code in this series is the full extent of vfio awareness of > kvm. Thanks, Hi Alex, Thanks for the comments, I'm composing a notifier chain in vfio-group, hopefully that can address current concerns. However, as for the vfio awareness of kvm, implementing notifiers doesn't seem better for that? Do you think if somehow, we are able to figure out a programmatic method in qemu, to trigger intel vGPU related quirks, would still be a better choice? -- Thanks, Jike -- 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 11/09/2016 10:28 AM, Jike Song wrote: > On 11/08/2016 02:28 AM, Alex Williamson wrote: >> On Mon, 7 Nov 2016 19:10:37 +0100 >> Paolo Bonzini <pbonzini@redhat.com> wrote: >>> On 07/11/2016 19:04, Alex Williamson wrote: >>>>>> +struct kvm *vfio_group_get_kvm(struct vfio_group *group) >>>>>> +{ >>>>>> + struct kvm *kvm = NULL; >>>> Unnecessary initialization. >>>> >>>>>> + >>>>>> + mutex_lock(&group->udata.lock); >>>>>> + >>>>>> + kvm = group->udata.kvm; >>>>>> + if (kvm) >>>>>> + kvm_get_kvm(kvm); >>>>>> + >>>>>> + mutex_unlock(&group->udata.lock); >>>>>> + >>>>>> + return kvm; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); >>>> >>>> How are kvm references acquired through vfio_group_get_kvm() ever >>>> released? >>> >>> They are released with kvm_put_kvm, but it's done in the vendor driver >>> so that VFIO core doesn't have a dependency on kvm.ko. >> >> We could do a symbol_get() to avoid that so we could have a balanced >> get/put through one interface. >> >>>> Can the reference become invalid? >>> >>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >>> probably should be renamed...). >> >> The caller gets a reference to kvm, but there's no guarantee that the >> association of that kvm reference to the group stays valid. Once we're >> outside of that mutex, we might as well consider that kvm:group >> association stale. >> >>>> The caller may still hold >>>> a kvm references, but couldn't the group be detached from one kvm >>>> instance and re-attached to another? >>> >>> Can this be handled by the vendor driver? Does it get a callback when >>> it's detached from a KVM instance? >> >> The only release callback through vfio is when the user closes the >> device, the code in this series is the full extent of vfio awareness of >> kvm. Thanks, > > Hi Alex, > > Thanks for the comments, I'm composing a notifier chain in vfio-group, > hopefully that can address current concerns. > > However, as for the vfio awareness of kvm, implementing notifiers doesn't > seem better for that? Do you think if somehow, we are able to figure out > a programmatic method in qemu, to trigger intel vGPU related quirks, would > still be a better choice? I do not think so,,, communicating VFIO with KVM should be generic as it may have more users in the future except KVMGT. I think notification is worth to try - vendor driver can register its callbacks into vfio-group which get called when KVM binds/unbinds with VFIO -- 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 11/09/2016 10:52 AM, Xiao Guangrong wrote: > > > On 11/09/2016 10:28 AM, Jike Song wrote: >> On 11/08/2016 02:28 AM, Alex Williamson wrote: >>> On Mon, 7 Nov 2016 19:10:37 +0100 >>> Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> On 07/11/2016 19:04, Alex Williamson wrote: >>>>>>> +struct kvm *vfio_group_get_kvm(struct vfio_group *group) >>>>>>> +{ >>>>>>> + struct kvm *kvm = NULL; >>>>> Unnecessary initialization. >>>>> >>>>>>> + >>>>>>> + mutex_lock(&group->udata.lock); >>>>>>> + >>>>>>> + kvm = group->udata.kvm; >>>>>>> + if (kvm) >>>>>>> + kvm_get_kvm(kvm); >>>>>>> + >>>>>>> + mutex_unlock(&group->udata.lock); >>>>>>> + >>>>>>> + return kvm; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); >>>>> >>>>> How are kvm references acquired through vfio_group_get_kvm() ever >>>>> released? >>>> >>>> They are released with kvm_put_kvm, but it's done in the vendor driver >>>> so that VFIO core doesn't have a dependency on kvm.ko. >>> >>> We could do a symbol_get() to avoid that so we could have a balanced >>> get/put through one interface. >>> >>>>> Can the reference become invalid? >>>> >>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >>>> probably should be renamed...). >>> >>> The caller gets a reference to kvm, but there's no guarantee that the >>> association of that kvm reference to the group stays valid. Once we're >>> outside of that mutex, we might as well consider that kvm:group >>> association stale. >>> >>>>> The caller may still hold >>>>> a kvm references, but couldn't the group be detached from one kvm >>>>> instance and re-attached to another? >>>> >>>> Can this be handled by the vendor driver? Does it get a callback when >>>> it's detached from a KVM instance? >>> >>> The only release callback through vfio is when the user closes the >>> device, the code in this series is the full extent of vfio awareness of >>> kvm. Thanks, >> >> Hi Alex, >> >> Thanks for the comments, I'm composing a notifier chain in vfio-group, >> hopefully that can address current concerns. >> >> However, as for the vfio awareness of kvm, implementing notifiers doesn't >> seem better for that? Do you think if somehow, we are able to figure out >> a programmatic method in qemu, to trigger intel vGPU related quirks, would >> still be a better choice? > > I do not think so,,, communicating VFIO with KVM should be generic as it may > have more users in the future except KVMGT. > > I think notification is worth to try - vendor driver can register its > callbacks into vfio-group which get called when KVM binds/unbinds with VFIO > Certainly it's worthy, I agree :-) Just being anxious about how could the kvm awareness in vfio be avoided ... -- Thanks, Jike -- 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/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index e3e58e3..41611cc 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -34,6 +34,7 @@ #include <linux/uaccess.h> #include <linux/vfio.h> #include <linux/wait.h> +#include <linux/kvm_host.h> #define DRIVER_VERSION "0.3" #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" @@ -86,6 +87,10 @@ struct vfio_group { struct mutex unbound_lock; atomic_t opened; bool noiommu; + struct { + struct kvm *kvm; + struct mutex lock; + } udata; }; struct vfio_device { @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) mutex_init(&group->device_lock); INIT_LIST_HEAD(&group->unbound_list); mutex_init(&group->unbound_lock); + mutex_init(&group->udata.lock); atomic_set(&group->container_users, 0); atomic_set(&group->opened, 0); group->iommu_group = iommu_group; @@ -1739,6 +1745,30 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) } EXPORT_SYMBOL_GPL(vfio_external_check_extension); +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) +{ + mutex_lock(&group->udata.lock); + group->udata.kvm = kvm; + mutex_unlock(&group->udata.lock); +} +EXPORT_SYMBOL_GPL(vfio_group_set_kvm); + +struct kvm *vfio_group_get_kvm(struct vfio_group *group) +{ + struct kvm *kvm = NULL; + + mutex_lock(&group->udata.lock); + + kvm = group->udata.kvm; + if (kvm) + kvm_get_kvm(kvm); + + mutex_unlock(&group->udata.lock); + + return kvm; +} +EXPORT_SYMBOL_GPL(vfio_group_get_kvm); + /** * Sub-module support */ diff --git a/include/linux/vfio.h b/include/linux/vfio.h index ad9b857..3abd690 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -95,6 +95,10 @@ extern long vfio_external_check_extension(struct vfio_group *group, extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); extern void vfio_group_put(struct vfio_group *group); +struct kvm; +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm); +extern struct kvm *vfio_group_get_kvm(struct vfio_group *group); + /* * Sub-module helpers */
A vfio_group may be or may not be attached to a KVM instance, if it is, the user of vfio_group might also want to know which KVM instance it is attached to, to utilize features provided by KVM. In VFIO there are already external APIs for KVM to get/put the vfio_group, by extending that, KVM can set or clear itself to/from the vfio_group, for external users to use. Signed-off-by: Jike Song <jike.song@intel.com> --- drivers/vfio/vfio.c | 30 ++++++++++++++++++++++++++++++ include/linux/vfio.h | 4 ++++ 2 files changed, 34 insertions(+)