[v7 0/3] plumb kvm/vfio to notify kvm:group attach/detach

[v7 0/3] plumb kvm/vfio to notify kvm:group attach/detach

[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, when it will be detached.

There is already a kvm_vfio device serving for similar
purpose, this series extends it to allow users register
notifiers on a vfio_group, and notify kvm:group attaching
and detaching events to that.


Changes v7:
	- replace vfio->opened with container user like iommu notifier registration
	- fix a typo
Changes v6:
	- only allow registration on already opened vfio_group 
	- moved WARN_ON from vfio_group_release to vfio_group_fops_release
	- remove unnecessary check
Changes v5:
	- separate vfio_group and vfio_iommu events from each other
	- force user to specify which type it is interested in, and which events.
	  Register it IFF all required events are supported
Changes v4:
	- use a notifier-based implementation in vfio_group
	- combine the notifier along with iommu notifier by Kirti
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 (3):
  vfio: vfio_register_notifier: classify iommu notifier
  vfio: support notifier chain in vfio_group
  kvm: set/clear kvm to/from vfio_group when group add/delete

 drivers/vfio/vfio.c  | 167 ++++++++++++++++++++++++++++++++++++++++++++-------
 include/linux/vfio.h |  22 ++++++-
 virt/kvm/vfio.c      |  31 ++++++++++
 3 files changed, 195 insertions(+), 25 deletions(-)

-- 
1.9.1

[v7 1/3] vfio: vfio_register_notifier: classify iommu notifier

[Jike Song]

Currently vfio_register_notifier assumes that there is only one
notifier chain, which is in vfio_iommu. However, the user might
also be interested in events other than vfio_iommu, for example,
vfio_group. Refactor vfio_{un}register_notifier implementation
to make it feasible.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/vfio/vfio.c  | 85 ++++++++++++++++++++++++++++++++++++++--------------
 include/linux/vfio.h | 16 ++++++++--
 2 files changed, 76 insertions(+), 25 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 0aac3ca..493bbad 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2008,23 +2008,24 @@ int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage)
 }
 EXPORT_SYMBOL(vfio_unpin_pages);
 
-int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
+static int vfio_register_iommu_notifier(struct vfio_group *group,
+					unsigned long *events,
+					struct notifier_block *nb)
 {
 	struct vfio_container *container;
-	struct vfio_group *group;
 	struct vfio_iommu_driver *driver;
 	int ret;
 
-	if (!dev || !nb)
-		return -EINVAL;
+	/* clear known events */
+	*events &= ~VFIO_IOMMU_NOTIFY_DMA_UNMAP;
 
-	group = vfio_group_get_from_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
+	/* refuse to register if still events remaining */
+	if (*events)
+		return -EINVAL;
 
 	ret = vfio_group_add_container_user(group);
 	if (ret)
-		goto err_register_nb;
+		return -EINVAL;
 
 	container = group->container;
 	down_read(&container->group_lock);
@@ -2038,29 +2039,19 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
 	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
-err_register_nb:
-	vfio_group_put(group);
 	return ret;
 }
-EXPORT_SYMBOL(vfio_register_notifier);
 
-int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
+static int vfio_unregister_iommu_notifier(struct vfio_group *group,
+					  struct notifier_block *nb)
 {
 	struct vfio_container *container;
-	struct vfio_group *group;
 	struct vfio_iommu_driver *driver;
 	int ret;
 
-	if (!dev || !nb)
-		return -EINVAL;
-
-	group = vfio_group_get_from_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
 	ret = vfio_group_add_container_user(group);
 	if (ret)
-		goto err_unregister_nb;
+		return -EINVAL;
 
 	container = group->container;
 	down_read(&container->group_lock);
@@ -2075,7 +2066,57 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
 	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
-err_unregister_nb:
+	return ret;
+}
+
+int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
+			   unsigned long *events,
+			   struct notifier_block *nb)
+{
+	struct vfio_group *group;
+	int ret;
+
+	if (!dev || !nb || !events || (*events == 0))
+		return -EINVAL;
+
+	group = vfio_group_get_from_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	switch (type) {
+	case VFIO_IOMMU_NOTIFY:
+		ret = vfio_register_iommu_notifier(group, events, nb);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	vfio_group_put(group);
+	return ret;
+}
+EXPORT_SYMBOL(vfio_register_notifier);
+
+int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type,
+			     struct notifier_block *nb)
+{
+	struct vfio_group *group;
+	int ret;
+
+	if (!dev || !nb)
+		return -EINVAL;
+
+	group = vfio_group_get_from_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	switch (type) {
+	case VFIO_IOMMU_NOTIFY:
+		ret = vfio_unregister_iommu_notifier(group, nb);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
 	vfio_group_put(group);
 	return ret;
 }
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 15ff042..6f3ff31 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -107,14 +107,24 @@ extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
 extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
 			    int npage);
 
-#define VFIO_IOMMU_NOTIFY_DMA_UNMAP	(1)
-
+typedef unsigned short vfio_notify_type_t;
 extern int vfio_register_notifier(struct device *dev,
+				  vfio_notify_type_t type,
+				  unsigned long *required_events,
 				  struct notifier_block *nb);
-
 extern int vfio_unregister_notifier(struct device *dev,
+				    vfio_notify_type_t type,
 				    struct notifier_block *nb);
 
+/* each type has independent events */
+enum vfio_notify_type {
+	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
+};
+
+/* events for VFIO_IOMMU_NOTIFY */
+#define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
+
+
 /*
  * Sub-module helpers
  */
-- 
1.9.1

[v7 2/3] vfio: support notifier chain in vfio_group

[Jike Song]

Beyond vfio_iommu events, users might also be interested in
vfio_group events. For example, if a vfio_group is used along
with Qemu/KVM, whenever kvm pointer is set to/cleared from the
vfio_group, users could be notified.

Currently only VFIO_GROUP_NOTIFY_SET_KVM supported.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/vfio/vfio.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  6 ++++
 2 files changed, 88 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 493bbad..3ab5edb 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -86,6 +86,8 @@ struct vfio_group {
 	struct mutex			unbound_lock;
 	atomic_t			opened;
 	bool				noiommu;
+	struct kvm			*kvm;
+	struct blocking_notifier_head	notifier;
 };
 
 struct vfio_device {
@@ -339,6 +341,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
 #ifdef CONFIG_VFIO_NOIOMMU
 	group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu);
 #endif
+	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
 
 	group->nb.notifier_call = vfio_iommu_group_notifier;
 
@@ -1581,6 +1584,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 
 	filep->private_data = NULL;
 
+	/* Any user didn't unregister? */
+	WARN_ON(group->notifier.head);
+
 	vfio_group_try_dissolve_container(group);
 
 	atomic_dec(&group->opened);
@@ -2069,6 +2075,76 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
 	return ret;
 }
 
+void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
+{
+	group->kvm = kvm;
+	blocking_notifier_call_chain(&group->notifier,
+				VFIO_GROUP_NOTIFY_SET_KVM, kvm);
+}
+EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
+
+static int vfio_register_group_notifier(struct vfio_group *group,
+					unsigned long *events,
+					struct notifier_block *nb)
+{
+	struct vfio_container *container;
+	int ret;
+	bool set_kvm = false;
+
+	if (*events & VFIO_GROUP_NOTIFY_SET_KVM)
+		set_kvm = true;
+
+	/* clear known events */
+	*events &= ~VFIO_GROUP_NOTIFY_SET_KVM;
+
+	/* refuse to continue if still events remaining */
+	if (*events)
+		return -EINVAL;
+
+	ret = vfio_group_add_container_user(group);
+	if (ret)
+		return -EINVAL;
+
+	container = group->container;
+	down_read(&container->group_lock);
+
+	ret = blocking_notifier_chain_register(&group->notifier, nb);
+
+	/*
+	 * The attaching of kvm and vfio_group might already happen, so
+	 * here we replay once upon registration.
+	 */
+	if (!ret && set_kvm && group->kvm)
+		blocking_notifier_call_chain(&group->notifier,
+					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
+
+	up_read(&container->group_lock);
+	vfio_group_try_dissolve_container(group);
+
+	return ret;
+}
+
+static int vfio_unregister_group_notifier(struct vfio_group *group,
+					 struct notifier_block *nb)
+{
+	struct vfio_container *container;
+	int ret;
+
+	ret = vfio_group_add_container_user(group);
+	if (ret)
+		return -EINVAL;
+
+	container = group->container;
+	down_read(&container->group_lock);
+
+	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
+
+	up_read(&container->group_lock);
+	vfio_group_try_dissolve_container(group);
+
+	return ret;
+}
+
 int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
 			   unsigned long *events,
 			   struct notifier_block *nb)
