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

[v6 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 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 specifiy which type it is interested in, and which events.
	  Register it IFF all 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  | 156 +++++++++++++++++++++++++++++++++++++++++++--------
 include/linux/vfio.h |  22 +++++++-
 virt/kvm/vfio.c      |  31 ++++++++++
 3 files changed, 184 insertions(+), 25 deletions(-)

-- 
1.9.1

[v6 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  | 87 +++++++++++++++++++++++++++++++++++++++-------------
 include/linux/vfio.h | 16 ++++++++--
 2 files changed, 78 insertions(+), 25 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index c815067..ec62bec 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2007,23 +2007,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);
@@ -2037,29 +2038,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);
@@ -2074,7 +2065,59 @@ 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)
+		return -EINVAL;
+	if (!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

[v6 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  | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  6 +++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index ec62bec..e2bb197 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;
 
@@ -1015,6 +1018,63 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
 	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)
+{
+	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;
+
+	if (!atomic_inc_not_zero(&group->opened))
+		return -EINVAL;
+
+	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);
+
+	atomic_dec(&group->opened);
+
+	return ret;
+}
+
+static int vfio_unregister_group_notifier(struct vfio_group *group,
+					 struct notifier_block *nb)
+{
+	int ret;
+
+	if (!atomic_inc_not_zero(&group->opened))
+		return -EINVAL;
+
+	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
+
+	atomic_dec(&group->opened);
+	return ret;
+}
+
 /* hold write lock on container->group_lock */
 static int __vfio_container_attach_groups(struct vfio_container *container,
 					  struct vfio_iommu_driver *driver,
@@ -1581,6 +1641,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);
@@ -2088,6 +2151,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;
 	}
@@ -2114,6 +2180,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

[v6 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: [v6 1/3] vfio: vfio_register_notifier: classify iommu notifier

[Alex Williamson]

On Fri, 18 Nov 2016 19:01:47 +0800
Jike Song <jike.song@intel.com> wrote:

> 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  | 87 +++++++++++++++++++++++++++++++++++++++-------------
>  include/linux/vfio.h | 16 ++++++++--
>  2 files changed, 78 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index c815067..ec62bec 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -2007,23 +2007,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);
> @@ -2037,29 +2038,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);
> @@ -2074,7 +2065,59 @@ 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)
> +		return -EINVAL;
> +	if (!events || *events == 0)
> +		return -EINVAL;

nit, we could test all these within a single if()

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

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

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

[Alex Williamson]

On Fri, 18 Nov 2016 19:01:48 +0800
Jike Song <jike.song@intel.com> 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  | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/vfio.h |  6 +++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index ec62bec..e2bb197 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;
>  
> @@ -1015,6 +1018,63 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
>  	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)
> +{
> +	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;
> +
> +	if (!atomic_inc_not_zero(&group->opened))
> +		return -EINVAL;

vfio_group.opened is only used to make sure we don't allow multiple
opens of the group, incrementing it doesn't currently assure the group
remains opened.  What happens if the user process releases the group in
the midst of this?

> +
> +	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);
> +
> +	atomic_dec(&group->opened);
> +
> +	return ret;
> +}
> +
> +static int vfio_unregister_group_notifier(struct vfio_group *group,
> +					 struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	if (!atomic_inc_not_zero(&group->opened))
> +		return -EINVAL;
> +
> +	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
> +
> +	atomic_dec(&group->opened);
> +	return ret;
> +}
> +
>  /* hold write lock on container->group_lock */
>  static int __vfio_container_attach_groups(struct vfio_container *container,
>  					  struct vfio_iommu_driver *driver,
> @@ -1581,6 +1641,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);

Have you tested whether the ordering is correct that the vendor driver
sees the device release first and can therefore unregister notifiers
before the group is released when the user process is killed?

> +
>  	vfio_group_try_dissolve_container(group);
>  
>  	atomic_dec(&group->opened);
> @@ -2088,6 +2151,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;
>  	}
> @@ -2114,6 +2180,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

Re: [v6 1/3] vfio: vfio_register_notifier: classify iommu notifier

[Jike Song]

On 11/19/2016 01:55 AM, Alex Williamson wrote:
> On Fri, 18 Nov 2016 19:01:47 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> 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  | 87 +++++++++++++++++++++++++++++++++++++++-------------
>>  include/linux/vfio.h | 16 ++++++++--
>>  2 files changed, 78 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index c815067..ec62bec 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -2007,23 +2007,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);
>> @@ -2037,29 +2038,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);
>> @@ -2074,7 +2065,59 @@ 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)
>> +		return -EINVAL;
>> +	if (!events || *events == 0)
>> +		return -EINVAL;
> 
> nit, we could test all these within a single if()
> 

Will do that in next version.

>> +
>> +	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;
> 
> -EINVAL
> ^
> 

Ough, sorry .. will fix it in next version.

--
Thanks,
Jike
>> +	}
>> +
>> +	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
>>   */
> 

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

[Jike Song]

