[v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Jike Song]

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

[v3 1/5] vfio: Rearrange functions to get vfio_group from dev

[Jike Song]

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

[v3 2/5] vfio: export functions to get vfio_group from device and put it

[Jike Song]

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

[v3 3/5] KVM: move kvm_get_kvm to kvm_host.h

[Jike Song]

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

[v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

[v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete

[Jike Song]

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

Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Xiao Guangrong]

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

Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Xiao Guangrong]

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?

Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Jike Song]

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

Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Jike Song]

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

Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Xiao Guangrong]

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.

Re: [v3 3/5] KVM: move kvm_get_kvm to kvm_host.h

[Paolo Bonzini]

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

Re: [v3 5/5] KVM: set/clear kvm to/from vfio group during add/delete

[Paolo Bonzini]

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

Re: [v3 0/5] plumb kvm/vfio to allow getting kvm from vfio_group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Alex Williamson]

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
>   */

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Paolo Bonzini]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Alex Williamson]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Paolo Bonzini]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Xiao Guangrong]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Xiao Guangrong]

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.

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Paolo Bonzini]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Xiao Guangrong]

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.

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Paolo Bonzini]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Alex Williamson]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Alex Williamson]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Jike Song]

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
> 

Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Alex Williamson]

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