@@ -2087,6 +2163,9 @@ int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
 	case VFIO_IOMMU_NOTIFY:
 		ret = vfio_register_iommu_notifier(group, events, nb);
 		break;
+	case VFIO_GROUP_NOTIFY:
+		ret = vfio_register_group_notifier(group, events, nb);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -2113,6 +2192,9 @@ int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type,
 	case VFIO_IOMMU_NOTIFY:
 		ret = vfio_unregister_iommu_notifier(group, nb);
 		break;
+	case VFIO_GROUP_NOTIFY:
+		ret = vfio_unregister_group_notifier(group, nb);
+		break;
 	default:
 		ret = -EINVAL;
 	}
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 6f3ff31..5d46e3c 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -119,11 +119,17 @@ extern int vfio_unregister_notifier(struct device *dev,
 /* each type has independent events */
 enum vfio_notify_type {
 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
+	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
 };
 
 /* events for VFIO_IOMMU_NOTIFY */
 #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
 
+/* events for VFIO_GROUP_NOTIFY */
+#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
+
+struct kvm;
+extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
 
 /*
  * Sub-module helpers
-- 
1.9.1

[v7 3/3] kvm: set/clear kvm to/from vfio_group when group add/delete

[Jike Song]

Sometimes users need to be aware when a vfio_group attaches to a
KVM or detaches from it. KVM already calls get/put method from vfio to
manipulate the vfio_group reference, it can notify vfio_group in
a similar way.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 virt/kvm/vfio.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 1dd087d..ea5b4bb 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_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);
+
+	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);
+
+	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 +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_set_kvm(vfio_group, dev->kvm);
+
 		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_clear_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_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: [v7 2/3] vfio: support notifier chain in vfio_group

[Kirti Wankhede]

On 11/22/2016 11:39 AM, Jike Song wrote:
> Beyond vfio_iommu events, users might also be interested in
> vfio_group events. For example, if a vfio_group is used along
> with Qemu/KVM, whenever kvm pointer is set to/cleared from the
> vfio_group, users could be notified.
> 
> Currently only VFIO_GROUP_NOTIFY_SET_KVM supported.
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/vfio/vfio.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/vfio.h |  6 ++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 493bbad..3ab5edb 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -86,6 +86,8 @@ struct vfio_group {
>  	struct mutex			unbound_lock;
>  	atomic_t			opened;
>  	bool				noiommu;
> +	struct kvm			*kvm;
> +	struct blocking_notifier_head	notifier;
>  };
>  
>  struct vfio_device {
> @@ -339,6 +341,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
>  #ifdef CONFIG_VFIO_NOIOMMU
>  	group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu);
>  #endif
> +	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
>  
>  	group->nb.notifier_call = vfio_iommu_group_notifier;
>  
> @@ -1581,6 +1584,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
>  
>  	filep->private_data = NULL;
>  
> +	/* Any user didn't unregister? */
> +	WARN_ON(group->notifier.head);
> +
>  	vfio_group_try_dissolve_container(group);
>  
>  	atomic_dec(&group->opened);
> @@ -2069,6 +2075,76 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
>  	return ret;
>  }
>  
> +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> +{
> +	group->kvm = kvm;
> +	blocking_notifier_call_chain(&group->notifier,
> +				VFIO_GROUP_NOTIFY_SET_KVM, kvm);
> +}
> +EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
> +
> +static int vfio_register_group_notifier(struct vfio_group *group,
> +					unsigned long *events,
> +					struct notifier_block *nb)
> +{
> +	struct vfio_container *container;
> +	int ret;
> +	bool set_kvm = false;
> +
> +	if (*events & VFIO_GROUP_NOTIFY_SET_KVM)
> +		set_kvm = true;
> +
> +	/* clear known events */
> +	*events &= ~VFIO_GROUP_NOTIFY_SET_KVM;
> +
> +	/* refuse to continue if still events remaining */
> +	if (*events)
> +		return -EINVAL;
> +
> +	ret = vfio_group_add_container_user(group);
> +	if (ret)
> +		return -EINVAL;
> +
> +	container = group->container;
> +	down_read(&container->group_lock);
> +
> +	ret = blocking_notifier_chain_register(&group->notifier, nb);
> +
> +	/*
> +	 * The attaching of kvm and vfio_group might already happen, so
> +	 * here we replay once upon registration.
> +	 */
> +	if (!ret && set_kvm && group->kvm)
> +		blocking_notifier_call_chain(&group->notifier,
> +					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
> +
> +	up_read(&container->group_lock);
> +	vfio_group_try_dissolve_container(group);
> +
> +	return ret;
> +}
> +
> +static int vfio_unregister_group_notifier(struct vfio_group *group,
> +					 struct notifier_block *nb)
> +{
> +	struct vfio_container *container;
> +	int ret;
> +
> +	ret = vfio_group_add_container_user(group);
> +	if (ret)
> +		return -EINVAL;
> +
> +	container = group->container;
> +	down_read(&container->group_lock);
> +
> +	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
> +
> +	up_read(&container->group_lock);
> +	vfio_group_try_dissolve_container(group);
> +
> +	return ret;
> +}
> +
>  int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
>  			   unsigned long *events,
>  			   struct notifier_block *nb)
> @@ -2087,6 +2163,9 @@ int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
>  	case VFIO_IOMMU_NOTIFY:
>  		ret = vfio_register_iommu_notifier(group, events, nb);
>  		break;
> +	case VFIO_GROUP_NOTIFY:
> +		ret = vfio_register_group_notifier(group, events, nb);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -2113,6 +2192,9 @@ int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type,
>  	case VFIO_IOMMU_NOTIFY:
>  		ret = vfio_unregister_iommu_notifier(group, nb);
>  		break;
> +	case VFIO_GROUP_NOTIFY:
> +		ret = vfio_unregister_group_notifier(group, nb);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 6f3ff31..5d46e3c 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -119,11 +119,17 @@ extern int vfio_unregister_notifier(struct device *dev,
>  /* each type has independent events */
>  enum vfio_notify_type {
>  	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
> +	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
>  };
>  
>  /* events for VFIO_IOMMU_NOTIFY */
>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
>  
> +/* events for VFIO_GROUP_NOTIFY */
> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
> +
> +struct kvm;
> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
>  

If I understand correctly, notifier for backend iommu driver and
notifier for group should be registered with different notifier blocks,
as per your current implementation:

unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
struct notifier_block iommu_nb;

iommu_nb.notifier_call = vfio_iommu_notifier_cb;
vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);


unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
struct notifier_block group_nb;

group_nb.notifier_call = vfio_group_notifier_cb;
vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);

Then what is the use of having 'events' bitmask as input argument?

Its clear that caller should set:
-  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
-  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY

Notifier action could be a integer as earlier and indexing of action
would be different for both notifiers.

Then caller don't have to sent event bitmask, for caller it would look like:

struct notifier_block iommu_nb;

iommu_nb.notifier_call = vfio_iommu_notifier_cb;
vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);


struct notifier_block group_nb;

group_nb.notifier_call = vfio_group_notifier_cb;
vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);

Thanks,
Kirti

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Alex Williamson]