On 11/19/2016 01:55 AM, Alex Williamson wrote:
> On Fri, 18 Nov 2016 19:01:48 +0800
> Jike Song <jike.song@intel.com> 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  | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/vfio.h |  6 +++++
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index ec62bec..e2bb197 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;
>>  
>> @@ -1015,6 +1018,63 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
>>  	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)
>> +{
>> +	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;
>> +
>> +	if (!atomic_inc_not_zero(&group->opened))
>> +		return -EINVAL;
> 
> vfio_group.opened is only used to make sure we don't allow multiple
> opens of the group, incrementing it doesn't currently assure the group
> remains opened.  What happens if the user process releases the group in
> the midst of this?

Thanks for pointing out this.
It seems okay to think the group is open by checking group->opened,
but after that I failed to find any existing method to prevent a concurrent
vfio_group_fops_release, could you enlighten me a bit?

>> +
>> +	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);
>> +
>> +	atomic_dec(&group->opened);
>> +
>> +	return ret;
>> +}
>> +
>> +static int vfio_unregister_group_notifier(struct vfio_group *group,
>> +					 struct notifier_block *nb)
>> +{
>> +	int ret;
>> +
>> +	if (!atomic_inc_not_zero(&group->opened))
>> +		return -EINVAL;
>> +
>> +	ret = blocking_notifier_chain_unregister(&group->notifier, nb);
>> +
>> +	atomic_dec(&group->opened);
>> +	return ret;
>> +}
>> +
>>  /* hold write lock on container->group_lock */
>>  static int __vfio_container_attach_groups(struct vfio_container *container,
>>  					  struct vfio_iommu_driver *driver,
>> @@ -1581,6 +1641,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);
> 
> Have you tested whether the ordering is correct that the vendor driver
> sees the device release first and can therefore unregister notifiers
> before the group is released when the user process is killed?
>

Yes, mdev close happens before vfio_group fop close.

--
Thanks,
Jike
>> +
>>  	vfio_group_try_dissolve_container(group);
>>  
>>  	atomic_dec(&group->opened);
>> @@ -2088,6 +2151,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;
>>  	}
>> @@ -2114,6 +2180,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

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

[Alex Williamson]

On Sat, 19 Nov 2016 12:14:29 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/19/2016 01:55 AM, Alex Williamson wrote:
> > On Fri, 18 Nov 2016 19:01:48 +0800
> > Jike Song <jike.song@intel.com> 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  | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/vfio.h |  6 +++++
> >>  2 files changed, 75 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >> index ec62bec..e2bb197 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;
> >>  
> >> @@ -1015,6 +1018,63 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
> >>  	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)
> >> +{
> >> +	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;
> >> +
> >> +	if (!atomic_inc_not_zero(&group->opened))
> >> +		return -EINVAL;  
> > 
> > vfio_group.opened is only used to make sure we don't allow multiple
> > opens of the group, incrementing it doesn't currently assure the group
> > remains opened.  What happens if the user process releases the group in
> > the midst of this?  
> 
> Thanks for pointing out this.
> It seems okay to think the group is open by checking group->opened,
> but after that I failed to find any existing method to prevent a concurrent
> vfio_group_fops_release, could you enlighten me a bit?

I don't think we have such a thing.  I briefly investigated whether we
should add a group mutex rather than the atomic, but at this point I'm
just leaning towards using the same conditions as attaching the iommu
notifier, ie. call vfio_group_add_container_user().  This is also what
we do for vfio_group_get_external_user() so I think it makes sense that
any sort of group reference or notifier registration have the same
requirements.  Thanks,

Alex

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

[Jike Song]

On 11/22/2016 12:56 AM, Alex Williamson wrote:
> On Sat, 19 Nov 2016 12:14:29 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 11/19/2016 01:55 AM, Alex Williamson wrote:
>>> On Fri, 18 Nov 2016 19:01:48 +0800
>>> Jike Song <jike.song@intel.com> 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  | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/vfio.h |  6 +++++
>>>>  2 files changed, 75 insertions(+)
>>>>
>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>> index ec62bec..e2bb197 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;
>>>>  
>>>> @@ -1015,6 +1018,63 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
>>>>  	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)
>>>> +{
>>>> +	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;
>>>> +
>>>> +	if (!atomic_inc_not_zero(&group->opened))
>>>> +		return -EINVAL;  
>>>
>>> vfio_group.opened is only used to make sure we don't allow multiple
>>> opens of the group, incrementing it doesn't currently assure the group
>>> remains opened.  What happens if the user process releases the group in
>>> the midst of this?  
>>
>> Thanks for pointing out this.
>> It seems okay to think the group is open by checking group->opened,
>> but after that I failed to find any existing method to prevent a concurrent
>> vfio_group_fops_release, could you enlighten me a bit?
> 
> I don't think we have such a thing.  I briefly investigated whether we
> should add a group mutex rather than the atomic, but at this point I'm
> just leaning towards using the same conditions as attaching the iommu
> notifier, ie. call vfio_group_add_container_user(). 

Thanks, indeed it seems that a mutex is needed to protect the group itself,
but on the other hand, could open and release of group fops be actually
concurrent? I don't have a clear understanding here but doubt there won't.

> This is also what
> we do for vfio_group_get_external_user() so I think it makes sense that
> any sort of group reference or notifier registration have the same
> requirements.  Thanks,

Cooked a v7 according to your idea, please kindly have a look. Thanks!


--
Thanks,
Jike