* [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group
@ 2016-10-31 6:35 Jike Song
2016-10-31 6:35 ` [v3 1/5] vfio: Rearrange functions to get vfio_group from dev Jike Song
` (6 more replies)
0 siblings, 7 replies; 34+ messages in thread
From: Jike Song @ 2016-10-31 6:35 UTC (permalink / raw)
To: pbonzini, alex.williamson, guangrong.xiao
Cc: kwankhede, cjia, kevin.tian, jike.song, kvm
So far KVM and VFIO are mostly transparent to each other.
However, there are users who would rely on them both. For
example, KVMGT relies on VFIO to mediate device operations,
and it also relies on KVM for features such as guest page
tracking. To do that, it needs to know which KVM instance
a vfio_group is attached to.
There is already a kvm_vfio device serving for similar purpose,
this patchset extend it to allow external usrs like KVMGT to
get KVM instance from the vfio_group.
I picked one of Kirti's patchset from:
https://lkml.org/lkml/2016/10/26/1119
for the function to get the vfio_group from a given device.
Changes v3:
- don't touch kvm_put_kvm, vfio won't need it
- fix a race condition
Changes v2:
- As Paolo suggested, drop the type and get/put ops from
vfio, assuming that the data will always be kvm.
Jike Song (4):
vfio: export functions to get vfio_group from device and put it
KVM: move kvm_get_kvm to kvm_host.h
vfio: implement APIs to set/put kvm to/from vfio group
KVM: set/clear kvm to/from vfio group during add/delete
Kirti Wankhede (1):
vfio: Rearrange functions to get vfio_group from dev
drivers/vfio/vfio.c | 57 +++++++++++++++++++++++++++++++++++++++++-------
include/linux/kvm_host.h | 5 ++++-
include/linux/vfio.h | 7 ++++++
virt/kvm/kvm_main.c | 6 -----
virt/kvm/vfio.c | 33 ++++++++++++++++++++++++++++
5 files changed, 93 insertions(+), 15 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 34+ messages in thread* [v3 1/5] vfio: Rearrange functions to get vfio_group from dev 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song @ 2016-10-31 6:35 ` Jike Song 2016-10-31 6:35 ` [v3 2/5] vfio: export functions to get vfio_group from device and put it Jike Song ` (5 subsequent siblings) 6 siblings, 0 replies; 34+ messages in thread From: Jike Song @ 2016-10-31 6:35 UTC (permalink / raw) To: pbonzini, alex.williamson, guangrong.xiao Cc: kwankhede, cjia, kevin.tian, jike.song, kvm From: Kirti Wankhede <kwankhede@nvidia.com> This patch rearranges functions to get vfio_group from device Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> Signed-off-by: Neo Jia <cjia@nvidia.com> Change-Id: I1f93262bdbab75094bc24b087b29da35ba70c4c6 --- drivers/vfio/vfio.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index d1d70e0..23bc86c 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -480,6 +480,21 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) return group; } +static struct vfio_group *vfio_group_get_from_dev(struct device *dev) +{ + struct iommu_group *iommu_group; + struct vfio_group *group; + + iommu_group = iommu_group_get(dev); + if (!iommu_group) + return NULL; + + group = vfio_group_get_from_iommu(iommu_group); + iommu_group_put(iommu_group); + + return group; +} + /** * Device objects - create, release, get, put, search */ @@ -811,16 +826,10 @@ int vfio_add_group_dev(struct device *dev, */ struct vfio_device *vfio_device_get_from_dev(struct device *dev) { - struct iommu_group *iommu_group; struct vfio_group *group; struct vfio_device *device; - iommu_group = iommu_group_get(dev); - if (!iommu_group) - return NULL; - - group = vfio_group_get_from_iommu(iommu_group); - iommu_group_put(iommu_group); + group = vfio_group_get_from_dev(dev); if (!group) return NULL; -- 1.9.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [v3 2/5] vfio: export functions to get vfio_group from device and put it 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song 2016-10-31 6:35 ` [v3 1/5] vfio: Rearrange functions to get vfio_group from dev Jike Song @ 2016-10-31 6:35 ` Jike Song 2016-10-31 6:35 ` [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h Jike Song ` (4 subsequent siblings) 6 siblings, 0 replies; 34+ messages in thread From: Jike Song @ 2016-10-31 6:35 UTC (permalink / raw) To: pbonzini, alex.williamson, guangrong.xiao Cc: kwankhede, cjia, kevin.tian, jike.song, kvm External users might want to get the vfio_group from a given device, and put it after the usage. Signed-off-by: Jike Song <jike.song@intel.com> --- drivers/vfio/vfio.c | 6 ++++-- include/linux/vfio.h | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 23bc86c..e3e58e3 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -414,10 +414,11 @@ static void vfio_group_release(struct kref *kref) iommu_group_put(iommu_group); } -static void vfio_group_put(struct vfio_group *group) +void vfio_group_put(struct vfio_group *group) { kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); } +EXPORT_SYMBOL_GPL(vfio_group_put); /* Assume group_lock or group reference is held */ static void vfio_group_get(struct vfio_group *group) @@ -480,7 +481,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) return group; } -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) +struct vfio_group *vfio_group_get_from_dev(struct device *dev) { struct iommu_group *iommu_group; struct vfio_group *group; @@ -494,6 +495,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) return group; } +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); /** * Device objects - create, release, get, put, search diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 0ecae0b..ad9b857 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -92,6 +92,9 @@ extern void vfio_unregister_iommu_driver( extern long vfio_external_check_extension(struct vfio_group *group, unsigned long arg); +extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); +extern void vfio_group_put(struct vfio_group *group); + /* * Sub-module helpers */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song 2016-10-31 6:35 ` [v3 1/5] vfio: Rearrange functions to get vfio_group from dev Jike Song 2016-10-31 6:35 ` [v3 2/5] vfio: export functions to get vfio_group from device and put it Jike Song @ 2016-10-31 6:35 ` Jike Song 2016-10-31 8:33 ` Paolo Bonzini 2016-10-31 6:35 ` [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group Jike Song ` (3 subsequent siblings) 6 siblings, 1 reply; 34+ messages in thread From: Jike Song @ 2016-10-31 6:35 UTC (permalink / raw) To: pbonzini, alex.williamson, guangrong.xiao Cc: kwankhede, cjia, kevin.tian, jike.song, kvm So that external users like vfio can call it without introducing symbol-level dependency. Signed-off-by: Jike Song <jike.song@intel.com> --- include/linux/kvm_host.h | 5 ++++- virt/kvm/kvm_main.c | 6 ------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 01c0b9c..e1e877af 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -526,7 +526,10 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, struct module *module); void kvm_exit(void); -void kvm_get_kvm(struct kvm *kvm); +static inline void kvm_get_kvm(struct kvm *kvm) +{ + atomic_inc(&kvm->users_count); +} void kvm_put_kvm(struct kvm *kvm); static inline struct kvm_memslots *__kvm_memslots(struct kvm *kvm, int as_id) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 348d6fd..e25359b 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -740,12 +740,6 @@ static void kvm_destroy_vm(struct kvm *kvm) mmdrop(mm); } -void kvm_get_kvm(struct kvm *kvm) -{ - atomic_inc(&kvm->users_count); -} -EXPORT_SYMBOL_GPL(kvm_get_kvm); - void kvm_put_kvm(struct kvm *kvm) { if (atomic_dec_and_test(&kvm->users_count)) -- 1.9.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h 2016-10-31 6:35 ` [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h Jike Song @ 2016-10-31 8:33 ` Paolo Bonzini 0 siblings, 0 replies; 34+ messages in thread From: Paolo Bonzini @ 2016-10-31 8:33 UTC (permalink / raw) To: Jike Song Cc: alex williamson, guangrong xiao, kwankhede, cjia, kevin tian, kvm > So that external users like vfio can call it without introducing > symbol-level dependency. > > Signed-off-by: Jike Song <jike.song@intel.com> > --- > include/linux/kvm_host.h | 5 ++++- > virt/kvm/kvm_main.c | 6 ------ > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 01c0b9c..e1e877af 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -526,7 +526,10 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned > vcpu_align, > struct module *module); > void kvm_exit(void); > > -void kvm_get_kvm(struct kvm *kvm); > +static inline void kvm_get_kvm(struct kvm *kvm) > +{ > + atomic_inc(&kvm->users_count); > +} > void kvm_put_kvm(struct kvm *kvm); > > static inline struct kvm_memslots *__kvm_memslots(struct kvm *kvm, int > as_id) > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 348d6fd..e25359b 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -740,12 +740,6 @@ static void kvm_destroy_vm(struct kvm *kvm) > mmdrop(mm); > } > > -void kvm_get_kvm(struct kvm *kvm) > -{ > - atomic_inc(&kvm->users_count); > -} > -EXPORT_SYMBOL_GPL(kvm_get_kvm); > - > void kvm_put_kvm(struct kvm *kvm) > { > if (atomic_dec_and_test(&kvm->users_count)) > -- > 1.9.1 > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> ^ permalink raw reply [flat|nested] 34+ messages in thread
* [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song ` (2 preceding siblings ...) 2016-10-31 6:35 ` [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h Jike Song @ 2016-10-31 6:35 ` Jike Song 2016-11-07 18:04 ` Alex Williamson 2016-10-31 6:35 ` [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete Jike Song ` (2 subsequent siblings) 6 siblings, 1 reply; 34+ messages in thread From: Jike Song @ 2016-10-31 6:35 UTC (permalink / raw) To: pbonzini, alex.williamson, guangrong.xiao Cc: kwankhede, cjia, kevin.tian, jike.song, kvm 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; + + 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 */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-10-31 6:35 ` [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group Jike Song @ 2016-11-07 18:04 ` Alex Williamson 2016-11-07 18:10 ` Paolo Bonzini 0 siblings, 1 reply; 34+ messages in thread From: Alex Williamson @ 2016-11-07 18:04 UTC (permalink / raw) To: Jike Song; +Cc: pbonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm 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 > */ ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-07 18:04 ` Alex Williamson @ 2016-11-07 18:10 ` Paolo Bonzini 2016-11-07 18:28 ` Alex Williamson 0 siblings, 1 reply; 34+ messages in thread From: Paolo Bonzini @ 2016-11-07 18:10 UTC (permalink / raw) To: Alex Williamson, Jike Song Cc: guangrong.xiao, kwankhede, cjia, kevin.tian, kvm 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-07 18:10 ` Paolo Bonzini @ 2016-11-07 18:28 ` Alex Williamson 2016-11-07 20:45 ` Paolo Bonzini 2016-11-09 2:28 ` Jike Song 0 siblings, 2 replies; 34+ messages in thread From: Alex Williamson @ 2016-11-07 18:28 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Jike Song, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-07 18:28 ` Alex Williamson @ 2016-11-07 20:45 ` Paolo Bonzini 2016-11-09 12:49 ` Jike Song 2016-11-09 2:28 ` Jike Song 1 sibling, 1 reply; 34+ messages in thread From: Paolo Bonzini @ 2016-11-07 20:45 UTC (permalink / raw) To: Alex Williamson Cc: Jike Song, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-07 20:45 ` Paolo Bonzini @ 2016-11-09 12:49 ` Jike Song 2016-11-09 13:06 ` Xiao Guangrong 2016-11-09 17:53 ` Alex Williamson 0 siblings, 2 replies; 34+ messages in thread From: Jike Song @ 2016-11-09 12:49 UTC (permalink / raw) To: Paolo Bonzini Cc: Alex Williamson, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > 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. Hi Alex & Paolo, So I cooked another draft version of this, there is no kvm pointer saved in vfio_group in this version, and notifier will be called on attach/detach, please kindly have a look :-) -- Thanks, Jike diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index ed2361e4..20b5da9 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 mutex lock; + struct blocking_notifier_head notifier; + } 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; @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) iommu_group_put(iommu_group); } -static void vfio_group_put(struct vfio_group *group) +void vfio_group_put(struct vfio_group *group) { kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); } +EXPORT_SYMBOL_GPL(vfio_group_put); /* Assume group_lock or group reference is held */ static void vfio_group_get(struct vfio_group *group) @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) return group; } -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) +struct vfio_group *vfio_group_get_from_dev(struct device *dev) { struct iommu_group *iommu_group; struct vfio_group *group; @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) return group; } +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); /** * Device objects - create, release, get, put, search @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) } EXPORT_SYMBOL_GPL(vfio_external_check_extension); +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&group->udata.notifier, nb); +} +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); + +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); +} +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); + +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)) +{ + mutex_lock(&group->udata.lock); + + fn(kvm); + blocking_notifier_call_chain(&group->udata.notifier, + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); + + mutex_unlock(&group->udata.lock); +} +EXPORT_SYMBOL_GPL(vfio_group_attach_kvm); + +void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)) +{ + mutex_lock(&group->udata.lock); + + blocking_notifier_call_chain(&group->udata.notifier, + VFIO_GROUP_NOTIFY_DETACH_KVM, kvm); + fn(kvm); + + mutex_unlock(&group->udata.lock); +} +EXPORT_SYMBOL_GPL(vfio_group_detach_kvm); + /** * Sub-module support */ diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 87c9afe..4819a45 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -102,6 +102,21 @@ extern void vfio_unregister_iommu_driver( extern long vfio_external_check_extension(struct vfio_group *group, unsigned long arg); +extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); +extern void vfio_group_put(struct vfio_group *group); + +#define VFIO_GROUP_NOTIFY_ATTACH_KVM 1 +#define VFIO_GROUP_NOTIFY_DETACH_KVM 2 +struct kvm; +extern void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)); +extern void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)); +extern int vfio_group_register_notifier(struct vfio_group *group, + struct notifier_block *nb); +extern int vfio_group_unregister_notifier(struct vfio_group *group, + struct notifier_block *nb); + /* * Sub-module helpers */ diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 1dd087d..d889b56 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -60,6 +60,32 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group) symbol_put(vfio_group_put_external_user); } +static void kvm_vfio_group_attach_kvm(struct vfio_group *group, struct kvm_device *dev) +{ + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); + + fn = symbol_get(vfio_group_attach_kvm); + if (!fn) + return; + + fn(group, dev->kvm, kvm_get_kvm); + + symbol_put(vfio_group_attach_kvm); +} + +static void kvm_vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm) +{ + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); + + fn = symbol_get(vfio_group_detach_kvm); + if (!fn) + return; + + fn(group, kvm, kvm_put_kvm); + + symbol_put(vfio_group_detach_kvm); +} + static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) { long (*fn)(struct vfio_group *, unsigned long); @@ -155,6 +181,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) list_add_tail(&kvg->node, &kv->group_list); kvg->vfio_group = vfio_group; + kvm_vfio_group_attach_kvm(vfio_group, dev); + kvm_arch_start_assignment(dev->kvm); mutex_unlock(&kv->lock); @@ -196,6 +224,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) mutex_unlock(&kv->lock); + kvm_vfio_group_detach_kvm(vfio_group, dev->kvm); + kvm_vfio_group_put_external_user(vfio_group); kvm_vfio_update_coherency(dev); @@ -240,6 +270,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) struct kvm_vfio_group *kvg, *tmp; list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { + kvm_vfio_group_detach_kvm(kvg->vfio_group, dev->kvm); kvm_vfio_group_put_external_user(kvg->vfio_group); list_del(&kvg->node); kfree(kvg); ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 12:49 ` Jike Song @ 2016-11-09 13:06 ` Xiao Guangrong 2016-11-09 13:31 ` Paolo Bonzini 2016-11-09 17:53 ` Alex Williamson 1 sibling, 1 reply; 34+ messages in thread From: Xiao Guangrong @ 2016-11-09 13:06 UTC (permalink / raw) To: Jike Song, Paolo Bonzini Cc: Alex Williamson, kwankhede, cjia, kevin.tian, kvm On 11/09/2016 08:49 PM, Jike Song wrote: > +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)) > +{ > + mutex_lock(&group->udata.lock); This lock is needed, please see below. > + > + fn(kvm); > + blocking_notifier_call_chain(&group->udata.notifier, > + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); As this is a callback before KVM releases its last refcount, i do not think vendor driver need to get additional KVM refcount. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 13:06 ` Xiao Guangrong @ 2016-11-09 13:31 ` Paolo Bonzini 2016-11-09 14:00 ` Xiao Guangrong 0 siblings, 1 reply; 34+ messages in thread From: Paolo Bonzini @ 2016-11-09 13:31 UTC (permalink / raw) To: Xiao Guangrong, Jike Song Cc: Alex Williamson, kwankhede, cjia, kevin.tian, kvm On 09/11/2016 14:06, Xiao Guangrong wrote: > > > On 11/09/2016 08:49 PM, Jike Song wrote: > >> +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, >> + void (*fn)(struct kvm *)) >> +{ >> + mutex_lock(&group->udata.lock); > > This lock is needed, please see below. *not* needed I guess. >> + >> + fn(kvm); >> + blocking_notifier_call_chain(&group->udata.notifier, >> + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); > > As this is a callback before KVM releases its last refcount, i do not > think vendor driver need to get additional KVM refcount. The *group* driver doesn't need it indeed. The mdev vendor driver however does, so it will use kvm_get_kvm under its own mutex. That is: - attach kvm mutex_lock(mdev_driver->lock); mdev_driver->kvm = opaque; kvm_get_kvm(mdev_driver->kvm); mutex_unlock(mdev_driver->lock); - detach kvm mutex_lock(mdev_driver->lock); kvm_put_kvm(mdev_driver->kvm); WARN_ON(mdev_driver->kvm != opaque); mdev_driver->kvm = NULL; mutex_unlock(mdev_driver->lock); - use kvm mutex_lock(mdev_driver->lock); kvm = mdev_driver->kvm; ... mutex_unlock(mdev_driver->lock); or if safe: mutex_lock(mdev_driver->lock); kvm = mdev_driver->kvm; kvm_get_kvm(kvm); mutex_unlock(mdev_driver->lock); ... kvm_put_kvm(kvm); Thanks, Paolo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 13:31 ` Paolo Bonzini @ 2016-11-09 14:00 ` Xiao Guangrong 2016-11-09 14:28 ` Paolo Bonzini 2016-11-10 4:13 ` Jike Song 0 siblings, 2 replies; 34+ messages in thread From: Xiao Guangrong @ 2016-11-09 14:00 UTC (permalink / raw) To: Paolo Bonzini, Jike Song Cc: Alex Williamson, kwankhede, cjia, kevin.tian, kvm On 11/09/2016 09:31 PM, Paolo Bonzini wrote: > > > On 09/11/2016 14:06, Xiao Guangrong wrote: >> >> >> On 11/09/2016 08:49 PM, Jike Song wrote: >> >>> +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, >>> + void (*fn)(struct kvm *)) >>> +{ >>> + mutex_lock(&group->udata.lock); >> >> This lock is needed, please see below. > > *not* needed I guess. Yes, indeed. Sorry for the typo. :( > >>> + >>> + fn(kvm); >>> + blocking_notifier_call_chain(&group->udata.notifier, >>> + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); >> >> As this is a callback before KVM releases its last refcount, i do not >> think vendor driver need to get additional KVM refcount. > > The *group* driver doesn't need it indeed. The mdev vendor driver > however does, so it will use kvm_get_kvm under its own mutex. That is: Yes, own mutex is definitely can work.:) It is vendor driver internal operation and it depends on the internal implementation. My idea is that we can make sure the KVM instance is valid before calling DETACH callback. So if we can properly release all the resource associated with the kvm instance in this callback, it is okay without additional kvm ref. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 14:00 ` Xiao Guangrong @ 2016-11-09 14:28 ` Paolo Bonzini 2016-11-10 4:13 ` Jike Song 1 sibling, 0 replies; 34+ messages in thread From: Paolo Bonzini @ 2016-11-09 14:28 UTC (permalink / raw) To: Xiao Guangrong, Jike Song Cc: Alex Williamson, kwankhede, cjia, kevin.tian, kvm On 09/11/2016 15:00, Xiao Guangrong wrote: >> >> The *group* driver doesn't need it indeed. The mdev vendor driver >> however does, so it will use kvm_get_kvm under its own mutex. That is: > > Yes, own mutex is definitely can work.:) It is vendor driver internal > operation and it depends on the internal implementation. > > My idea is that we can make sure the KVM instance is valid before calling > DETACH callback. So if we can properly release all the resource associated > with the kvm instance in this callback, it is okay without additional kvm > ref. That should work if they have their own mutex, but please add a comment. :) Paolo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 14:00 ` Xiao Guangrong 2016-11-09 14:28 ` Paolo Bonzini @ 2016-11-10 4:13 ` Jike Song 1 sibling, 0 replies; 34+ messages in thread From: Jike Song @ 2016-11-10 4:13 UTC (permalink / raw) To: Xiao Guangrong Cc: Paolo Bonzini, Alex Williamson, kwankhede, cjia, kevin.tian, kvm On 11/09/2016 10:00 PM, Xiao Guangrong wrote: > On 11/09/2016 09:31 PM, Paolo Bonzini wrote: >> On 09/11/2016 14:06, Xiao Guangrong wrote: >>> On 11/09/2016 08:49 PM, Jike Song wrote: >>> >>>> +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, >>>> + void (*fn)(struct kvm *)) >>>> +{ >>>> + mutex_lock(&group->udata.lock); >>> >>> This lock is needed, please see below. >> >> *not* needed I guess. > > Yes, indeed. Sorry for the typo. :( > >> >>>> + >>>> + fn(kvm); >>>> + blocking_notifier_call_chain(&group->udata.notifier, >>>> + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); >>> >>> As this is a callback before KVM releases its last refcount, i do not >>> think vendor driver need to get additional KVM refcount. >> >> The *group* driver doesn't need it indeed. The mdev vendor driver >> however does, so it will use kvm_get_kvm under its own mutex. That is: > > Yes, own mutex is definitely can work.:) It is vendor driver internal > operation and it depends on the internal implementation. > > My idea is that we can make sure the KVM instance is valid before calling > DETACH callback. So if we can properly release all the resource associated > with the kvm instance in this callback, it is okay without additional kvm > ref. Yes, the mutex in vfio_group is actually pointless, now I understand and agree. Thanks Guangrong and Polao :) -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 12:49 ` Jike Song 2016-11-09 13:06 ` Xiao Guangrong @ 2016-11-09 17:53 ` Alex Williamson 2016-11-10 4:10 ` Jike Song 2016-11-14 10:19 ` Jike Song 1 sibling, 2 replies; 34+ messages in thread From: Alex Williamson @ 2016-11-09 17:53 UTC (permalink / raw) To: Jike Song; +Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On Wed, 09 Nov 2016 20:49:32 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > > 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. > > Hi Alex & Paolo, > > So I cooked another draft version of this, there is no kvm pointer saved > in vfio_group in this version, and notifier will be called on attach/detach, > please kindly have a look :-) > > > -- > Thanks, > Jike > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index ed2361e4..20b5da9 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 mutex lock; > + struct blocking_notifier_head notifier; > + } 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; > @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) > iommu_group_put(iommu_group); > } > > -static void vfio_group_put(struct vfio_group *group) > +void vfio_group_put(struct vfio_group *group) > { > kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); > } > +EXPORT_SYMBOL_GPL(vfio_group_put); > > /* Assume group_lock or group reference is held */ > static void vfio_group_get(struct vfio_group *group) > @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) > return group; > } > > -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > +struct vfio_group *vfio_group_get_from_dev(struct device *dev) > { > struct iommu_group *iommu_group; > struct vfio_group *group; > @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > > return group; > } > +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); > > /** > * Device objects - create, release, get, put, search > @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > } > EXPORT_SYMBOL_GPL(vfio_external_check_extension); > > +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register(&group->udata.notifier, nb); > +} > +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); > + > +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) > +{ > + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); > +} > +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); Kirti is already adding vfio_register_notifier & vfio_unregister_notifier, these are not exclusive to the iommu, I clarified that in my question that IOVA range invalidation is just one aspect of what that notifier might be used for. The mdev framework also automatically registers and unregisters that notifier around open/release. So, I don't think we want a new notifier, we just want vfio.c to also consume that notifier. So I think this patch needs a few components that build on what Kirti has, 1) we add a blocking_notifier_head per vfio_group and have vfio_{un}regsiter_notifier add and remove that notifier to the group chain, 2) we create a vfio_group_notify() function that the kvm-vfio pseudo device can call via symbol_get, 3) Have kvm-vfio call vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a pointer to the struct kvm (or NULL to unset, we don't need separate set vs unset notifiers). Does that work? Thanks, Alex > + > +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)) > +{ > + mutex_lock(&group->udata.lock); > + > + fn(kvm); > + blocking_notifier_call_chain(&group->udata.notifier, > + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); > + > + mutex_unlock(&group->udata.lock); > +} > +EXPORT_SYMBOL_GPL(vfio_group_attach_kvm); > + > +void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)) > +{ > + mutex_lock(&group->udata.lock); > + > + blocking_notifier_call_chain(&group->udata.notifier, > + VFIO_GROUP_NOTIFY_DETACH_KVM, kvm); > + fn(kvm); > + > + mutex_unlock(&group->udata.lock); > +} > +EXPORT_SYMBOL_GPL(vfio_group_detach_kvm); > + > /** > * Sub-module support > */ > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 87c9afe..4819a45 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -102,6 +102,21 @@ extern void vfio_unregister_iommu_driver( > extern long vfio_external_check_extension(struct vfio_group *group, > unsigned long arg); > > +extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); > +extern void vfio_group_put(struct vfio_group *group); > + > +#define VFIO_GROUP_NOTIFY_ATTACH_KVM 1 > +#define VFIO_GROUP_NOTIFY_DETACH_KVM 2 > +struct kvm; > +extern void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)); > +extern void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)); > +extern int vfio_group_register_notifier(struct vfio_group *group, > + struct notifier_block *nb); > +extern int vfio_group_unregister_notifier(struct vfio_group *group, > + struct notifier_block *nb); > + > /* > * Sub-module helpers > */ > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 1dd087d..d889b56 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -60,6 +60,32 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group) > symbol_put(vfio_group_put_external_user); > } > > +static void kvm_vfio_group_attach_kvm(struct vfio_group *group, struct kvm_device *dev) > +{ > + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); > + > + fn = symbol_get(vfio_group_attach_kvm); > + if (!fn) > + return; > + > + fn(group, dev->kvm, kvm_get_kvm); > + > + symbol_put(vfio_group_attach_kvm); > +} > + > +static void kvm_vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm) > +{ > + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); > + > + fn = symbol_get(vfio_group_detach_kvm); > + if (!fn) > + return; > + > + fn(group, kvm, kvm_put_kvm); > + > + symbol_put(vfio_group_detach_kvm); > +} > + > static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) > { > long (*fn)(struct vfio_group *, unsigned long); > @@ -155,6 +181,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) > list_add_tail(&kvg->node, &kv->group_list); > kvg->vfio_group = vfio_group; > > + kvm_vfio_group_attach_kvm(vfio_group, dev); > + > kvm_arch_start_assignment(dev->kvm); > > mutex_unlock(&kv->lock); > @@ -196,6 +224,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) > > mutex_unlock(&kv->lock); > > + kvm_vfio_group_detach_kvm(vfio_group, dev->kvm); > + > kvm_vfio_group_put_external_user(vfio_group); > > kvm_vfio_update_coherency(dev); > @@ -240,6 +270,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) > struct kvm_vfio_group *kvg, *tmp; > > list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { > + kvm_vfio_group_detach_kvm(kvg->vfio_group, dev->kvm); > kvm_vfio_group_put_external_user(kvg->vfio_group); > list_del(&kvg->node); > kfree(kvg); ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 17:53 ` Alex Williamson @ 2016-11-10 4:10 ` Jike Song 2016-11-10 6:04 ` Jike Song 2016-11-14 10:19 ` Jike Song 1 sibling, 1 reply; 34+ messages in thread From: Jike Song @ 2016-11-10 4:10 UTC (permalink / raw) To: Alex Williamson Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On 11/10/2016 01:53 AM, Alex Williamson wrote: > On Wed, 09 Nov 2016 20:49:32 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>> 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. >> >> Hi Alex & Paolo, >> >> So I cooked another draft version of this, there is no kvm pointer saved >> in vfio_group in this version, and notifier will be called on attach/detach, >> please kindly have a look :-) >> >> >> -- >> Thanks, >> Jike >> >> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >> index ed2361e4..20b5da9 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 mutex lock; >> + struct blocking_notifier_head notifier; >> + } 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; >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >> iommu_group_put(iommu_group); >> } >> >> -static void vfio_group_put(struct vfio_group *group) >> +void vfio_group_put(struct vfio_group *group) >> { >> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >> } >> +EXPORT_SYMBOL_GPL(vfio_group_put); >> >> /* Assume group_lock or group reference is held */ >> static void vfio_group_get(struct vfio_group *group) >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >> return group; >> } >> >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> { >> struct iommu_group *iommu_group; >> struct vfio_group *group; >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> >> return group; >> } >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >> >> /** >> * Device objects - create, release, get, put, search >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >> } >> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >> >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >> + >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > > Kirti is already adding vfio_register_notifier & > vfio_unregister_notifier, these are not exclusive to the iommu, I > clarified that in my question that IOVA range invalidation is just one > aspect of what that notifier might be used for. The mdev framework > also automatically registers and unregisters that notifier around > open/release. So, I don't think we want a new notifier, we just want > vfio.c to also consume that notifier. Unfortunately the kvm:group attaching happens before device opening, so registering the notifier in open() is not functional: the event has disappeared before we start watching it. A possible workaround is, register the notifier in create() instead of open(). That should be functional, but will cause another issue: being able to register a notifier means we have a vfio-group reference, when to put that reference? putting it in remove() is not a good idea since a device might be open/release multiple times between create/remove, holding the ref until removal breaks it; putting it in release() is obviously not a good idea neither. IOW, having the notifiers there must be some dirty work in vendor driver to work around the issue above :( > So I think this patch needs a few components that build on what Kirti > has, 1) we add a blocking_notifier_head per vfio_group and have > vfio_{un}regsiter_notifier add and remove that notifier to the group > chain, 2) we create a vfio_group_notify() function that the kvm-vfio > pseudo device can call via symbol_get, 3) Have kvm-vfio call > vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a > pointer to the struct kvm (or NULL to unset, we don't need separate set > vs unset notifiers). Does that work? Thanks, Yes, it works better than the original form of below patch. vfio side doesn't store any data, nor introduce any lock, only a callback for kvm to use. -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-10 4:10 ` Jike Song @ 2016-11-10 6:04 ` Jike Song 2016-11-10 15:37 ` Alex Williamson 0 siblings, 1 reply; 34+ messages in thread From: Jike Song @ 2016-11-10 6:04 UTC (permalink / raw) To: Alex Williamson Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On 11/10/2016 12:10 PM, Jike Song wrote: > On 11/10/2016 01:53 AM, Alex Williamson wrote: >> On Wed, 09 Nov 2016 20:49:32 +0800 >> Jike Song <jike.song@intel.com> wrote: >> >>> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>>> 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. >>> >>> Hi Alex & Paolo, >>> >>> So I cooked another draft version of this, there is no kvm pointer saved >>> in vfio_group in this version, and notifier will be called on attach/detach, >>> please kindly have a look :-) >>> >>> >>> -- >>> Thanks, >>> Jike >>> >>> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >>> index ed2361e4..20b5da9 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 mutex lock; >>> + struct blocking_notifier_head notifier; >>> + } 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; >>> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >>> iommu_group_put(iommu_group); >>> } >>> >>> -static void vfio_group_put(struct vfio_group *group) >>> +void vfio_group_put(struct vfio_group *group) >>> { >>> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >>> } >>> +EXPORT_SYMBOL_GPL(vfio_group_put); >>> >>> /* Assume group_lock or group reference is held */ >>> static void vfio_group_get(struct vfio_group *group) >>> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >>> return group; >>> } >>> >>> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>> { >>> struct iommu_group *iommu_group; >>> struct vfio_group *group; >>> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>> >>> return group; >>> } >>> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >>> >>> /** >>> * Device objects - create, release, get, put, search >>> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >>> } >>> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >>> >>> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >>> +{ >>> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >>> +} >>> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >>> + >>> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >>> +{ >>> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >>> +} >>> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); >> >> Kirti is already adding vfio_register_notifier & >> vfio_unregister_notifier, these are not exclusive to the iommu, I >> clarified that in my question that IOVA range invalidation is just one >> aspect of what that notifier might be used for. The mdev framework >> also automatically registers and unregisters that notifier around >> open/release. So, I don't think we want a new notifier, we just want >> vfio.c to also consume that notifier. > > Unfortunately the kvm:group attaching happens before device opening, > so registering the notifier in open() is not functional: the event > has disappeared before we start watching it. > > A possible workaround is, register the notifier in create() instead of > open(). That should be functional, but will cause another issue: being able > to register a notifier means we have a vfio-group reference, when to put > that reference? putting it in remove() is not a good idea since a device > might be open/release multiple times between create/remove, holding the ref > until removal breaks it; putting it in release() is obviously not a > good idea neither. > > IOW, having the notifiers there must be some dirty work in vendor > driver to work around the issue above :( > >> So I think this patch needs a few components that build on what Kirti >> has, 1) we add a blocking_notifier_head per vfio_group and have >> vfio_{un}regsiter_notifier add and remove that notifier to the group >> chain, 2) we create a vfio_group_notify() function that the kvm-vfio >> pseudo device can call via symbol_get, 3) Have kvm-vfio call >> vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a >> pointer to the struct kvm (or NULL to unset, we don't need separate set >> vs unset notifiers). Does that work? Thanks, > > Yes, it works better than the original form of below patch. > vfio side doesn't store any data, nor introduce any lock, only a callback > for kvm to use. > To make my reply clearer: the notifier can work without two separate set/unset, can be combined with Kirti's iommu notifier, however, the problem of being too late to register from open() still exists, and I still find it difficult to work around. -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-10 6:04 ` Jike Song @ 2016-11-10 15:37 ` Alex Williamson 2016-11-11 7:29 ` Jike Song 0 siblings, 1 reply; 34+ messages in thread From: Alex Williamson @ 2016-11-10 15:37 UTC (permalink / raw) To: Jike Song; +Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On Thu, 10 Nov 2016 14:04:19 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/10/2016 12:10 PM, Jike Song wrote: > > On 11/10/2016 01:53 AM, Alex Williamson wrote: > >> On Wed, 09 Nov 2016 20:49:32 +0800 > >> Jike Song <jike.song@intel.com> wrote: > >> > >>> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > >>>> 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. > >>> > >>> Hi Alex & Paolo, > >>> > >>> So I cooked another draft version of this, there is no kvm pointer saved > >>> in vfio_group in this version, and notifier will be called on attach/detach, > >>> please kindly have a look :-) > >>> > >>> > >>> -- > >>> Thanks, > >>> Jike > >>> > >>> > >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >>> index ed2361e4..20b5da9 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 mutex lock; > >>> + struct blocking_notifier_head notifier; > >>> + } 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; > >>> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) > >>> iommu_group_put(iommu_group); > >>> } > >>> > >>> -static void vfio_group_put(struct vfio_group *group) > >>> +void vfio_group_put(struct vfio_group *group) > >>> { > >>> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); > >>> } > >>> +EXPORT_SYMBOL_GPL(vfio_group_put); > >>> > >>> /* Assume group_lock or group reference is held */ > >>> static void vfio_group_get(struct vfio_group *group) > >>> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) > >>> return group; > >>> } > >>> > >>> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >>> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >>> { > >>> struct iommu_group *iommu_group; > >>> struct vfio_group *group; > >>> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >>> > >>> return group; > >>> } > >>> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); > >>> > >>> /** > >>> * Device objects - create, release, get, put, search > >>> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > >>> } > >>> EXPORT_SYMBOL_GPL(vfio_external_check_extension); > >>> > >>> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) > >>> +{ > >>> + return blocking_notifier_chain_register(&group->udata.notifier, nb); > >>> +} > >>> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); > >>> + > >>> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) > >>> +{ > >>> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); > >>> +} > >>> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > >> > >> Kirti is already adding vfio_register_notifier & > >> vfio_unregister_notifier, these are not exclusive to the iommu, I > >> clarified that in my question that IOVA range invalidation is just one > >> aspect of what that notifier might be used for. The mdev framework > >> also automatically registers and unregisters that notifier around > >> open/release. So, I don't think we want a new notifier, we just want > >> vfio.c to also consume that notifier. > > > > Unfortunately the kvm:group attaching happens before device opening, > > so registering the notifier in open() is not functional: the event > > has disappeared before we start watching it. > > > > A possible workaround is, register the notifier in create() instead of > > open(). That should be functional, but will cause another issue: being able > > to register a notifier means we have a vfio-group reference, when to put > > that reference? putting it in remove() is not a good idea since a device > > might be open/release multiple times between create/remove, holding the ref > > until removal breaks it; putting it in release() is obviously not a > > good idea neither. > > > > IOW, having the notifiers there must be some dirty work in vendor > > driver to work around the issue above :( > > > >> So I think this patch needs a few components that build on what Kirti > >> has, 1) we add a blocking_notifier_head per vfio_group and have > >> vfio_{un}regsiter_notifier add and remove that notifier to the group > >> chain, 2) we create a vfio_group_notify() function that the kvm-vfio > >> pseudo device can call via symbol_get, 3) Have kvm-vfio call > >> vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a > >> pointer to the struct kvm (or NULL to unset, we don't need separate set > >> vs unset notifiers). Does that work? Thanks, > > > > Yes, it works better than the original form of below patch. > > vfio side doesn't store any data, nor introduce any lock, only a callback > > for kvm to use. > > > > To make my reply clearer: the notifier can work without two separate > set/unset, can be combined with Kirti's iommu notifier, however, the problem > of being too late to register from open() still exists, and I still find it > difficult to work around. Ok, so it's a little bit ugly, but kvm-vfio can tell vfio about struct kvm with a vfio_group_set_kvm() callback that it uses via symbol get, using the real struct kvm pointer or NULL to unset. vfio caches this on the vfio_group. When a notifier is registered, it replays the event through the notifier. Any updates while a notifier is connected are both cached and immediately replayed through the notifier. So the vendor driver to vfio communication channel is the same, but the vfio to kvm-vfio involves some buffering. Does that work? Thanks, Alex ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-10 15:37 ` Alex Williamson @ 2016-11-11 7:29 ` Jike Song 0 siblings, 0 replies; 34+ messages in thread From: Jike Song @ 2016-11-11 7:29 UTC (permalink / raw) To: Alex Williamson Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On 11/10/2016 11:37 PM, Alex Williamson wrote: > On Thu, 10 Nov 2016 14:04:19 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/10/2016 12:10 PM, Jike Song wrote: >>> On 11/10/2016 01:53 AM, Alex Williamson wrote: >>>> On Wed, 09 Nov 2016 20:49:32 +0800 >>>> Jike Song <jike.song@intel.com> wrote: >>>> >>>>> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>>>>> 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. >>>>> >>>>> Hi Alex & Paolo, >>>>> >>>>> So I cooked another draft version of this, there is no kvm pointer saved >>>>> in vfio_group in this version, and notifier will be called on attach/detach, >>>>> please kindly have a look :-) >>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> Jike >>>>> >>>>> >>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >>>>> index ed2361e4..20b5da9 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 mutex lock; >>>>> + struct blocking_notifier_head notifier; >>>>> + } 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; >>>>> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >>>>> iommu_group_put(iommu_group); >>>>> } >>>>> >>>>> -static void vfio_group_put(struct vfio_group *group) >>>>> +void vfio_group_put(struct vfio_group *group) >>>>> { >>>>> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >>>>> } >>>>> +EXPORT_SYMBOL_GPL(vfio_group_put); >>>>> >>>>> /* Assume group_lock or group reference is held */ >>>>> static void vfio_group_get(struct vfio_group *group) >>>>> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >>>>> return group; >>>>> } >>>>> >>>>> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>>>> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>>>> { >>>>> struct iommu_group *iommu_group; >>>>> struct vfio_group *group; >>>>> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>>>> >>>>> return group; >>>>> } >>>>> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >>>>> >>>>> /** >>>>> * Device objects - create, release, get, put, search >>>>> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >>>>> } >>>>> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >>>>> >>>>> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >>>>> +{ >>>>> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >>>>> + >>>>> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >>>>> +{ >>>>> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); >>>> >>>> Kirti is already adding vfio_register_notifier & >>>> vfio_unregister_notifier, these are not exclusive to the iommu, I >>>> clarified that in my question that IOVA range invalidation is just one >>>> aspect of what that notifier might be used for. The mdev framework >>>> also automatically registers and unregisters that notifier around >>>> open/release. So, I don't think we want a new notifier, we just want >>>> vfio.c to also consume that notifier. >>> >>> Unfortunately the kvm:group attaching happens before device opening, >>> so registering the notifier in open() is not functional: the event >>> has disappeared before we start watching it. >>> >>> A possible workaround is, register the notifier in create() instead of >>> open(). That should be functional, but will cause another issue: being able >>> to register a notifier means we have a vfio-group reference, when to put >>> that reference? putting it in remove() is not a good idea since a device >>> might be open/release multiple times between create/remove, holding the ref >>> until removal breaks it; putting it in release() is obviously not a >>> good idea neither. >>> >>> IOW, having the notifiers there must be some dirty work in vendor >>> driver to work around the issue above :( >>> >>>> So I think this patch needs a few components that build on what Kirti >>>> has, 1) we add a blocking_notifier_head per vfio_group and have >>>> vfio_{un}regsiter_notifier add and remove that notifier to the group >>>> chain, 2) we create a vfio_group_notify() function that the kvm-vfio >>>> pseudo device can call via symbol_get, 3) Have kvm-vfio call >>>> vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a >>>> pointer to the struct kvm (or NULL to unset, we don't need separate set >>>> vs unset notifiers). Does that work? Thanks, >>> >>> Yes, it works better than the original form of below patch. >>> vfio side doesn't store any data, nor introduce any lock, only a callback >>> for kvm to use. >>> >> >> To make my reply clearer: the notifier can work without two separate >> set/unset, can be combined with Kirti's iommu notifier, however, the problem >> of being too late to register from open() still exists, and I still find it >> difficult to work around. > > Ok, so it's a little bit ugly, but kvm-vfio can tell vfio about struct > kvm with a vfio_group_set_kvm() callback that it uses via symbol get, > using the real struct kvm pointer or NULL to unset. vfio caches this > on the vfio_group. When a notifier is registered, it replays the event > through the notifier. Any updates while a notifier is connected are > both cached and immediately replayed through the notifier. So the > vendor driver to vfio communication channel is the same, but the vfio > to kvm-vfio involves some buffering. Does that work? Thanks, Yes it works, thanks for the suggestion, I'll post a v4 series. -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 17:53 ` Alex Williamson 2016-11-10 4:10 ` Jike Song @ 2016-11-14 10:19 ` Jike Song 2016-11-14 15:52 ` Alex Williamson 1 sibling, 1 reply; 34+ messages in thread From: Jike Song @ 2016-11-14 10:19 UTC (permalink / raw) To: Alex Williamson Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On 11/10/2016 01:53 AM, Alex Williamson wrote: > On Wed, 09 Nov 2016 20:49:32 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>> 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. >> >> Hi Alex & Paolo, >> >> So I cooked another draft version of this, there is no kvm pointer saved >> in vfio_group in this version, and notifier will be called on attach/detach, >> please kindly have a look :-) >> >> >> -- >> Thanks, >> Jike >> >> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >> index ed2361e4..20b5da9 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 mutex lock; >> + struct blocking_notifier_head notifier; >> + } 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; >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >> iommu_group_put(iommu_group); >> } >> >> -static void vfio_group_put(struct vfio_group *group) >> +void vfio_group_put(struct vfio_group *group) >> { >> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >> } >> +EXPORT_SYMBOL_GPL(vfio_group_put); >> >> /* Assume group_lock or group reference is held */ >> static void vfio_group_get(struct vfio_group *group) >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >> return group; >> } >> >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> { >> struct iommu_group *iommu_group; >> struct vfio_group *group; >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> >> return group; >> } >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >> >> /** >> * Device objects - create, release, get, put, search >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >> } >> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >> >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >> + >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > > Kirti is already adding vfio_register_notifier & > vfio_unregister_notifier, these are not exclusive to the iommu, I > clarified that in my question that IOVA range invalidation is just one > aspect of what that notifier might be used for. The mdev framework > also automatically registers and unregisters that notifier around > open/release. So, I don't think we want a new notifier, we just want > vfio.c to also consume that notifier. > Hi Alex, Sorry, I have one more question: does combining Kirti's iommu notifier and my group notifier mean there should only one blocking_notifier_head? If so, where should it be? vfio_container, vfio_group or vfio_iommu? -- Thanks, Jike > So I think this patch needs a few components that build on what Kirti > has, 1) we add a blocking_notifier_head per vfio_group and have > vfio_{un}regsiter_notifier add and remove that notifier to the group > chain, 2) we create a vfio_group_notify() function that the kvm-vfio > pseudo device can call via symbol_get, 3) Have kvm-vfio call > vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a > pointer to the struct kvm (or NULL to unset, we don't need separate set > vs unset notifiers). Does that work? Thanks, > > Alex > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-14 10:19 ` Jike Song @ 2016-11-14 15:52 ` Alex Williamson 0 siblings, 0 replies; 34+ messages in thread From: Alex Williamson @ 2016-11-14 15:52 UTC (permalink / raw) To: Jike Song; +Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On Mon, 14 Nov 2016 18:19:34 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/10/2016 01:53 AM, Alex Williamson wrote: > > On Wed, 09 Nov 2016 20:49:32 +0800 > > Jike Song <jike.song@intel.com> wrote: > > > >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > >>> 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. > >> > >> Hi Alex & Paolo, > >> > >> So I cooked another draft version of this, there is no kvm pointer saved > >> in vfio_group in this version, and notifier will be called on attach/detach, > >> please kindly have a look :-) > >> > >> > >> -- > >> Thanks, > >> Jike > >> > >> > >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >> index ed2361e4..20b5da9 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 mutex lock; > >> + struct blocking_notifier_head notifier; > >> + } 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; > >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) > >> iommu_group_put(iommu_group); > >> } > >> > >> -static void vfio_group_put(struct vfio_group *group) > >> +void vfio_group_put(struct vfio_group *group) > >> { > >> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); > >> } > >> +EXPORT_SYMBOL_GPL(vfio_group_put); > >> > >> /* Assume group_lock or group reference is held */ > >> static void vfio_group_get(struct vfio_group *group) > >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) > >> return group; > >> } > >> > >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >> { > >> struct iommu_group *iommu_group; > >> struct vfio_group *group; > >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >> > >> return group; > >> } > >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); > >> > >> /** > >> * Device objects - create, release, get, put, search > >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > >> } > >> EXPORT_SYMBOL_GPL(vfio_external_check_extension); > >> > >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) > >> +{ > >> + return blocking_notifier_chain_register(&group->udata.notifier, nb); > >> +} > >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); > >> + > >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) > >> +{ > >> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); > >> +} > >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > > > > Kirti is already adding vfio_register_notifier & > > vfio_unregister_notifier, these are not exclusive to the iommu, I > > clarified that in my question that IOVA range invalidation is just one > > aspect of what that notifier might be used for. The mdev framework > > also automatically registers and unregisters that notifier around > > open/release. So, I don't think we want a new notifier, we just want > > vfio.c to also consume that notifier. > > > > Hi Alex, > > Sorry, I have one more question: does combining Kirti's iommu notifier > and my group notifier mean there should only one blocking_notifier_head? > If so, where should it be? vfio_container, vfio_group or vfio_iommu? I suspect the most straightforward approach is to place a blocking_notifier_head on the vfio_group in addition to the one that Kirti has placed on the vfio_iommu. Both will include the same notifier_block from the vendor driver and call the notifier chain independently. Thanks, Alex ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-07 18:28 ` Alex Williamson 2016-11-07 20:45 ` Paolo Bonzini @ 2016-11-09 2:28 ` Jike Song 2016-11-09 2:52 ` Xiao Guangrong 1 sibling, 1 reply; 34+ messages in thread From: Jike Song @ 2016-11-09 2:28 UTC (permalink / raw) To: Alex Williamson Cc: Paolo Bonzini, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 2:28 ` Jike Song @ 2016-11-09 2:52 ` Xiao Guangrong 2016-11-09 3:07 ` Jike Song 0 siblings, 1 reply; 34+ messages in thread From: Xiao Guangrong @ 2016-11-09 2:52 UTC (permalink / raw) To: Jike Song, Alex Williamson Cc: Paolo Bonzini, kwankhede, cjia, kevin.tian, kvm 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group 2016-11-09 2:52 ` Xiao Guangrong @ 2016-11-09 3:07 ` Jike Song 0 siblings, 0 replies; 34+ messages in thread From: Jike Song @ 2016-11-09 3:07 UTC (permalink / raw) To: Xiao Guangrong Cc: Alex Williamson, Paolo Bonzini, kwankhede, cjia, kevin.tian, kvm 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song ` (3 preceding siblings ...) 2016-10-31 6:35 ` [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group Jike Song @ 2016-10-31 6:35 ` Jike Song 2016-10-31 8:33 ` Paolo Bonzini 2016-10-31 7:06 ` [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Xiao Guangrong 2016-11-02 1:06 ` Jike Song 6 siblings, 1 reply; 34+ messages in thread From: Jike Song @ 2016-10-31 6:35 UTC (permalink / raw) To: pbonzini, alex.williamson, guangrong.xiao Cc: kwankhede, cjia, kevin.tian, jike.song, kvm With VFIO being capable of carrying usrdata, we can extend the existing interfaces between KVM and VFIO, whenever a group is added to or deleted from KVM, set/clear KVM instance to/from this group. That enables 3rd-party users of vfio_group to know which KVM it attached to, if there is one. Signed-off-by: Jike Song <jike.song@intel.com> --- virt/kvm/vfio.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 1dd087d..62da226 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -60,6 +60,34 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group) symbol_put(vfio_group_put_external_user); } +static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) +{ + void (*fn)(struct vfio_group *, struct kvm *); + + fn = symbol_get(vfio_group_set_kvm); + if (!fn) + return; + + fn(group, kvm); + kvm_get_kvm(kvm); + + symbol_put(vfio_group_set_kvm); +} + +static void kvm_vfio_group_clear_kvm(struct vfio_group *group, struct kvm *kvm) +{ + void (*fn)(struct vfio_group *, struct kvm *); + + fn = symbol_get(vfio_group_set_kvm); + if (!fn) + return; + + fn(group, NULL); + kvm_put_kvm(kvm); + + symbol_put(vfio_group_set_kvm); +} + static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) { long (*fn)(struct vfio_group *, unsigned long); @@ -155,6 +183,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) list_add_tail(&kvg->node, &kv->group_list); kvg->vfio_group = vfio_group; + kvm_vfio_group_set_kvm(vfio_group, dev->kvm); + kvm_arch_start_assignment(dev->kvm); mutex_unlock(&kv->lock); @@ -196,6 +226,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) mutex_unlock(&kv->lock); + kvm_vfio_group_clear_kvm(vfio_group, dev->kvm); + kvm_vfio_group_put_external_user(vfio_group); kvm_vfio_update_coherency(dev); @@ -240,6 +272,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) struct kvm_vfio_group *kvg, *tmp; list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { + kvm_vfio_group_clear_kvm(kvg->vfio_group, dev->kvm); kvm_vfio_group_put_external_user(kvg->vfio_group); list_del(&kvg->node); kfree(kvg); -- 1.9.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete 2016-10-31 6:35 ` [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete Jike Song @ 2016-10-31 8:33 ` Paolo Bonzini 0 siblings, 0 replies; 34+ messages in thread From: Paolo Bonzini @ 2016-10-31 8:33 UTC (permalink / raw) To: Jike Song Cc: alex williamson, guangrong xiao, kwankhede, cjia, kevin tian, kvm > With VFIO being capable of carrying usrdata, we can extend the > existing interfaces between KVM and VFIO, whenever a group is > added to or deleted from KVM, set/clear KVM instance to/from > this group. That enables 3rd-party users of vfio_group to know > which KVM it attached to, if there is one. > > Signed-off-by: Jike Song <jike.song@intel.com> > --- > virt/kvm/vfio.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 1dd087d..62da226 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -60,6 +60,34 @@ static void kvm_vfio_group_put_external_user(struct > vfio_group *vfio_group) > symbol_put(vfio_group_put_external_user); > } > > +static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm > *kvm) > +{ > + void (*fn)(struct vfio_group *, struct kvm *); > + > + fn = symbol_get(vfio_group_set_kvm); > + if (!fn) > + return; > + > + fn(group, kvm); > + kvm_get_kvm(kvm); > + > + symbol_put(vfio_group_set_kvm); > +} > + > +static void kvm_vfio_group_clear_kvm(struct vfio_group *group, struct kvm > *kvm) > +{ > + void (*fn)(struct vfio_group *, struct kvm *); > + > + fn = symbol_get(vfio_group_set_kvm); > + if (!fn) > + return; > + > + fn(group, NULL); > + kvm_put_kvm(kvm); > + > + symbol_put(vfio_group_set_kvm); > +} > + > static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) > { > long (*fn)(struct vfio_group *, unsigned long); > @@ -155,6 +183,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, > long attr, u64 arg) > list_add_tail(&kvg->node, &kv->group_list); > kvg->vfio_group = vfio_group; > > + kvm_vfio_group_set_kvm(vfio_group, dev->kvm); > + > kvm_arch_start_assignment(dev->kvm); > > mutex_unlock(&kv->lock); > @@ -196,6 +226,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, > long attr, u64 arg) > > mutex_unlock(&kv->lock); > > + kvm_vfio_group_clear_kvm(vfio_group, dev->kvm); > + > kvm_vfio_group_put_external_user(vfio_group); > > kvm_vfio_update_coherency(dev); > @@ -240,6 +272,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) > struct kvm_vfio_group *kvg, *tmp; > > list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { > + kvm_vfio_group_clear_kvm(kvg->vfio_group, dev->kvm); > kvm_vfio_group_put_external_user(kvg->vfio_group); > list_del(&kvg->node); > kfree(kvg); > -- > 1.9.1 > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song ` (4 preceding siblings ...) 2016-10-31 6:35 ` [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete Jike Song @ 2016-10-31 7:06 ` Xiao Guangrong 2016-10-31 7:24 ` Jike Song 2016-11-02 1:06 ` Jike Song 6 siblings, 1 reply; 34+ messages in thread From: Xiao Guangrong @ 2016-10-31 7:06 UTC (permalink / raw) To: Jike Song, pbonzini, alex.williamson; +Cc: kwankhede, cjia, kevin.tian, kvm On 10/31/2016 02:35 PM, Jike Song wrote: > So far KVM and VFIO are mostly transparent to each other. > However, there are users who would rely on them both. For > example, KVMGT relies on VFIO to mediate device operations, > and it also relies on KVM for features such as guest page > tracking. To do that, it needs to know which KVM instance > a vfio_group is attached to. > > There is already a kvm_vfio device serving for similar purpose, > this patchset extend it to allow external usrs like KVMGT to > get KVM instance from the vfio_group. > > > I picked one of Kirti's patchset from: > > https://lkml.org/lkml/2016/10/26/1119 > > for the function to get the vfio_group from a given device. > > > Changes v3: > - don't touch kvm_put_kvm, vfio won't need it Do not understand. vfio does not use it indeed, however, the user of udata, i.e, KVMGT, should put kvm after vfio_group_get_kvm(). ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group 2016-10-31 7:06 ` [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Xiao Guangrong @ 2016-10-31 7:24 ` Jike Song 2016-10-31 7:24 ` Xiao Guangrong 0 siblings, 1 reply; 34+ messages in thread From: Jike Song @ 2016-10-31 7:24 UTC (permalink / raw) To: Xiao Guangrong Cc: pbonzini, alex.williamson, kwankhede, cjia, kevin.tian, kvm On 10/31/2016 03:06 PM, Xiao Guangrong wrote: > > > On 10/31/2016 02:35 PM, Jike Song wrote: >> So far KVM and VFIO are mostly transparent to each other. >> However, there are users who would rely on them both. For >> example, KVMGT relies on VFIO to mediate device operations, >> and it also relies on KVM for features such as guest page >> tracking. To do that, it needs to know which KVM instance >> a vfio_group is attached to. >> >> There is already a kvm_vfio device serving for similar purpose, >> this patchset extend it to allow external usrs like KVMGT to >> get KVM instance from the vfio_group. >> >> >> I picked one of Kirti's patchset from: >> >> https://lkml.org/lkml/2016/10/26/1119 >> >> for the function to get the vfio_group from a given device. >> >> >> Changes v3: >> - don't touch kvm_put_kvm, vfio won't need it > > Do not understand. vfio does not use it indeed, however, the > user of udata, i.e, KVMGT, should put kvm after vfio_group_get_kvm(). Yes, but I guess KVMGT can has symbol-level dependency on KVM :-) -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group 2016-10-31 7:24 ` Jike Song @ 2016-10-31 7:24 ` Xiao Guangrong 2016-10-31 7:30 ` Jike Song 0 siblings, 1 reply; 34+ messages in thread From: Xiao Guangrong @ 2016-10-31 7:24 UTC (permalink / raw) To: Jike Song; +Cc: pbonzini, alex.williamson, kwankhede, cjia, kevin.tian, kvm On 10/31/2016 03:24 PM, Jike Song wrote: > On 10/31/2016 03:06 PM, Xiao Guangrong wrote: >> >> >> On 10/31/2016 02:35 PM, Jike Song wrote: >>> So far KVM and VFIO are mostly transparent to each other. >>> However, there are users who would rely on them both. For >>> example, KVMGT relies on VFIO to mediate device operations, >>> and it also relies on KVM for features such as guest page >>> tracking. To do that, it needs to know which KVM instance >>> a vfio_group is attached to. >>> >>> There is already a kvm_vfio device serving for similar purpose, >>> this patchset extend it to allow external usrs like KVMGT to >>> get KVM instance from the vfio_group. >>> >>> >>> I picked one of Kirti's patchset from: >>> >>> https://lkml.org/lkml/2016/10/26/1119 >>> >>> for the function to get the vfio_group from a given device. >>> >>> >>> Changes v3: >>> - don't touch kvm_put_kvm, vfio won't need it >> >> Do not understand. vfio does not use it indeed, however, the >> user of udata, i.e, KVMGT, should put kvm after vfio_group_get_kvm(). > > Yes, but I guess KVMGT can has symbol-level dependency on KVM :-) Anyway, kvm_put_kvm() should be exported? ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group 2016-10-31 7:24 ` Xiao Guangrong @ 2016-10-31 7:30 ` Jike Song 2016-10-31 7:35 ` Xiao Guangrong 0 siblings, 1 reply; 34+ messages in thread From: Jike Song @ 2016-10-31 7:30 UTC (permalink / raw) To: Xiao Guangrong Cc: pbonzini, alex.williamson, kwankhede, cjia, kevin.tian, kvm On 10/31/2016 03:24 PM, Xiao Guangrong wrote: > On 10/31/2016 03:24 PM, Jike Song wrote: >> On 10/31/2016 03:06 PM, Xiao Guangrong wrote: >>> >>> >>> On 10/31/2016 02:35 PM, Jike Song wrote: >>>> So far KVM and VFIO are mostly transparent to each other. >>>> However, there are users who would rely on them both. For >>>> example, KVMGT relies on VFIO to mediate device operations, >>>> and it also relies on KVM for features such as guest page >>>> tracking. To do that, it needs to know which KVM instance >>>> a vfio_group is attached to. >>>> >>>> There is already a kvm_vfio device serving for similar purpose, >>>> this patchset extend it to allow external usrs like KVMGT to >>>> get KVM instance from the vfio_group. >>>> >>>> >>>> I picked one of Kirti's patchset from: >>>> >>>> https://lkml.org/lkml/2016/10/26/1119 >>>> >>>> for the function to get the vfio_group from a given device. >>>> >>>> >>>> Changes v3: >>>> - don't touch kvm_put_kvm, vfio won't need it >>> >>> Do not understand. vfio does not use it indeed, however, the >>> user of udata, i.e, KVMGT, should put kvm after vfio_group_get_kvm(). >> >> Yes, but I guess KVMGT can has symbol-level dependency on KVM :-) > > Anyway, kvm_put_kvm() should be exported? Yes, current KVM already exported it, so no need to change :-) -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group 2016-10-31 7:30 ` Jike Song @ 2016-10-31 7:35 ` Xiao Guangrong 0 siblings, 0 replies; 34+ messages in thread From: Xiao Guangrong @ 2016-10-31 7:35 UTC (permalink / raw) To: Jike Song; +Cc: pbonzini, alex.williamson, kwankhede, cjia, kevin.tian, kvm On 10/31/2016 03:30 PM, Jike Song wrote: > On 10/31/2016 03:24 PM, Xiao Guangrong wrote: >> On 10/31/2016 03:24 PM, Jike Song wrote: >>> On 10/31/2016 03:06 PM, Xiao Guangrong wrote: >>>> >>>> >>>> On 10/31/2016 02:35 PM, Jike Song wrote: >>>>> So far KVM and VFIO are mostly transparent to each other. >>>>> However, there are users who would rely on them both. For >>>>> example, KVMGT relies on VFIO to mediate device operations, >>>>> and it also relies on KVM for features such as guest page >>>>> tracking. To do that, it needs to know which KVM instance >>>>> a vfio_group is attached to. >>>>> >>>>> There is already a kvm_vfio device serving for similar purpose, >>>>> this patchset extend it to allow external usrs like KVMGT to >>>>> get KVM instance from the vfio_group. >>>>> >>>>> >>>>> I picked one of Kirti's patchset from: >>>>> >>>>> https://lkml.org/lkml/2016/10/26/1119 >>>>> >>>>> for the function to get the vfio_group from a given device. >>>>> >>>>> >>>>> Changes v3: >>>>> - don't touch kvm_put_kvm, vfio won't need it >>>> >>>> Do not understand. vfio does not use it indeed, however, the >>>> user of udata, i.e, KVMGT, should put kvm after vfio_group_get_kvm(). >>> >>> Yes, but I guess KVMGT can has symbol-level dependency on KVM :-) >> >> Anyway, kvm_put_kvm() should be exported? > > Yes, current KVM already exported it, so no need to change :-) Ah, sorry, i just briefly followed the changelog without check the code, then it makes sense. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song ` (5 preceding siblings ...) 2016-10-31 7:06 ` [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Xiao Guangrong @ 2016-11-02 1:06 ` Jike Song 6 siblings, 0 replies; 34+ messages in thread From: Jike Song @ 2016-11-02 1:06 UTC (permalink / raw) To: Jike Song Cc: pbonzini, alex.williamson, guangrong.xiao, kwankhede, cjia, kevin.tian, kvm On 10/31/2016 02:35 PM, Jike Song wrote: > So far KVM and VFIO are mostly transparent to each other. > However, there are users who would rely on them both. For > example, KVMGT relies on VFIO to mediate device operations, > and it also relies on KVM for features such as guest page > tracking. To do that, it needs to know which KVM instance > a vfio_group is attached to. > > There is already a kvm_vfio device serving for similar purpose, > this patchset extend it to allow external usrs like KVMGT to > get KVM instance from the vfio_group. > > > I picked one of Kirti's patchset from: > > https://lkml.org/lkml/2016/10/26/1119 > > for the function to get the vfio_group from a given device. > > > Changes v3: > - don't touch kvm_put_kvm, vfio won't need it > - fix a race condition > Changes v2: > - As Paolo suggested, drop the type and get/put ops from > vfio, assuming that the data will always be kvm. > > Jike Song (4): > vfio: export functions to get vfio_group from device and put it > KVM: move kvm_get_kvm to kvm_host.h > vfio: implement APIs to set/put kvm to/from vfio group > KVM: set/clear kvm to/from vfio group during add/delete > > Kirti Wankhede (1): > vfio: Rearrange functions to get vfio_group from dev > > drivers/vfio/vfio.c | 57 +++++++++++++++++++++++++++++++++++++++++------- > include/linux/kvm_host.h | 5 ++++- > include/linux/vfio.h | 7 ++++++ > virt/kvm/kvm_main.c | 6 ----- > virt/kvm/vfio.c | 33 ++++++++++++++++++++++++++++ > 5 files changed, 93 insertions(+), 15 deletions(-) Hi Alex, Are you okay with changes in this series? Since the patches for vfio and kvm patches are interleaved and Paolo has acked the kvm ones, I guess you will pick up them? :-) -- Thanks, Jike ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2016-11-14 15:52 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-31 6:35 [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Jike Song 2016-10-31 6:35 ` [v3 1/5] vfio: Rearrange functions to get vfio_group from dev Jike Song 2016-10-31 6:35 ` [v3 2/5] vfio: export functions to get vfio_group from device and put it Jike Song 2016-10-31 6:35 ` [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h Jike Song 2016-10-31 8:33 ` Paolo Bonzini 2016-10-31 6:35 ` [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group Jike Song 2016-11-07 18:04 ` Alex Williamson 2016-11-07 18:10 ` Paolo Bonzini 2016-11-07 18:28 ` Alex Williamson 2016-11-07 20:45 ` Paolo Bonzini 2016-11-09 12:49 ` Jike Song 2016-11-09 13:06 ` Xiao Guangrong 2016-11-09 13:31 ` Paolo Bonzini 2016-11-09 14:00 ` Xiao Guangrong 2016-11-09 14:28 ` Paolo Bonzini 2016-11-10 4:13 ` Jike Song 2016-11-09 17:53 ` Alex Williamson 2016-11-10 4:10 ` Jike Song 2016-11-10 6:04 ` Jike Song 2016-11-10 15:37 ` Alex Williamson 2016-11-11 7:29 ` Jike Song 2016-11-14 10:19 ` Jike Song 2016-11-14 15:52 ` Alex Williamson 2016-11-09 2:28 ` Jike Song 2016-11-09 2:52 ` Xiao Guangrong 2016-11-09 3:07 ` Jike Song 2016-10-31 6:35 ` [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete Jike Song 2016-10-31 8:33 ` Paolo Bonzini 2016-10-31 7:06 ` [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group Xiao Guangrong 2016-10-31 7:24 ` Jike Song 2016-10-31 7:24 ` Xiao Guangrong 2016-10-31 7:30 ` Jike Song 2016-10-31 7:35 ` Xiao Guangrong 2016-11-02 1:06 ` Jike Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.