On Tue, 22 Nov 2016 19:05:16 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 11/22/2016 11:39 AM, Jike Song wrote:
> > Beyond vfio_iommu events, users might also be interested in
> > vfio_group events. For example, if a vfio_group is used along
> > with Qemu/KVM, whenever kvm pointer is set to/cleared from the
> > vfio_group, users could be notified.
> > 
> > Currently only VFIO_GROUP_NOTIFY_SET_KVM supported.
> > 
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > Signed-off-by: Jike Song <jike.song@intel.com>
> > ---
> >  drivers/vfio/vfio.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/vfio.h |  6 ++++
> >  2 files changed, 88 insertions(+)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 493bbad..3ab5edb 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -86,6 +86,8 @@ struct vfio_group {
> >  	struct mutex			unbound_lock;
> >  	atomic_t			opened;
> >  	bool				noiommu;
> > +	struct kvm			*kvm;
> > +	struct blocking_notifier_head	notifier;
> >  };
> >  
> >  struct vfio_device {
> > @@ -339,6 +341,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
> >  #ifdef CONFIG_VFIO_NOIOMMU
> >  	group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu);
> >  #endif
> > +	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
> >  
> >  	group->nb.notifier_call = vfio_iommu_group_notifier;
> >  
> > @@ -1581,6 +1584,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
> >  
> >  	filep->private_data = NULL;
> >  
> > +	/* Any user didn't unregister? */
> > +	WARN_ON(group->notifier.head);
> > +
> >  	vfio_group_try_dissolve_container(group);
> >  
> >  	atomic_dec(&group->opened);
> > @@ -2069,6 +2075,76 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
> >  	return ret;
> >  }
> >  
> > +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> > +{
> > +	group->kvm = kvm;
> > +	blocking_notifier_call_chain(&group->notifier,
> > +				VFIO_GROUP_NOTIFY_SET_KVM, kvm);
> > +}
> > +EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
> > +
> > +static int vfio_register_group_notifier(struct vfio_group *group,
> > +					unsigned long *events,
> > +					struct notifier_block *nb)
> > +{
> > +	struct vfio_container *container;
> > +	int ret;
> > +	bool set_kvm = false;
> > +
> > +	if (*events & VFIO_GROUP_NOTIFY_SET_KVM)
> > +		set_kvm = true;
> > +
> > +	/* clear known events */
> > +	*events &= ~VFIO_GROUP_NOTIFY_SET_KVM;
> > +
> > +	/* refuse to continue if still events remaining */
> > +	if (*events)
> > +		return -EINVAL;
> > +
> > +	ret = vfio_group_add_container_user(group);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	container = group->container;
> > +	down_read(&container->group_lock);
> > +
> > +	ret = blocking_notifier_chain_register(&group->notifier, nb);
> > +
> > +	/*
> > +	 * The attaching of kvm and vfio_group might already happen, so
> > +	 * here we replay once upon registration.
> > +	 */
> > +	if (!ret && set_kvm && group->kvm)
> > +		blocking_notifier_call_chain(&group->notifier,
> > +					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
> > +
> > +	up_read(&container->group_lock);
> > +	vfio_group_try_dissolve_container(group);
> > +
> > +	return ret;
> > +}
> > +
> > +static int vfio_unregister_group_notifier(struct vfio_group *group,
> > +					 struct notifier_block *nb)
> > +{
> > +	struct vfio_container *container;
> > +	int ret;
> > +
> > +	ret = vfio_group_add_container_user(group);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	container = group->container;
> > +	down_read(&container->group_lock);
> > +
> > +	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
> > +
> > +	up_read(&container->group_lock);
> > +	vfio_group_try_dissolve_container(group);
> > +
> > +	return ret;
> > +}
> > +
> >  int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
> >  			   unsigned long *events,
> >  			   struct notifier_block *nb)
> > @@ -2087,6 +2163,9 @@ int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
> >  	case VFIO_IOMMU_NOTIFY:
> >  		ret = vfio_register_iommu_notifier(group, events, nb);
> >  		break;
> > +	case VFIO_GROUP_NOTIFY:
> > +		ret = vfio_register_group_notifier(group, events, nb);
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > @@ -2113,6 +2192,9 @@ int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type,
> >  	case VFIO_IOMMU_NOTIFY:
> >  		ret = vfio_unregister_iommu_notifier(group, nb);
> >  		break;
> > +	case VFIO_GROUP_NOTIFY:
> > +		ret = vfio_unregister_group_notifier(group, nb);
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> > index 6f3ff31..5d46e3c 100644
> > --- a/include/linux/vfio.h
> > +++ b/include/linux/vfio.h
> > @@ -119,11 +119,17 @@ extern int vfio_unregister_notifier(struct device *dev,
> >  /* each type has independent events */
> >  enum vfio_notify_type {
> >  	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
> > +	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
> >  };
> >  
> >  /* events for VFIO_IOMMU_NOTIFY */
> >  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
> >  
> > +/* events for VFIO_GROUP_NOTIFY */
> > +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
> > +
> > +struct kvm;
> > +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
> >    
> 
> If I understand correctly, notifier for backend iommu driver and
> notifier for group should be registered with different notifier blocks,
> as per your current implementation:
> 
> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> struct notifier_block iommu_nb;
> 
> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
> 
> 
> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
> struct notifier_block group_nb;
> 
> group_nb.notifier_call = vfio_group_notifier_cb;
> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
> 
> Then what is the use of having 'events' bitmask as input argument?
> 
> Its clear that caller should set:
> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
> 
> Notifier action could be a integer as earlier and indexing of action
> would be different for both notifiers.
> 
> Then caller don't have to sent event bitmask, for caller it would look like:
> 
> struct notifier_block iommu_nb;
> 
> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
> 
> 
> struct notifier_block group_nb;
> 
> group_nb.notifier_call = vfio_group_notifier_cb;
> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);

If we did that then we'd effectively need to add a new notifier any
time we want a new signal because the vendor driver would have no
ability to query the notifications available.  For instance, say we
added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
that and would either refuse to work or would take a backup path if
that notification isn't available.  How could it operate in your
proposal?  By passing a bitmask of events, the vendor driver can
specify the set of required events and see which event signaling is
unavailable.  The purpose of using a bitmask and passing the set of
required bits on registration is to support future expansion.  Thanks,

Alex

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Kirti Wankhede]

On 11/22/2016 7:32 PM, Alex Williamson wrote:
> On Tue, 22 Nov 2016 19:05:16 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 11/22/2016 11:39 AM, Jike Song wrote:

<snip>

>>>  /* events for VFIO_IOMMU_NOTIFY */
>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
>>>  
>>> +/* events for VFIO_GROUP_NOTIFY */
>>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
>>> +
>>> +struct kvm;
>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
>>>    
>>
>> If I understand correctly, notifier for backend iommu driver and
>> notifier for group should be registered with different notifier blocks,
>> as per your current implementation:
>>
>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>> struct notifier_block iommu_nb;
>>
>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
>>
>>
>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
>> struct notifier_block group_nb;
>>
>> group_nb.notifier_call = vfio_group_notifier_cb;
>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
>>
>> Then what is the use of having 'events' bitmask as input argument?
>>
>> Its clear that caller should set:
>> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
>> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
>>
>> Notifier action could be a integer as earlier and indexing of action
>> would be different for both notifiers.
>>
>> Then caller don't have to sent event bitmask, for caller it would look like:
>>
>> struct notifier_block iommu_nb;
>>
>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
>>
>>
>> struct notifier_block group_nb;
>>
>> group_nb.notifier_call = vfio_group_notifier_cb;
>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);
> 
> If we did that then we'd effectively need to add a new notifier any
> time we want a new signal because the vendor driver would have no
> ability to query the notifications available.  For instance, say we
> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
> that and would either refuse to work or would take a backup path if
> that notification isn't available.  How could it operate in your
> proposal?  By passing a bitmask of events, the vendor driver can
> specify the set of required events and see which event signaling is
> unavailable.  The purpose of using a bitmask and passing the set of
> required bits on registration is to support future expansion.  Thanks,
> 

If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
action in include/linux/vfio.h

 #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
+#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)

Vendor driver anyways need to be compiled against the kernel to which
its going to run. So vendor driver could use conditional directive:

#if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
    /* foo signal available*/
#else
    /* foo signal not available*/
#endif

Thanks,
Kirti

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Alex Williamson]

On Tue, 22 Nov 2016 20:09:32 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 11/22/2016 7:32 PM, Alex Williamson wrote:
> > On Tue, 22 Nov 2016 19:05:16 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 11/22/2016 11:39 AM, Jike Song wrote:  
> 
> <snip>
> 
> >>>  /* events for VFIO_IOMMU_NOTIFY */
> >>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
> >>>  
> >>> +/* events for VFIO_GROUP_NOTIFY */
> >>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
> >>> +
> >>> +struct kvm;
> >>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
> >>>      
> >>
> >> If I understand correctly, notifier for backend iommu driver and
> >> notifier for group should be registered with different notifier blocks,
> >> as per your current implementation:
> >>
> >> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> >> struct notifier_block iommu_nb;
> >>
> >> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> >> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
> >>
> >>
> >> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
> >> struct notifier_block group_nb;
> >>
> >> group_nb.notifier_call = vfio_group_notifier_cb;
> >> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
> >>
> >> Then what is the use of having 'events' bitmask as input argument?
> >>
> >> Its clear that caller should set:
> >> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
> >> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
> >>
> >> Notifier action could be a integer as earlier and indexing of action
> >> would be different for both notifiers.
> >>
> >> Then caller don't have to sent event bitmask, for caller it would look like:
> >>
> >> struct notifier_block iommu_nb;
> >>
> >> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> >> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
> >>
> >>
> >> struct notifier_block group_nb;
> >>
> >> group_nb.notifier_call = vfio_group_notifier_cb;
> >> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);  
> > 
> > If we did that then we'd effectively need to add a new notifier any
> > time we want a new signal because the vendor driver would have no
> > ability to query the notifications available.  For instance, say we
> > added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
> > that and would either refuse to work or would take a backup path if
> > that notification isn't available.  How could it operate in your
> > proposal?  By passing a bitmask of events, the vendor driver can
> > specify the set of required events and see which event signaling is
> > unavailable.  The purpose of using a bitmask and passing the set of
> > required bits on registration is to support future expansion.  Thanks,
> >   
> 
> If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
> action in include/linux/vfio.h
> 
>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
> +#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)
> 
> Vendor driver anyways need to be compiled against the kernel to which
> its going to run. So vendor driver could use conditional directive:
> 
> #if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
>     /* foo signal available*/
> #else
>     /* foo signal not available*/
> #endif

No, the vendor driver is not necessarily compiled against the given
kernel.  We can also have different backends in vfio, what if we have
IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
the vendor driver might only support one of these and can't tell at
compile time which is active.  Thanks,

Alex

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Jike Song]

On 11/22/2016 10:50 PM, Alex Williamson wrote:
> On Tue, 22 Nov 2016 20:09:32 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 11/22/2016 7:32 PM, Alex Williamson wrote:
>>> On Tue, 22 Nov 2016 19:05:16 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>   
>>>> On 11/22/2016 11:39 AM, Jike Song wrote:  
>>
>> <snip>
>>
>>>>>  /* events for VFIO_IOMMU_NOTIFY */
>>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
>>>>>  
>>>>> +/* events for VFIO_GROUP_NOTIFY */
>>>>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
>>>>> +
>>>>> +struct kvm;
>>>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
>>>>>      
>>>>
>>>> If I understand correctly, notifier for backend iommu driver and
>>>> notifier for group should be registered with different notifier blocks,
>>>> as per your current implementation:
>>>>
>>>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>>>> struct notifier_block iommu_nb;
>>>>
>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
>>>>
>>>>
>>>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
>>>> struct notifier_block group_nb;
>>>>
>>>> group_nb.notifier_call = vfio_group_notifier_cb;
>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
>>>>
>>>> Then what is the use of having 'events' bitmask as input argument?
>>>>
>>>> Its clear that caller should set:
>>>> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
>>>> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
>>>>
>>>> Notifier action could be a integer as earlier and indexing of action
>>>> would be different for both notifiers.
>>>>
>>>> Then caller don't have to sent event bitmask, for caller it would look like:
>>>>
>>>> struct notifier_block iommu_nb;
>>>>
>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
>>>>
>>>>
>>>> struct notifier_block group_nb;
>>>>
>>>> group_nb.notifier_call = vfio_group_notifier_cb;
>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);  
>>>
>>> If we did that then we'd effectively need to add a new notifier any
>>> time we want a new signal because the vendor driver would have no
>>> ability to query the notifications available.  For instance, say we
>>> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
>>> that and would either refuse to work or would take a backup path if
>>> that notification isn't available.  How could it operate in your
>>> proposal?  By passing a bitmask of events, the vendor driver can
>>> specify the set of required events and see which event signaling is
>>> unavailable.  The purpose of using a bitmask and passing the set of
>>> required bits on registration is to support future expansion.  Thanks,
>>>   
>>
>> If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
>> action in include/linux/vfio.h
>>
>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
>> +#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)
>>
>> Vendor driver anyways need to be compiled against the kernel to which
>> its going to run. So vendor driver could use conditional directive:
>>
>> #if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
>>     /* foo signal available*/
>> #else
>>     /* foo signal not available*/
>> #endif
> 
> No, the vendor driver is not necessarily compiled against the given
> kernel.  We can also have different backends in vfio, what if we have
> IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
> the vendor driver might only support one of these and can't tell at
> compile time which is active.  Thanks,

Yes, if we rely on macros only, driver not built against current kernel
won't work correctly. But have IOMMU backends considered, seems that we
don't differentiating them, having only one VFIO_IOMMU_NOTIFY?

--
Thanks,
Jike

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Kirti Wankhede]

On 11/23/2016 8:50 AM, Jike Song wrote:
> On 11/22/2016 10:50 PM, Alex Williamson wrote:
>> On Tue, 22 Nov 2016 20:09:32 +0530
>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>
>>> On 11/22/2016 7:32 PM, Alex Williamson wrote:
>>>> On Tue, 22 Nov 2016 19:05:16 +0530
>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>   
>>>>> On 11/22/2016 11:39 AM, Jike Song wrote:  
>>>
>>> <snip>
>>>
>>>>>>  /* events for VFIO_IOMMU_NOTIFY */
>>>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
>>>>>>  
>>>>>> +/* events for VFIO_GROUP_NOTIFY */
>>>>>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
>>>>>> +
>>>>>> +struct kvm;
>>>>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
>>>>>>      
>>>>>
>>>>> If I understand correctly, notifier for backend iommu driver and
>>>>> notifier for group should be registered with different notifier blocks,
>>>>> as per your current implementation:
>>>>>
>>>>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>>>>> struct notifier_block iommu_nb;
>>>>>
>>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
>>>>>
>>>>>
>>>>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
>>>>> struct notifier_block group_nb;
>>>>>
>>>>> group_nb.notifier_call = vfio_group_notifier_cb;
>>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
>>>>>
>>>>> Then what is the use of having 'events' bitmask as input argument?
>>>>>
>>>>> Its clear that caller should set:
>>>>> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
>>>>> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
>>>>>
>>>>> Notifier action could be a integer as earlier and indexing of action
>>>>> would be different for both notifiers.
>>>>>
>>>>> Then caller don't have to sent event bitmask, for caller it would look like:
>>>>>
>>>>> struct notifier_block iommu_nb;
>>>>>
>>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
>>>>>
>>>>>
>>>>> struct notifier_block group_nb;
>>>>>
>>>>> group_nb.notifier_call = vfio_group_notifier_cb;
>>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);  
>>>>
>>>> If we did that then we'd effectively need to add a new notifier any
>>>> time we want a new signal because the vendor driver would have no
>>>> ability to query the notifications available.  For instance, say we
>>>> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
>>>> that and would either refuse to work or would take a backup path if
>>>> that notification isn't available.  How could it operate in your
>>>> proposal?  By passing a bitmask of events, the vendor driver can
>>>> specify the set of required events and see which event signaling is
>>>> unavailable.  The purpose of using a bitmask and passing the set of
>>>> required bits on registration is to support future expansion.  Thanks,
>>>>   
>>>
>>> If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
>>> action in include/linux/vfio.h
>>>
>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
>>> +#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)
>>>
>>> Vendor driver anyways need to be compiled against the kernel to which
>>> its going to run. So vendor driver could use conditional directive:
>>>
>>> #if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
>>>     /* foo signal available*/
>>> #else
>>>     /* foo signal not available*/
>>> #endif
>>
>> No, the vendor driver is not necessarily compiled against the given
>> kernel.  We can also have different backends in vfio, what if we have
>> IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
>> the vendor driver might only support one of these and can't tell at
>> compile time which is active.  Thanks,
> 
> Yes, if we rely on macros only, driver not built against current kernel
> won't work correctly. But have IOMMU backends considered, seems that we
> don't differentiating them, having only one VFIO_IOMMU_NOTIFY?
> 

Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its
mostly safe to build driver against the kernel to which its going to load.
For backend IOMMU module, we have same callback functions or
VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules
might have different events/actions. Then the event check shouldn't be
in vfio module, it should be in backend module.

Thanks,
Kirti

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Alex Williamson]

On Wed, 23 Nov 2016 10:22:37 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 11/23/2016 8:50 AM, Jike Song wrote:
> > On 11/22/2016 10:50 PM, Alex Williamson wrote:  
> >> On Tue, 22 Nov 2016 20:09:32 +0530
> >> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>  
> >>> On 11/22/2016 7:32 PM, Alex Williamson wrote:  
> >>>> On Tue, 22 Nov 2016 19:05:16 +0530
> >>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>     
> >>>>> On 11/22/2016 11:39 AM, Jike Song wrote:    
> >>>
> >>> <snip>
> >>>  
> >>>>>>  /* events for VFIO_IOMMU_NOTIFY */
> >>>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
> >>>>>>  
> >>>>>> +/* events for VFIO_GROUP_NOTIFY */
> >>>>>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
> >>>>>> +
> >>>>>> +struct kvm;
> >>>>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
> >>>>>>        
> >>>>>
> >>>>> If I understand correctly, notifier for backend iommu driver and
> >>>>> notifier for group should be registered with different notifier blocks,
> >>>>> as per your current implementation:
> >>>>>
> >>>>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> >>>>> struct notifier_block iommu_nb;
> >>>>>
> >>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> >>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
> >>>>>
> >>>>>
> >>>>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
> >>>>> struct notifier_block group_nb;
> >>>>>
> >>>>> group_nb.notifier_call = vfio_group_notifier_cb;
> >>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
> >>>>>
> >>>>> Then what is the use of having 'events' bitmask as input argument?
> >>>>>
> >>>>> Its clear that caller should set:
> >>>>> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
> >>>>> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
> >>>>>
> >>>>> Notifier action could be a integer as earlier and indexing of action
> >>>>> would be different for both notifiers.
> >>>>>
> >>>>> Then caller don't have to sent event bitmask, for caller it would look like:
> >>>>>
> >>>>> struct notifier_block iommu_nb;
> >>>>>
> >>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> >>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
> >>>>>
> >>>>>
> >>>>> struct notifier_block group_nb;
> >>>>>
> >>>>> group_nb.notifier_call = vfio_group_notifier_cb;
> >>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);    
> >>>>
> >>>> If we did that then we'd effectively need to add a new notifier any
> >>>> time we want a new signal because the vendor driver would have no
> >>>> ability to query the notifications available.  For instance, say we
> >>>> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
> >>>> that and would either refuse to work or would take a backup path if
> >>>> that notification isn't available.  How could it operate in your
> >>>> proposal?  By passing a bitmask of events, the vendor driver can
> >>>> specify the set of required events and see which event signaling is
> >>>> unavailable.  The purpose of using a bitmask and passing the set of
> >>>> required bits on registration is to support future expansion.  Thanks,
> >>>>     
> >>>
> >>> If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
> >>> action in include/linux/vfio.h
> >>>
> >>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
> >>> +#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)
> >>>
> >>> Vendor driver anyways need to be compiled against the kernel to which
> >>> its going to run. So vendor driver could use conditional directive:
> >>>
> >>> #if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
> >>>     /* foo signal available*/
> >>> #else
> >>>     /* foo signal not available*/
> >>> #endif  
> >>
> >> No, the vendor driver is not necessarily compiled against the given
> >> kernel.  We can also have different backends in vfio, what if we have
> >> IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
> >> the vendor driver might only support one of these and can't tell at
> >> compile time which is active.  Thanks,  
> > 
> > Yes, if we rely on macros only, driver not built against current kernel
> > won't work correctly. But have IOMMU backends considered, seems that we
> > don't differentiating them, having only one VFIO_IOMMU_NOTIFY?
> >   
> 
> Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its
> mostly safe to build driver against the kernel to which its going to load.
> For backend IOMMU module, we have same callback functions or
> VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules
> might have different events/actions. Then the event check shouldn't be
> in vfio module, it should be in backend module.

Yes, patch 1/3 is wrong, the required events mask should be passed to
the iommu backend and handled there, not in vfio.c.  Thanks,

Alex

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Jike Song]

On 11/23/2016 01:56 PM, Alex Williamson wrote:
> On Wed, 23 Nov 2016 10:22:37 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 11/23/2016 8:50 AM, Jike Song wrote:
>>> On 11/22/2016 10:50 PM, Alex Williamson wrote:  
>>>> On Tue, 22 Nov 2016 20:09:32 +0530
>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>  
>>>>> On 11/22/2016 7:32 PM, Alex Williamson wrote:  
>>>>>> On Tue, 22 Nov 2016 19:05:16 +0530
>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>     
>>>>>>> On 11/22/2016 11:39 AM, Jike Song wrote:    
>>>>>
>>>>> <snip>
>>>>>  
>>>>>>>>  /* events for VFIO_IOMMU_NOTIFY */
>>>>>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
>>>>>>>>  
>>>>>>>> +/* events for VFIO_GROUP_NOTIFY */
>>>>>>>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
>>>>>>>> +
>>>>>>>> +struct kvm;
>>>>>>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
>>>>>>>>        
>>>>>>>
>>>>>>> If I understand correctly, notifier for backend iommu driver and
>>>>>>> notifier for group should be registered with different notifier blocks,
>>>>>>> as per your current implementation:
>>>>>>>
>>>>>>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>>>>>>> struct notifier_block iommu_nb;
>>>>>>>
>>>>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>>>>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
>>>>>>>
>>>>>>>
>>>>>>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
>>>>>>> struct notifier_block group_nb;
>>>>>>>
>>>>>>> group_nb.notifier_call = vfio_group_notifier_cb;
>>>>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
>>>>>>>
>>>>>>> Then what is the use of having 'events' bitmask as input argument?
>>>>>>>
>>>>>>> Its clear that caller should set:
>>>>>>> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
>>>>>>> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
>>>>>>>
>>>>>>> Notifier action could be a integer as earlier and indexing of action
>>>>>>> would be different for both notifiers.
>>>>>>>
>>>>>>> Then caller don't have to sent event bitmask, for caller it would look like:
>>>>>>>
>>>>>>> struct notifier_block iommu_nb;
>>>>>>>
>>>>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>>>>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
>>>>>>>
>>>>>>>
>>>>>>> struct notifier_block group_nb;
>>>>>>>
>>>>>>> group_nb.notifier_call = vfio_group_notifier_cb;
>>>>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);    
>>>>>>
>>>>>> If we did that then we'd effectively need to add a new notifier any
>>>>>> time we want a new signal because the vendor driver would have no
>>>>>> ability to query the notifications available.  For instance, say we
>>>>>> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
>>>>>> that and would either refuse to work or would take a backup path if
>>>>>> that notification isn't available.  How could it operate in your
>>>>>> proposal?  By passing a bitmask of events, the vendor driver can
>>>>>> specify the set of required events and see which event signaling is
>>>>>> unavailable.  The purpose of using a bitmask and passing the set of
>>>>>> required bits on registration is to support future expansion.  Thanks,
>>>>>>     
>>>>>
>>>>> If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
>>>>> action in include/linux/vfio.h
>>>>>
>>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
>>>>> +#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)
>>>>>
>>>>> Vendor driver anyways need to be compiled against the kernel to which
>>>>> its going to run. So vendor driver could use conditional directive:
>>>>>
>>>>> #if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
>>>>>     /* foo signal available*/
>>>>> #else
>>>>>     /* foo signal not available*/
>>>>> #endif  
>>>>
>>>> No, the vendor driver is not necessarily compiled against the given
>>>> kernel.  We can also have different backends in vfio, what if we have
>>>> IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
>>>> the vendor driver might only support one of these and can't tell at
>>>> compile time which is active.  Thanks,  
>>>
>>> Yes, if we rely on macros only, driver not built against current kernel
>>> won't work correctly. But have IOMMU backends considered, seems that we
>>> don't differentiating them, having only one VFIO_IOMMU_NOTIFY?
>>>   
>>
>> Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its
>> mostly safe to build driver against the kernel to which its going to load.
>> For backend IOMMU module, we have same callback functions or
>> VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules
>> might have different events/actions. Then the event check shouldn't be
>> in vfio module, it should be in backend module.
> 
> Yes, patch 1/3 is wrong, the required events mask should be passed to
> the iommu backend and handled there, not in vfio.c.  Thanks,

Will change that. BTW, do you think the iommu types should be different?
that is to say, having VFIO_IOMMU_NOTIFY_TYPE1 instead of VFIO_IOMMU_NOTIFY?

--
Thanks,
Jike

RE: [v7 2/3] vfio: support notifier chain in vfio_group

[Tian, Kevin]

> From: Song, Jike
> Sent: Wednesday, November 23, 2016 2:30 PM
> 
> On 11/23/2016 01:56 PM, Alex Williamson wrote:
> > On Wed, 23 Nov 2016 10:22:37 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> On 11/23/2016 8:50 AM, Jike Song wrote:
> >>> On 11/22/2016 10:50 PM, Alex Williamson wrote:
> >>>> On Tue, 22 Nov 2016 20:09:32 +0530
> >>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>
> >>>>> On 11/22/2016 7:32 PM, Alex Williamson wrote:
> >>>>>> On Tue, 22 Nov 2016 19:05:16 +0530
> >>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>
> >>>>>>> On 11/22/2016 11:39 AM, Jike Song wrote:
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>>>>  /* events for VFIO_IOMMU_NOTIFY */
> >>>>>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
> >>>>>>>>
> >>>>>>>> +/* events for VFIO_GROUP_NOTIFY */
> >>>>>>>> +#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
> >>>>>>>> +
> >>>>>>>> +struct kvm;
> >>>>>>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm
> *kvm);
> >>>>>>>>
> >>>>>>>
> >>>>>>> If I understand correctly, notifier for backend iommu driver and
> >>>>>>> notifier for group should be registered with different notifier blocks,
> >>>>>>> as per your current implementation:
> >>>>>>>
> >>>>>>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> >>>>>>> struct notifier_block iommu_nb;
> >>>>>>>
> >>>>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> >>>>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event,
> &iommu_nb);
> >>>>>>>
> >>>>>>>
> >>>>>>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
> >>>>>>> struct notifier_block group_nb;
> >>>>>>>
> >>>>>>> group_nb.notifier_call = vfio_group_notifier_cb;
> >>>>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event,
> &group_nb);
> >>>>>>>
> >>>>>>> Then what is the use of having 'events' bitmask as input argument?
> >>>>>>>
> >>>>>>> Its clear that caller should set:
> >>>>>>> -  VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is
> VFIO_IOMMU_NOTIFY
> >>>>>>> -  VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is
> VFIO_GROUP_NOTIFY
> >>>>>>>
> >>>>>>> Notifier action could be a integer as earlier and indexing of action
> >>>>>>> would be different for both notifiers.
> >>>>>>>
> >>>>>>> Then caller don't have to sent event bitmask, for caller it would look like:
> >>>>>>>
> >>>>>>> struct notifier_block iommu_nb;
> >>>>>>>
> >>>>>>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
> >>>>>>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
> >>>>>>>
> >>>>>>>
> >>>>>>> struct notifier_block group_nb;
> >>>>>>>
> >>>>>>> group_nb.notifier_call = vfio_group_notifier_cb;
> >>>>>>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);
> >>>>>>
> >>>>>> If we did that then we'd effectively need to add a new notifier any
> >>>>>> time we want a new signal because the vendor driver would have no
> >>>>>> ability to query the notifications available.  For instance, say we
> >>>>>> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
> >>>>>> that and would either refuse to work or would take a backup path if
> >>>>>> that notification isn't available.  How could it operate in your
> >>>>>> proposal?  By passing a bitmask of events, the vendor driver can
> >>>>>> specify the set of required events and see which event signaling is
> >>>>>> unavailable.  The purpose of using a bitmask and passing the set of
> >>>>>> required bits on registration is to support future expansion.  Thanks,
> >>>>>>
> >>>>>
> >>>>> If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
> >>>>> action in include/linux/vfio.h
> >>>>>
> >>>>>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP    (1)
> >>>>> +#define VFIO_IOMMU_NOTIFY_SET_FOO      (2)
> >>>>>
> >>>>> Vendor driver anyways need to be compiled against the kernel to which
> >>>>> its going to run. So vendor driver could use conditional directive:
> >>>>>
> >>>>> #if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
> >>>>>     /* foo signal available*/
> >>>>> #else
> >>>>>     /* foo signal not available*/
> >>>>> #endif
> >>>>
> >>>> No, the vendor driver is not necessarily compiled against the given
> >>>> kernel.  We can also have different backends in vfio, what if we have
> >>>> IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
> >>>> the vendor driver might only support one of these and can't tell at
> >>>> compile time which is active.  Thanks,
> >>>
> >>> Yes, if we rely on macros only, driver not built against current kernel
> >>> won't work correctly. But have IOMMU backends considered, seems that we
> >>> don't differentiating them, having only one VFIO_IOMMU_NOTIFY?
> >>>
> >>
> >> Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its
> >> mostly safe to build driver against the kernel to which its going to load.
> >> For backend IOMMU module, we have same callback functions or
> >> VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules
> >> might have different events/actions. Then the event check shouldn't be
> >> in vfio module, it should be in backend module.
> >
> > Yes, patch 1/3 is wrong, the required events mask should be passed to
> > the iommu backend and handled there, not in vfio.c.  Thanks,
> 
> Will change that. BTW, do you think the iommu types should be different?
> that is to say, having VFIO_IOMMU_NOTIFY_TYPE1 instead of VFIO_IOMMU_NOTIFY?
> 

Should vendor driver care about underlying IOMMU difference? Assume
those notification events should be IOMMU vendor agnostic... 

Thanks
Kevin

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Jike Song]

On 11/23/2016 02:33 PM, Tian, Kevin wrote:
>> From: Song, Jike
>> Sent: Wednesday, November 23, 2016 2:30 PM
>> On 11/23/2016 01:56 PM, Alex Williamson wrote:
>>> On Wed, 23 Nov 2016 10:22:37 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>
>>>> Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its
>>>> mostly safe to build driver against the kernel to which its going to load.
>>>> For backend IOMMU module, we have same callback functions or
>>>> VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules
>>>> might have different events/actions. Then the event check shouldn't be
>>>> in vfio module, it should be in backend module.
>>>
>>> Yes, patch 1/3 is wrong, the required events mask should be passed to
>>> the iommu backend and handled there, not in vfio.c.  Thanks,
>>
>> Will change that. BTW, do you think the iommu types should be different?
>> that is to say, having VFIO_IOMMU_NOTIFY_TYPE1 instead of VFIO_IOMMU_NOTIFY?
>>
> 
> Should vendor driver care about underlying IOMMU difference? Assume
> those notification events should be IOMMU vendor agnostic... 

I also lean towards keeping VFIO_IOMMU_NOTIFY, will update [1/3] without
changing that part.

--
Thanks,
Jike

[UPDATE v7 1/3] vfio: vfio_register_notifier: classify iommu notifier

[Jike Song]

vfio_register_notifier assumes that there is only one notifier
chain, which is in vfio_iommu. However, the user might also be
interested in events other than vfio_iommu, for example,
vfio_group. Refactor vfio_{un}register_notifier implementation
to make it feasible.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/vfio/vfio.c             | 85 +++++++++++++++++++++++++++++------------
 drivers/vfio/vfio_iommu_type1.c |  8 ++++
 include/linux/vfio.h            | 17 +++++++--
 3 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 0aac3ca..cb2ce2d 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2008,59 +2008,44 @@ int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage)
 }
 EXPORT_SYMBOL(vfio_unpin_pages);
 
-int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
+static int vfio_register_iommu_notifier(struct vfio_group *group,
+					unsigned long *events,
+					struct notifier_block *nb)
 {
 	struct vfio_container *container;
-	struct vfio_group *group;
 	struct vfio_iommu_driver *driver;
 	int ret;
 
-	if (!dev || !nb)
-		return -EINVAL;
-
-	group = vfio_group_get_from_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
 	ret = vfio_group_add_container_user(group);
 	if (ret)
-		goto err_register_nb;
+		return -EINVAL;
 
 	container = group->container;
 	down_read(&container->group_lock);
 
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->register_notifier))
-		ret = driver->ops->register_notifier(container->iommu_data, nb);
+		ret = driver->ops->register_notifier(container->iommu_data,
+						     events, nb);
 	else
 		ret = -ENOTTY;
 
 	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
-err_register_nb:
-	vfio_group_put(group);
 	return ret;
 }
-EXPORT_SYMBOL(vfio_register_notifier);
 
-int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
+static int vfio_unregister_iommu_notifier(struct vfio_group *group,
+					  struct notifier_block *nb)
 {
 	struct vfio_container *container;
-	struct vfio_group *group;
 	struct vfio_iommu_driver *driver;
 	int ret;
 
-	if (!dev || !nb)
-		return -EINVAL;
-
-	group = vfio_group_get_from_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
 	ret = vfio_group_add_container_user(group);
 	if (ret)
-		goto err_unregister_nb;
+		return -EINVAL;
 
 	container = group->container;
 	down_read(&container->group_lock);
@@ -2075,7 +2060,57 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
 	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
-err_unregister_nb:
+	return ret;
+}
+
+int vfio_register_notifier(struct device *dev, vfio_notify_type_t type,
+			   unsigned long *events,
+			   struct notifier_block *nb)
+{
+	struct vfio_group *group;
+	int ret;
+
+	if (!dev || !nb || !events || (*events == 0))
+		return -EINVAL;
+
+	group = vfio_group_get_from_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	switch (type) {
+	case VFIO_IOMMU_NOTIFY:
+		ret = vfio_register_iommu_notifier(group, events, nb);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	vfio_group_put(group);
+	return ret;
+}
+EXPORT_SYMBOL(vfio_register_notifier);
+
+int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type,
+			     struct notifier_block *nb)
+{
+	struct vfio_group *group;
+	int ret;
+
+	if (!dev || !nb)
+		return -EINVAL;
+
+	group = vfio_group_get_from_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	switch (type) {
+	case VFIO_IOMMU_NOTIFY:
+		ret = vfio_unregister_iommu_notifier(group, nb);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
 	vfio_group_put(group);
 	return ret;
 }
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 51810a9..b88ad1e 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1584,10 +1584,18 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 }
 
 static int vfio_iommu_type1_register_notifier(void *iommu_data,
+					      unsigned long *events,
 					      struct notifier_block *nb)
 {
 	struct vfio_iommu *iommu = iommu_data;
 
+	/* clear known events */
+	*events &= ~VFIO_IOMMU_NOTIFY_DMA_UNMAP;
+
+	/* refuse to register if still events remaining */
+	if (*events)
+		return -EINVAL;
+
 	return blocking_notifier_chain_register(&iommu->notifier, nb);
 }
 
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 15ff042..fdb8460 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -81,6 +81,7 @@ struct vfio_iommu_driver_ops {
 	int		(*unpin_pages)(void *iommu_data,
 				       unsigned long *user_pfn, int npage);
 	int		(*register_notifier)(void *iommu_data,
+					     unsigned long *events,
 					     struct notifier_block *nb);
 	int		(*unregister_notifier)(void *iommu_data,
 					       struct notifier_block *nb);
@@ -107,14 +108,24 @@ extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
 extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
 			    int npage);
 
-#define VFIO_IOMMU_NOTIFY_DMA_UNMAP	(1)
-
+typedef unsigned short vfio_notify_type_t;
 extern int vfio_register_notifier(struct device *dev,
+				  vfio_notify_type_t type,
+				  unsigned long *required_events,
 				  struct notifier_block *nb);
-
 extern int vfio_unregister_notifier(struct device *dev,
+				    vfio_notify_type_t type,
 				    struct notifier_block *nb);
 
+/* each type has independent events */
+enum vfio_notify_type {
+	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
+};
+
+/* events for VFIO_IOMMU_NOTIFY */
+#define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
+
+
 /*
  * Sub-module helpers
  */
-- 
1.9.1

Re: [v7 2/3] vfio: support notifier chain in vfio_group

[Alex Williamson]

On Wed, 23 Nov 2016 15:53:23 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/23/2016 02:33 PM, Tian, Kevin wrote:
> >> From: Song, Jike
> >> Sent: Wednesday, November 23, 2016 2:30 PM
> >> On 11/23/2016 01:56 PM, Alex Williamson wrote:  
> >>> On Wed, 23 Nov 2016 10:22:37 +0530
> >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:  
> >>>>
> >>>> Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its
> >>>> mostly safe to build driver against the kernel to which its going to load.
> >>>> For backend IOMMU module, we have same callback functions or
> >>>> VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules
> >>>> might have different events/actions. Then the event check shouldn't be
> >>>> in vfio module, it should be in backend module.  
> >>>
> >>> Yes, patch 1/3 is wrong, the required events mask should be passed to
> >>> the iommu backend and handled there, not in vfio.c.  Thanks,  
> >>
> >> Will change that. BTW, do you think the iommu types should be different?
> >> that is to say, having VFIO_IOMMU_NOTIFY_TYPE1 instead of VFIO_IOMMU_NOTIFY?
> >>  
> > 
> > Should vendor driver care about underlying IOMMU difference? Assume
> > those notification events should be IOMMU vendor agnostic...   
> 
> I also lean towards keeping VFIO_IOMMU_NOTIFY, will update [1/3] without
> changing that part.

Agreed, the vendor driver has no visibility to the iommu model selected
by the user.  The type identifies where the notifier is attached.  The
event mask allows the vendor driver to set required notifications.  If
an iommu backend does not support a compatible notification, it doesn't
matter whether it's type1 vs spapr vs any future iommu backend type.
Thanks,

Alex

Re: [v7 0/3] plumb kvm/vfio to notify kvm:group attach/detach

[Jike Song]

On 11/22/2016 02:09 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, when it will be detached.
> 
> There is already a kvm_vfio device serving for similar
> purpose, this series extends it to allow users register
> notifiers on a vfio_group, and notify kvm:group attaching
> and detaching events to that.
> 
> 
> Changes v7:
> 	- replace vfio->opened with container user like iommu notifier registration
> 	- fix a typo
> Changes v6:
> 	- only allow registration on already opened vfio_group 
> 	- moved WARN_ON from vfio_group_release to vfio_group_fops_release
> 	- remove unnecessary check
> Changes v5:
> 	- separate vfio_group and vfio_iommu events from each other
> 	- force user to specify which type it is interested in, and which events.
> 	  Register it IFF all required events are supported
> Changes v4:
> 	- use a notifier-based implementation in vfio_group
> 	- combine the notifier along with iommu notifier by Kirti
> 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 (3):
>   vfio: vfio_register_notifier: classify iommu notifier
>   vfio: support notifier chain in vfio_group
>   kvm: set/clear kvm to/from vfio_group when group add/delete
> 
>  drivers/vfio/vfio.c  | 167 ++++++++++++++++++++++++++++++++++++++++++++-------
>  include/linux/vfio.h |  22 ++++++-
>  virt/kvm/vfio.c      |  31 ++++++++++
>  3 files changed, 195 insertions(+), 25 deletions(-)

Hi Alex,

Since Paolo has ACKed a previous version of PATCH [3/3] before, if you have
no more comments on this series, could you pick them up? Please be aware that
1/3 was updated, or I can send a v8 if you prefer :)

--
Thanks,
Jike

Re: [v7 3/3] kvm: set/clear kvm to/from vfio_group when group add/delete

[Alex Williamson]

On Tue, 22 Nov 2016 14:09:33 +0800
Jike Song <jike.song@intel.com> wrote:

> Sometimes users need to be aware when a vfio_group attaches to a
> KVM or detaches from it. KVM already calls get/put method from vfio to
> manipulate the vfio_group reference, it can notify vfio_group in
> a similar way.
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  virt/kvm/vfio.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 1dd087d..ea5b4bb 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_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);
> +
> +	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);
> +
> +	symbol_put(vfio_group_set_kvm);
> +}


Why do we need this function?  Can't we simply call
kvm_vfio_group_set_kvm(vfio_group, NULL)?  Not to mention that the
struct kvm arg is unused here otherwise.

> +
>  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_set_kvm(vfio_group, dev->kvm);
> +
>  		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_clear_kvm(vfio_group, dev->kvm);
> +


Why did we set_kvm within kv->lock, but clear it outside of kv->lock?
I'm not sure kv->lock is particularly relevant to us but we might as
well be consistent.

>  		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_clear_kvm(kvg->vfio_group, dev->kvm);
>  		kvm_vfio_group_put_external_user(kvg->vfio_group);
>  		list_del(&kvg->node);
>  		kfree(kvg);

Re: [v7 0/3] plumb kvm/vfio to notify kvm:group attach/detach

[Alex Williamson]

On Tue, 29 Nov 2016 11:02:02 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/22/2016 02:09 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, when it will be detached.
> > 
> > There is already a kvm_vfio device serving for similar
> > purpose, this series extends it to allow users register
> > notifiers on a vfio_group, and notify kvm:group attaching
> > and detaching events to that.
> > 
> > 
> > Changes v7:
> > 	- replace vfio->opened with container user like iommu notifier registration
> > 	- fix a typo
> > Changes v6:
> > 	- only allow registration on already opened vfio_group 
> > 	- moved WARN_ON from vfio_group_release to vfio_group_fops_release
> > 	- remove unnecessary check
> > Changes v5:
> > 	- separate vfio_group and vfio_iommu events from each other
> > 	- force user to specify which type it is interested in, and which events.
> > 	  Register it IFF all required events are supported
> > Changes v4:
> > 	- use a notifier-based implementation in vfio_group
> > 	- combine the notifier along with iommu notifier by Kirti
> > 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 (3):
> >   vfio: vfio_register_notifier: classify iommu notifier
> >   vfio: support notifier chain in vfio_group
> >   kvm: set/clear kvm to/from vfio_group when group add/delete
> > 
> >  drivers/vfio/vfio.c  | 167 ++++++++++++++++++++++++++++++++++++++++++++-------
> >  include/linux/vfio.h |  22 ++++++-
> >  virt/kvm/vfio.c      |  31 ++++++++++
> >  3 files changed, 195 insertions(+), 25 deletions(-)  
> 
> Hi Alex,
> 
> Since Paolo has ACKed a previous version of PATCH [3/3] before, if you have
> no more comments on this series, could you pick them up? Please be aware that
> 1/3 was updated, or I can send a v8 if you prefer :)

Sorry for the delay, I was on holiday.  Please send a v8, particularly
since I noted updates in patch 3/3 as well.  I would prefer to commit
this with a R-b from Kirti since the NVIDIA driver is also affected by
this, but I believe this is the right approach for us long term.  I'm
also having trouble finding an ack from Paolo that's particularly
relevant for the kvm-vfio portion, but I don't think this is a
controversial issue, I feel comfortable that he's onboard with the
idea.  Thanks,

Alex

Re: [v7 0/3] plumb kvm/vfio to notify kvm:group attach/detach

[Jike Song]

On 12/01/2016 01:06 AM, Alex Williamson wrote:
> On Tue, 29 Nov 2016 11:02:02 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 11/22/2016 02:09 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, when it will be detached.
>>>
>>> There is already a kvm_vfio device serving for similar
>>> purpose, this series extends it to allow users register
>>> notifiers on a vfio_group, and notify kvm:group attaching
>>> and detaching events to that.
>>>
>>>
>>> Changes v7:
>>> 	- replace vfio->opened with container user like iommu notifier registration
>>> 	- fix a typo
>>> Changes v6:
>>> 	- only allow registration on already opened vfio_group 
>>> 	- moved WARN_ON from vfio_group_release to vfio_group_fops_release
>>> 	- remove unnecessary check
>>> Changes v5:
>>> 	- separate vfio_group and vfio_iommu events from each other
>>> 	- force user to specify which type it is interested in, and which events.
>>> 	  Register it IFF all required events are supported
>>> Changes v4:
>>> 	- use a notifier-based implementation in vfio_group
>>> 	- combine the notifier along with iommu notifier by Kirti
>>> 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 (3):
>>>   vfio: vfio_register_notifier: classify iommu notifier
>>>   vfio: support notifier chain in vfio_group
>>>   kvm: set/clear kvm to/from vfio_group when group add/delete
>>>
>>>  drivers/vfio/vfio.c  | 167 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>  include/linux/vfio.h |  22 ++++++-
>>>  virt/kvm/vfio.c      |  31 ++++++++++
>>>  3 files changed, 195 insertions(+), 25 deletions(-)  
>>
>> Hi Alex,
>>
>> Since Paolo has ACKed a previous version of PATCH [3/3] before, if you have
>> no more comments on this series, could you pick them up? Please be aware that
>> 1/3 was updated, or I can send a v8 if you prefer :)
> 
> Sorry for the delay, I was on holiday.  Please send a v8, particularly
> since I noted updates in patch 3/3 as well.  I would prefer to commit
> this with a R-b from Kirti since the NVIDIA driver is also affected by
> this, but I believe this is the right approach for us long term.  I'm
> also having trouble finding an ack from Paolo that's particularly
> relevant for the kvm-vfio portion, but I don't think this is a
> controversial issue, I feel comfortable that he's onboard with the
> idea.  Thanks,

Hi Alex,

Yes thanks, I will push the v8 asap.


Hi Kirti,

I will commit 1/3 with your R-b in next version, hope you are okay
with that :-)


Hi Paolo,

I got you ACK on one of v3:

	http://www.spinics.net/lists/kvm/msg139498.html

Now 3/3 of this version is a little different:

	kvm_get_kvm is removed from kvm_vfio_group_set_kvm, and
	kvm_put_kvm is removed from kvm_vfio_group_put_kvm,
since:
	the former is already in kvm_ioctl_create_device, and
	the latter is already in kvm_device_release.

Are you okay with that? I'll still put your ACK in 3/3, hope
you are okay with that :-)

--
Thanks,
Jike

Re: [v7 3/3] kvm: set/clear kvm to/from vfio_group when group add/delete

[Jike Song]

On 12/01/2016 01:02 AM, Alex Williamson wrote:
> On Tue, 22 Nov 2016 14:09:33 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> Sometimes users need to be aware when a vfio_group attaches to a
>> KVM or detaches from it. KVM already calls get/put method from vfio to
>> manipulate the vfio_group reference, it can notify vfio_group in
>> a similar way.
>>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Kirti Wankhede <kwankhede@nvidia.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> ---
>>  virt/kvm/vfio.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
>> index 1dd087d..ea5b4bb 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_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);
>> +
>> +	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);
>> +
>> +	symbol_put(vfio_group_set_kvm);
>> +}
> 
> Why do we need this function?  Can't we simply call
> kvm_vfio_group_set_kvm(vfio_group, NULL)?  Not to mention that the
> struct kvm arg is unused here otherwise.

This was the place to call kvm_put_kvm in previous series, but yes,
it's not necessary in current series, will drop it.

>> +
>>  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_set_kvm(vfio_group, dev->kvm);
>> +
>>  		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_clear_kvm(vfio_group, dev->kvm);
>> +
> 
> 
> Why did we set_kvm within kv->lock, but clear it outside of kv->lock?
> I'm not sure kv->lock is particularly relevant to us but we might as
> well be consistent.

kv->lock is not to protect the vfio_group anyway, so being out of the lock
should be safe. But placing set_kvm before acquiring kv->lock will generate
more code in error path, so I guess the better way is to place it after
unlocking. Will reflect that in next version.

>>  		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_clear_kvm(kvg->vfio_group, dev->kvm);
>>  		kvm_vfio_group_put_external_user(kvg->vfio_group);
>>  		list_del(&kvg->node);
>>  		kfree(kvg);
> 

--
Thanks,
Jike

Re: [v7 0/3] plumb kvm/vfio to notify kvm:group attach/detach

[Kirti Wankhede]

On 12/1/2016 7:57 AM, Jike Song wrote:
> On 12/01/2016 01:06 AM, Alex Williamson wrote:
>> On Tue, 29 Nov 2016 11:02:02 +0800
>> Jike Song <jike.song@intel.com> wrote:
>>
>>> On 11/22/2016 02:09 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, when it will be detached.
>>>>
>>>> There is already a kvm_vfio device serving for similar
>>>> purpose, this series extends it to allow users register
>>>> notifiers on a vfio_group, and notify kvm:group attaching
>>>> and detaching events to that.
>>>>
>>>>
>>>> Changes v7:
>>>> 	- replace vfio->opened with container user like iommu notifier registration
>>>> 	- fix a typo
>>>> Changes v6:
>>>> 	- only allow registration on already opened vfio_group 
>>>> 	- moved WARN_ON from vfio_group_release to vfio_group_fops_release
>>>> 	- remove unnecessary check
>>>> Changes v5:
>>>> 	- separate vfio_group and vfio_iommu events from each other
>>>> 	- force user to specify which type it is interested in, and which events.
>>>> 	  Register it IFF all required events are supported
>>>> Changes v4:
>>>> 	- use a notifier-based implementation in vfio_group
>>>> 	- combine the notifier along with iommu notifier by Kirti
>>>> 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 (3):
>>>>   vfio: vfio_register_notifier: classify iommu notifier
>>>>   vfio: support notifier chain in vfio_group
>>>>   kvm: set/clear kvm to/from vfio_group when group add/delete
>>>>
>>>>  drivers/vfio/vfio.c  | 167 ++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  include/linux/vfio.h |  22 ++++++-
>>>>  virt/kvm/vfio.c      |  31 ++++++++++
>>>>  3 files changed, 195 insertions(+), 25 deletions(-)  
>>>
>>> Hi Alex,
>>>
>>> Since Paolo has ACKed a previous version of PATCH [3/3] before, if you have
>>> no more comments on this series, could you pick them up? Please be aware that
>>> 1/3 was updated, or I can send a v8 if you prefer :)
>>
>> Sorry for the delay, I was on holiday.  Please send a v8, particularly
>> since I noted updates in patch 3/3 as well.  I would prefer to commit
>> this with a R-b from Kirti since the NVIDIA driver is also affected by
>> this, but I believe this is the right approach for us long term.  I'm
>> also having trouble finding an ack from Paolo that's particularly
>> relevant for the kvm-vfio portion, but I don't think this is a
>> controversial issue, I feel comfortable that he's onboard with the
>> idea.  Thanks,
> 
> Hi Alex,
> 
> Yes thanks, I will push the v8 asap.
> 
> 
> Hi Kirti,
> 
> I will commit 1/3 with your R-b in next version, hope you are okay
> with that :-)
> 

Yes. 1/3 looks good to me.

Thanks,
Kirti