[v4 0/3] plumb kvm/vfio to notify kvm:group attaching/detaching

[v4 0/3] plumb kvm/vfio to notify kvm:group attaching/detaching

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


Based on Kirti:

	http://marc.info/?l=kvm&m=147913808504902


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: add vfio_group_notify support
  vfio_register_notifier: also register on the group notifier
  kvm: notify vfio on attaching and detaching

 drivers/vfio/vfio.c  | 34 ++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  4 ++++
 virt/kvm/vfio.c      | 31 +++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+)

-- 
1.9.1

[v4 1/3] vfio: add vfio_group_notify support

[Jike Song]

A vfio_group may be or may not be attached to a KVM instance,
if it is, the user of vfio_group might also want to know which
KVM instance it is attached to, and when it will detach. In VFIO
there are already external APIs for KVM to get/put vfio_group,
by providing a similar vfio_group_notify, KVM can notify the
vfio_group about attaching events.

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

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 0afb58e..b149ced 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 {
@@ -1015,6 +1017,34 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
 	return ret;
 }
 
+void vfio_group_notify(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_notify);
+
+static void vfio_group_register_notifier(struct vfio_group *group,
+					 struct notifier_block *nb)
+{
+	blocking_notifier_chain_register(&group->notifier, nb);
+
+	/*
+	 * The attaching of kvm and vfio_group might already happen, so
+	 * here we replay once on registration.
+	 */
+	if (group->kvm)
+		blocking_notifier_call_chain(&group->notifier,
+					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
+}
+
+static void vfio_group_unregister_notifier(struct vfio_group *group,
+					 struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&group->notifier, nb);
+}
+
 /* hold write lock on container->group_lock */
 static int __vfio_container_attach_groups(struct vfio_container *container,
 					  struct vfio_iommu_driver *driver,
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index d4ce14d..ec9f74f 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -100,6 +100,9 @@ extern void vfio_unregister_iommu_driver(
 extern long vfio_external_check_extension(struct vfio_group *group,
 					  unsigned long arg);
 
+struct kvm;
+extern void vfio_group_notify(struct vfio_group *group, struct kvm *kvm);
+
 /*
  * Sub-module helpers
  */
@@ -149,6 +152,7 @@ extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
 			    int npage);
 
 #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	1
+#define VFIO_GROUP_NOTIFY_SET_KVM	2
 
 extern int vfio_register_notifier(struct device *dev,
 				  struct notifier_block *nb);
-- 
1.9.1

[v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

The user of vfio_register_notifier might care about not only
iommu events but also vfio_group events, so also register the
notifier_block on vfio_group.

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

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index b149ced..2c0eedb 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
 	else
 		ret = -ENOTTY;
 
+	vfio_group_register_notifier(group, nb);
+
 	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
@@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
 	else
 		ret = -ENOTTY;
 
+	vfio_group_unregister_notifier(group, nb);
+
 	up_read(&container->group_lock);
 	vfio_group_try_dissolve_container(group);
 
-- 
1.9.1

[v4 3/3] kvm: notify vfio on attaching and detaching

[Jike Song]

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

Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.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..104082b 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -60,6 +60,32 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
 	symbol_put(vfio_group_put_external_user);
 }
 
+static void kvm_vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm)
+{
+	void (*fn)(struct vfio_group *, struct kvm *);
+
+	fn = symbol_get(vfio_group_notify);
+	if (!fn)
+		return;
+
+	fn(group, kvm);
+
+	symbol_put(vfio_group_notify);
+}
+
+static void kvm_vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm)
+{
+	void (*fn)(struct vfio_group *, struct kvm *);
+
+	fn = symbol_get(vfio_group_notify);
+	if (!fn)
+		return;
+
+	fn(group, NULL);
+
+	symbol_put(vfio_group_notify);
+}
+
 static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group)
 {
 	long (*fn)(struct vfio_group *, unsigned long);
@@ -155,6 +181,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
 		list_add_tail(&kvg->node, &kv->group_list);
 		kvg->vfio_group = vfio_group;
 
+		kvm_vfio_group_attach_kvm(vfio_group, dev->kvm);
+
 		kvm_arch_start_assignment(dev->kvm);
 
 		mutex_unlock(&kv->lock);
@@ -196,6 +224,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
 
 		mutex_unlock(&kv->lock);
 
+		kvm_vfio_group_detach_kvm(vfio_group, dev->kvm);
+
 		kvm_vfio_group_put_external_user(vfio_group);
 
 		kvm_vfio_update_coherency(dev);
@@ -240,6 +270,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
 	struct kvm_vfio_group *kvg, *tmp;
 
 	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_detach_kvm(kvg->vfio_group, dev->kvm);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		list_del(&kvg->node);
 		kfree(kvg);
-- 
1.9.1

Re: [v4 1/3] vfio: add vfio_group_notify support

[Alex Williamson]

On Tue, 15 Nov 2016 19:35:45 +0800
Jike Song <jike.song@intel.com> wrote:

> A vfio_group may be or may not be attached to a KVM instance,
> if it is, the user of vfio_group might also want to know which
> KVM instance it is attached to, and when it will detach. In VFIO
> there are already external APIs for KVM to get/put vfio_group,
> by providing a similar vfio_group_notify, KVM can notify the
> vfio_group about attaching events.
> 
> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/vfio/vfio.c  | 30 ++++++++++++++++++++++++++++++
>  include/linux/vfio.h |  4 ++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 0afb58e..b149ced 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 {
> @@ -1015,6 +1017,34 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
>  	return ret;
>  }
>  
> +void vfio_group_notify(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_notify);

This shouldn't be called vfio_group_notify() if it's specific to kvm.
vfio_group_set_kvm() perhaps.

> +
> +static void vfio_group_register_notifier(struct vfio_group *group,
> +					 struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_register(&group->notifier, nb);
> +
> +	/*
> +	 * The attaching of kvm and vfio_group might already happen, so
> +	 * here we replay once on registration.
> +	 */
> +	if (group->kvm)
> +		blocking_notifier_call_chain(&group->notifier,
> +					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
> +}
> +
> +static void vfio_group_unregister_notifier(struct vfio_group *group,
> +					 struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&group->notifier, nb);
> +}
> +
>  /* hold write lock on container->group_lock */
>  static int __vfio_container_attach_groups(struct vfio_container *container,
>  					  struct vfio_iommu_driver *driver,
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index d4ce14d..ec9f74f 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -100,6 +100,9 @@ extern void vfio_unregister_iommu_driver(
>  extern long vfio_external_check_extension(struct vfio_group *group,
>  					  unsigned long arg);
>  
> +struct kvm;
> +extern void vfio_group_notify(struct vfio_group *group, struct kvm *kvm);
> +
>  /*
>   * Sub-module helpers
>   */
> @@ -149,6 +152,7 @@ extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
>  			    int npage);
>  
>  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	1
> +#define VFIO_GROUP_NOTIFY_SET_KVM	2
>  
>  extern int vfio_register_notifier(struct device *dev,
>  				  struct notifier_block *nb);

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Alex Williamson]

On Tue, 15 Nov 2016 19:35:46 +0800
Jike Song <jike.song@intel.com> wrote:

> The user of vfio_register_notifier might care about not only
> iommu events but also vfio_group events, so also register the
> notifier_block on vfio_group.
> 
> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/vfio/vfio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index b149ced..2c0eedb 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>  	else
>  		ret = -ENOTTY;
>  
> +	vfio_group_register_notifier(group, nb);
> +
>  	up_read(&container->group_lock);
>  	vfio_group_try_dissolve_container(group);
>  
> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>  	else
>  		ret = -ENOTTY;
>  
> +	vfio_group_unregister_notifier(group, nb);
> +
>  	up_read(&container->group_lock);
>  	vfio_group_try_dissolve_container(group);
>  

You haven't addressed the error paths, if the iommu driver returns
error and therefore the {un}register returns error, what is the caller
to expect about the group registration?

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

On 11/16/2016 07:11 AM, Alex Williamson wrote:
> On Tue, 15 Nov 2016 19:35:46 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> The user of vfio_register_notifier might care about not only
>> iommu events but also vfio_group events, so also register the
>> notifier_block on vfio_group.
>>
>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> ---
>>  drivers/vfio/vfio.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index b149ced..2c0eedb 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>>  	else
>>  		ret = -ENOTTY;
>>  
>> +	vfio_group_register_notifier(group, nb);
>> +
>>  	up_read(&container->group_lock);
>>  	vfio_group_try_dissolve_container(group);
>>  
>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>>  	else
>>  		ret = -ENOTTY;
>>  
>> +	vfio_group_unregister_notifier(group, nb);
>> +
>>  	up_read(&container->group_lock);
>>  	vfio_group_try_dissolve_container(group);
>>  
> 
> You haven't addressed the error paths, if the iommu driver returns
> error and therefore the {un}register returns error, what is the caller
> to expect about the group registration?
> 

Will change to:

	driver = container->iommu_driver;
	if (likely(driver && driver->ops->register_notifier))
		ret = driver->ops->register_notifier(container->iommu_data, nb);
	else
		ret = -ENOTTY;
	if (ret)
		goto err_register_iommu;

	ret = vfio_group_register_notifier(group, nb);
	if (ret)
		driver->ops->unregister_notifier(container->iommu_data, nb);

err_register_iommu:
	up_read(&container->group_lock);
	vfio_group_try_dissolve_container(group);

err_register_nb:
	vfio_group_put(group);
	return ret;


--
Thanks,
Jike

Re: [v4 1/3] vfio: add vfio_group_notify support

[Jike Song]

On 11/16/2016 07:11 AM, Alex Williamson wrote:
> On Tue, 15 Nov 2016 19:35:45 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> A vfio_group may be or may not be attached to a KVM instance,
>> if it is, the user of vfio_group might also want to know which
>> KVM instance it is attached to, and when it will detach. In VFIO
>> there are already external APIs for KVM to get/put vfio_group,
>> by providing a similar vfio_group_notify, KVM can notify the
>> vfio_group about attaching events.
>>
>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> ---
>>  drivers/vfio/vfio.c  | 30 ++++++++++++++++++++++++++++++
>>  include/linux/vfio.h |  4 ++++
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 0afb58e..b149ced 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 {
>> @@ -1015,6 +1017,34 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
>>  	return ret;
>>  }
>>  
>> +void vfio_group_notify(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_notify);
> 
> This shouldn't be called vfio_group_notify() if it's specific to kvm.
> vfio_group_set_kvm() perhaps.
> 

will change to that. thanks!

--
Thanks,
Jike

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Alex Williamson]

On Wed, 16 Nov 2016 11:01:37 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/16/2016 07:11 AM, Alex Williamson wrote:
> > On Tue, 15 Nov 2016 19:35:46 +0800
> > Jike Song <jike.song@intel.com> wrote:
> >   
> >> The user of vfio_register_notifier might care about not only
> >> iommu events but also vfio_group events, so also register the
> >> notifier_block on vfio_group.
> >>
> >> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Signed-off-by: Jike Song <jike.song@intel.com>
> >> ---
> >>  drivers/vfio/vfio.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >> index b149ced..2c0eedb 100644
> >> --- a/drivers/vfio/vfio.c
> >> +++ b/drivers/vfio/vfio.c
> >> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
> >>  	else
> >>  		ret = -ENOTTY;
> >>  
> >> +	vfio_group_register_notifier(group, nb);
> >> +
> >>  	up_read(&container->group_lock);
> >>  	vfio_group_try_dissolve_container(group);
> >>  
> >> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
> >>  	else
> >>  		ret = -ENOTTY;
> >>  
> >> +	vfio_group_unregister_notifier(group, nb);
> >> +
> >>  	up_read(&container->group_lock);
> >>  	vfio_group_try_dissolve_container(group);
> >>    
> > 
> > You haven't addressed the error paths, if the iommu driver returns
> > error and therefore the {un}register returns error, what is the caller
> > to expect about the group registration?
> >   
> 
> Will change to:
> 
> 	driver = container->iommu_driver;
> 	if (likely(driver && driver->ops->register_notifier))
> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
> 	else
> 		ret = -ENOTTY;
> 	if (ret)
> 		goto err_register_iommu;
> 
> 	ret = vfio_group_register_notifier(group, nb);
> 	if (ret)
> 		driver->ops->unregister_notifier(container->iommu_data, nb);
> 
> err_register_iommu:
> 	up_read(&container->group_lock);
> 	vfio_group_try_dissolve_container(group);
> 
> err_register_nb:
> 	vfio_group_put(group);
> 	return ret;



What if a vendor driver only cares about the kvm state and doesn't pin
memory (ie. no DMA) or only cares about iommu and not group notifies?
If we handled notifier_fn_t 'action' as a bitmask then we could have the
registrar specify which notification they wanted (a mask/filter), so if
they only want KVM, we only send that notify, if they only want UNMAPs,
etc. Then we know whether iommu registration is required.  As a bonus,
we could add a pr_info() indicating vendors that ask for KVM
notification so that we can interrogate why they think they need it.
The downside is that handling action as a bitmask means that we limit
the number of actions we have available (32 or 64 bits worth).  That
limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,

Alex

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Kirti Wankhede]

On 11/16/2016 9:13 AM, Alex Williamson wrote:
> On Wed, 16 Nov 2016 11:01:37 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 11/16/2016 07:11 AM, Alex Williamson wrote:
>>> On Tue, 15 Nov 2016 19:35:46 +0800
>>> Jike Song <jike.song@intel.com> wrote:
>>>   
>>>> The user of vfio_register_notifier might care about not only
>>>> iommu events but also vfio_group events, so also register the
>>>> notifier_block on vfio_group.
>>>>
>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>>> ---
>>>>  drivers/vfio/vfio.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>> index b149ced..2c0eedb 100644
>>>> --- a/drivers/vfio/vfio.c
>>>> +++ b/drivers/vfio/vfio.c
>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>>>>  	else
>>>>  		ret = -ENOTTY;
>>>>  
>>>> +	vfio_group_register_notifier(group, nb);
>>>> +
>>>>  	up_read(&container->group_lock);
>>>>  	vfio_group_try_dissolve_container(group);
>>>>  
>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>>>>  	else
>>>>  		ret = -ENOTTY;
>>>>  
>>>> +	vfio_group_unregister_notifier(group, nb);
>>>> +
>>>>  	up_read(&container->group_lock);
>>>>  	vfio_group_try_dissolve_container(group);
>>>>    
>>>
>>> You haven't addressed the error paths, if the iommu driver returns
>>> error and therefore the {un}register returns error, what is the caller
>>> to expect about the group registration?
>>>   
>>
>> Will change to:
>>
>> 	driver = container->iommu_driver;
>> 	if (likely(driver && driver->ops->register_notifier))
>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
>> 	else
>> 		ret = -ENOTTY;
>> 	if (ret)
>> 		goto err_register_iommu;
>>
>> 	ret = vfio_group_register_notifier(group, nb);
>> 	if (ret)
>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
>>
>> err_register_iommu:
>> 	up_read(&container->group_lock);
>> 	vfio_group_try_dissolve_container(group);
>>
>> err_register_nb:
>> 	vfio_group_put(group);
>> 	return ret;
> 
> 
> 
> What if a vendor driver only cares about the kvm state and doesn't pin
> memory (ie. no DMA) or only cares about iommu and not group notifies?
> If we handled notifier_fn_t 'action' as a bitmask then we could have the
> registrar specify which notification they wanted (a mask/filter), so if
> they only want KVM, we only send that notify, if they only want UNMAPs,
> etc. Then we know whether iommu registration is required.  As a bonus,
> we could add a pr_info() indicating vendors that ask for KVM
> notification so that we can interrogate why they think they need it.
> The downside is that handling action as a bitmask means that we limit
> the number of actions we have available (32 or 64 bits worth).  That
> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
> 

As per my understanding, this bitmask is input to
vfio_register_notifier() and vfio_unregister_notifier(), right?

These functions are not called from vendor driver directly, these are
called from vfio_mdev. Then should this bitmask be part of parent_ops
that vendor driver can specify?

Thanks,
Kirti

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

On 11/16/2016 05:14 PM, Kirti Wankhede wrote:
> On 11/16/2016 9:13 AM, Alex Williamson wrote:
>> On Wed, 16 Nov 2016 11:01:37 +0800
>> Jike Song <jike.song@intel.com> wrote:
>>
>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:
>>>> On Tue, 15 Nov 2016 19:35:46 +0800
>>>> Jike Song <jike.song@intel.com> wrote:
>>>>   
>>>>> The user of vfio_register_notifier might care about not only
>>>>> iommu events but also vfio_group events, so also register the
>>>>> notifier_block on vfio_group.
>>>>>
>>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>>>> ---
>>>>>  drivers/vfio/vfio.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>>> index b149ced..2c0eedb 100644
>>>>> --- a/drivers/vfio/vfio.c
>>>>> +++ b/drivers/vfio/vfio.c
>>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>>>>>  	else
>>>>>  		ret = -ENOTTY;
>>>>>  
>>>>> +	vfio_group_register_notifier(group, nb);
>>>>> +
>>>>>  	up_read(&container->group_lock);
>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>  
>>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>>>>>  	else
>>>>>  		ret = -ENOTTY;
>>>>>  
>>>>> +	vfio_group_unregister_notifier(group, nb);
>>>>> +
>>>>>  	up_read(&container->group_lock);
>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>    
>>>>
>>>> You haven't addressed the error paths, if the iommu driver returns
>>>> error and therefore the {un}register returns error, what is the caller
>>>> to expect about the group registration?
>>>>   
>>>
>>> Will change to:
>>>
>>> 	driver = container->iommu_driver;
>>> 	if (likely(driver && driver->ops->register_notifier))
>>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
>>> 	else
>>> 		ret = -ENOTTY;
>>> 	if (ret)
>>> 		goto err_register_iommu;
>>>
>>> 	ret = vfio_group_register_notifier(group, nb);
>>> 	if (ret)
>>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
>>>
>>> err_register_iommu:
>>> 	up_read(&container->group_lock);
>>> 	vfio_group_try_dissolve_container(group);
>>>
>>> err_register_nb:
>>> 	vfio_group_put(group);
>>> 	return ret;
>>
>>
>>
>> What if a vendor driver only cares about the kvm state and doesn't pin
>> memory (ie. no DMA) or only cares about iommu and not group notifies?
>> If we handled notifier_fn_t 'action' as a bitmask then we could have the
>> registrar specify which notification they wanted (a mask/filter), so if
>> they only want KVM, we only send that notify, if they only want UNMAPs,
>> etc. Then we know whether iommu registration is required.  As a bonus,
>> we could add a pr_info() indicating vendors that ask for KVM
>> notification so that we can interrogate why they think they need it.
>> The downside is that handling action as a bitmask means that we limit
>> the number of actions we have available (32 or 64 bits worth).  That
>> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
>>
> 
> As per my understanding, this bitmask is input to
> vfio_register_notifier() and vfio_unregister_notifier(), right?
> 
> These functions are not called from vendor driver directly, these are
> called from vfio_mdev. Then should this bitmask be part of parent_ops
> that vendor driver can specify?

I think so, there should be a 'notifiler_filter' in parent_ops to indicate
that. A draft patch to show Alex's proposal:


diff a/include/linux/vfio.h b/include/linux/vfio.h
@@ -153,13 +153,15 @@ 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_NOTIFY_IOMMU_DMA_UNMAP	BIT(0)
+#define VFIO_NOTIFY_GROUP_SET_KVM	BIT(1)
 
 extern int vfio_register_notifier(struct device *dev,
+				  const unsigned long filter,
 				  struct notifier_block *nb);
 
 extern int vfio_unregister_notifier(struct device *dev,
+				    const unsigned long filter,
 				    struct notifier_block *nb);
 /*
  * IRQfd - generic

diff a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
@@ -30,6 +30,9 @@ static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action,
 	struct mdev_device *mdev = container_of(nb, struct mdev_device, nb);
 	struct parent_device *parent = mdev->parent;
 
+	if (!(parent->ops->notifier_filter & action))
+		return -EINVAL;
+
 	return parent->ops->notifier(mdev, action, data);
 }
 
@@ -47,14 +50,18 @@ static int vfio_mdev_open(void *device_data)
 
 	if (likely(parent->ops->notifier)) {
 		mdev->nb.notifier_call = vfio_mdev_notifier;
-		if (vfio_register_notifier(&mdev->dev, &mdev->nb))
+		if (vfio_register_notifier(&mdev->dev,
+					   parent->ops->notifier_filter,
+					   &mdev->nb))
 			pr_err("Failed to register notifier for mdev\n");
 	}
 
 	ret = parent->ops->open(mdev);
 	if (ret) {
 		if (likely(parent->ops->notifier))
-			vfio_unregister_notifier(&mdev->dev, &mdev->nb);
+			vfio_unregister_notifier(&mdev->dev,
+						parent->ops->notifier_filter,
+						&mdev->nb);
 		module_put(THIS_MODULE);
 	}
 
@@ -70,7 +77,9 @@ static void vfio_mdev_release(void *device_data)
 		parent->ops->release(mdev);
 
 	if (likely(parent->ops->notifier)) {
-		if (vfio_unregister_notifier(&mdev->dev, &mdev->nb))
+		if (vfio_unregister_notifier(&mdev->dev,
+					     parent->ops->notifier_filter,
+					     &mdev->nb))
 			pr_err("Failed to unregister notifier for mdev\n");
 	}
 
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 94c4303..e015c1b 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -100,6 +100,7 @@ struct parent_ops {
 	struct module   *owner;
 	const struct attribute_group **dev_attr_groups;
 	const struct attribute_group **mdev_attr_groups;
+	const unsigned long notifier_filter;
 	struct attribute_group **supported_type_groups;
 
 	int     (*create)(struct kobject *kobj, struct mdev_device *mdev);





and the vfio_register_notifier:

int vfio_register_notifier(struct device *dev, const unsigned long filter,
			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 out_put_group;

	container = group->container;
	down_read(&container->group_lock);

	if (filter & VFIO_NOTIFY_GROUP_SET_KVM) {
		pr_info("vfio_group dependency on KVM should be avoided\n");

		ret = vfio_group_register_notifier(group, nb);
		if (ret)
			goto out_unlock;
	}

	if (filter & VFIO_NOTIFY_IOMMU_DMA_UNMAP) {
		driver = container->iommu_driver;
		if (likely(driver && driver->ops->register_notifier))
			ret = driver->ops->register_notifier(container->iommu_data, nb);
		else
			ret = -ENOTTY;
		if (ret)
			goto out_unregiter_group;
	}

	up_read(&container->group_lock);
	vfio_group_try_dissolve_container(group);
	vfio_group_put(group);
	return ret;

out_unregiter_group:
	if (filter & VFIO_NOTIFY_GROUP_SET_KVM)
		vfio_group_unregister_notifier(group, nb);

out_unlock:
	up_read(&container->group_lock);
	vfio_group_try_dissolve_container(group);

out_put_group:
	vfio_group_put(group);
	return ret;
}
EXPORT_SYMBOL(vfio_register_notifier);




Comments?


--
Thanks,
Jike

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Kirti Wankhede]

On 11/16/2016 3:07 PM, Jike Song wrote:
> On 11/16/2016 05:14 PM, Kirti Wankhede wrote:
>> On 11/16/2016 9:13 AM, Alex Williamson wrote:
>>> On Wed, 16 Nov 2016 11:01:37 +0800
>>> Jike Song <jike.song@intel.com> wrote:
>>>
>>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:
>>>>> On Tue, 15 Nov 2016 19:35:46 +0800
>>>>> Jike Song <jike.song@intel.com> wrote:
>>>>>   
>>>>>> The user of vfio_register_notifier might care about not only
>>>>>> iommu events but also vfio_group events, so also register the
>>>>>> notifier_block on vfio_group.
>>>>>>
>>>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>>>>> ---
>>>>>>  drivers/vfio/vfio.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>>>> index b149ced..2c0eedb 100644
>>>>>> --- a/drivers/vfio/vfio.c
>>>>>> +++ b/drivers/vfio/vfio.c
>>>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>>>>>>  	else
>>>>>>  		ret = -ENOTTY;
>>>>>>  
>>>>>> +	vfio_group_register_notifier(group, nb);
>>>>>> +
>>>>>>  	up_read(&container->group_lock);
>>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>>  
>>>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>>>>>>  	else
>>>>>>  		ret = -ENOTTY;
>>>>>>  
>>>>>> +	vfio_group_unregister_notifier(group, nb);
>>>>>> +
>>>>>>  	up_read(&container->group_lock);
>>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>>    
>>>>>
>>>>> You haven't addressed the error paths, if the iommu driver returns
>>>>> error and therefore the {un}register returns error, what is the caller
>>>>> to expect about the group registration?
>>>>>   
>>>>
>>>> Will change to:
>>>>
>>>> 	driver = container->iommu_driver;
>>>> 	if (likely(driver && driver->ops->register_notifier))
>>>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
>>>> 	else
>>>> 		ret = -ENOTTY;
>>>> 	if (ret)
>>>> 		goto err_register_iommu;
>>>>
>>>> 	ret = vfio_group_register_notifier(group, nb);
>>>> 	if (ret)
>>>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
>>>>
>>>> err_register_iommu:
>>>> 	up_read(&container->group_lock);
>>>> 	vfio_group_try_dissolve_container(group);
>>>>
>>>> err_register_nb:
>>>> 	vfio_group_put(group);
>>>> 	return ret;
>>>
>>>
>>>
>>> What if a vendor driver only cares about the kvm state and doesn't pin
>>> memory (ie. no DMA) or only cares about iommu and not group notifies?
>>> If we handled notifier_fn_t 'action' as a bitmask then we could have the
>>> registrar specify which notification they wanted (a mask/filter), so if
>>> they only want KVM, we only send that notify, if they only want UNMAPs,
>>> etc. Then we know whether iommu registration is required.  As a bonus,
>>> we could add a pr_info() indicating vendors that ask for KVM
>>> notification so that we can interrogate why they think they need it.
>>> The downside is that handling action as a bitmask means that we limit
>>> the number of actions we have available (32 or 64 bits worth).  That
>>> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
>>>
>>
>> As per my understanding, this bitmask is input to
>> vfio_register_notifier() and vfio_unregister_notifier(), right?
>>
>> These functions are not called from vendor driver directly, these are
>> called from vfio_mdev. Then should this bitmask be part of parent_ops
>> that vendor driver can specify?
> 
> I think so, there should be a 'notifiler_filter' in parent_ops to indicate
> that. A draft patch to show Alex's proposal:
> 

In that case how can we use bitmask to register multiple actions from
backend iommu module (now we only have VFIO_IOMMU_NOTIFY_DMA_UNMAP
action)? Even if we pass bitmask to backend iommu modules
register_notifier(), backend module would have to create separate
notifier for each action?

Instead of taking bitmask as input from vendor driver, vendor driver's
callback can send return depending on action if they don't want to get
future notification:

#define NOTIFY_DONE             0x0000          /* Don't care */
#define NOTIFY_OK               0x0001          /* Suits me */
#define NOTIFY_STOP_MASK        0x8000          /* Don't call further */
#define NOTIFY_BAD              (NOTIFY_STOP_MASK|0x0002)
                                                /* Bad/Veto action */
/*
 * Clean way to return from the notifier and stop further calls.
 */
#define NOTIFY_STOP             (NOTIFY_OK|NOTIFY_STOP_MASK)


Thanks,
Kirti

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Alex Williamson]

On Wed, 16 Nov 2016 16:14:24 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 11/16/2016 3:07 PM, Jike Song wrote:
> > On 11/16/2016 05:14 PM, Kirti Wankhede wrote:  
> >> On 11/16/2016 9:13 AM, Alex Williamson wrote:  
> >>> On Wed, 16 Nov 2016 11:01:37 +0800
> >>> Jike Song <jike.song@intel.com> wrote:
> >>>  
> >>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:  
> >>>>> On Tue, 15 Nov 2016 19:35:46 +0800
> >>>>> Jike Song <jike.song@intel.com> wrote:
> >>>>>     
> >>>>>> The user of vfio_register_notifier might care about not only
> >>>>>> iommu events but also vfio_group events, so also register the
> >>>>>> notifier_block on vfio_group.
> >>>>>>
> >>>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
> >>>>>> Signed-off-by: Jike Song <jike.song@intel.com>
> >>>>>> ---
> >>>>>>  drivers/vfio/vfio.c | 4 ++++
> >>>>>>  1 file changed, 4 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >>>>>> index b149ced..2c0eedb 100644
> >>>>>> --- a/drivers/vfio/vfio.c
> >>>>>> +++ b/drivers/vfio/vfio.c
> >>>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
> >>>>>>  	else
> >>>>>>  		ret = -ENOTTY;
> >>>>>>  
> >>>>>> +	vfio_group_register_notifier(group, nb);
> >>>>>> +
> >>>>>>  	up_read(&container->group_lock);
> >>>>>>  	vfio_group_try_dissolve_container(group);
> >>>>>>  
> >>>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
> >>>>>>  	else
> >>>>>>  		ret = -ENOTTY;
> >>>>>>  
> >>>>>> +	vfio_group_unregister_notifier(group, nb);
> >>>>>> +
> >>>>>>  	up_read(&container->group_lock);
> >>>>>>  	vfio_group_try_dissolve_container(group);
> >>>>>>      
> >>>>>
> >>>>> You haven't addressed the error paths, if the iommu driver returns
> >>>>> error and therefore the {un}register returns error, what is the caller
> >>>>> to expect about the group registration?
> >>>>>     
> >>>>
> >>>> Will change to:
> >>>>
> >>>> 	driver = container->iommu_driver;
> >>>> 	if (likely(driver && driver->ops->register_notifier))
> >>>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
> >>>> 	else
> >>>> 		ret = -ENOTTY;
> >>>> 	if (ret)
> >>>> 		goto err_register_iommu;
> >>>>
> >>>> 	ret = vfio_group_register_notifier(group, nb);
> >>>> 	if (ret)
> >>>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
> >>>>
> >>>> err_register_iommu:
> >>>> 	up_read(&container->group_lock);
> >>>> 	vfio_group_try_dissolve_container(group);
> >>>>
> >>>> err_register_nb:
> >>>> 	vfio_group_put(group);
> >>>> 	return ret;  
> >>>
> >>>
> >>>
> >>> What if a vendor driver only cares about the kvm state and doesn't pin
> >>> memory (ie. no DMA) or only cares about iommu and not group notifies?
> >>> If we handled notifier_fn_t 'action' as a bitmask then we could have the
> >>> registrar specify which notification they wanted (a mask/filter), so if
> >>> they only want KVM, we only send that notify, if they only want UNMAPs,
> >>> etc. Then we know whether iommu registration is required.  As a bonus,
> >>> we could add a pr_info() indicating vendors that ask for KVM
> >>> notification so that we can interrogate why they think they need it.
> >>> The downside is that handling action as a bitmask means that we limit
> >>> the number of actions we have available (32 or 64 bits worth).  That
> >>> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
> >>>  
> >>
> >> As per my understanding, this bitmask is input to
> >> vfio_register_notifier() and vfio_unregister_notifier(), right?
> >>
> >> These functions are not called from vendor driver directly, these are
> >> called from vfio_mdev. Then should this bitmask be part of parent_ops
> >> that vendor driver can specify?  
> > 
> > I think so, there should be a 'notifiler_filter' in parent_ops to indicate
> > that. A draft patch to show Alex's proposal:
> >   
> 
> In that case how can we use bitmask to register multiple actions from
> backend iommu module (now we only have VFIO_IOMMU_NOTIFY_DMA_UNMAP
> action)? Even if we pass bitmask to backend iommu modules
> register_notifier(), backend module would have to create separate
> notifier for each action?
> 
> Instead of taking bitmask as input from vendor driver, vendor driver's
> callback can send return depending on action if they don't want to get
> future notification:
> 
> #define NOTIFY_DONE             0x0000          /* Don't care */
> #define NOTIFY_OK               0x0001          /* Suits me */
> #define NOTIFY_STOP_MASK        0x8000          /* Don't call further */
> #define NOTIFY_BAD              (NOTIFY_STOP_MASK|0x0002)
>                                                 /* Bad/Veto action */
> /*
>  * Clean way to return from the notifier and stop further calls.
>  */
> #define NOTIFY_STOP             (NOTIFY_OK|NOTIFY_STOP_MASK)

I don't think that's a correct interpretation of NOTIFY_STOP_MASK, it
intends to say "this notification was for me, I've handled it, no need
to continue through the call chain".  It does not imply "don't tell me
about this event in the future", entries in the call chain don't have
the ability to suppress certain events.

Also note that NOTIFY_STOP_MASK is returned by
blocking_notifier_call_chain(), so we can pr_warn if a call chain
member is potentially preventing other notify-ees from receiving the
event.

Should we be revisiting the question of whether notifiers are registered
as part of the mdev-core?  In the current proposals
vfio_{un}register_notifier() is exported, so it's really just a
convenience that the mdev core registers it on behalf of the vendor
driver.  Vendor drivers could register on open and unregister on
release (vfio could additionally do housekeeping on release).  We
already expect vendor drivers to call vfio_{un}pin_pages() directly.
Then the vendor driver would provide filter flags directly and we'd
make that part of the mdev-vendor driver API a little more flexible.
Thanks,

Alex

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Kirti Wankhede]

On 11/16/2016 11:18 PM, Alex Williamson wrote:
> On Wed, 16 Nov 2016 16:14:24 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 11/16/2016 3:07 PM, Jike Song wrote:
>>> On 11/16/2016 05:14 PM, Kirti Wankhede wrote:  
>>>> On 11/16/2016 9:13 AM, Alex Williamson wrote:  
>>>>> On Wed, 16 Nov 2016 11:01:37 +0800
>>>>> Jike Song <jike.song@intel.com> wrote:
>>>>>  
>>>>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:  
>>>>>>> On Tue, 15 Nov 2016 19:35:46 +0800
>>>>>>> Jike Song <jike.song@intel.com> wrote:
>>>>>>>     
>>>>>>>> The user of vfio_register_notifier might care about not only
>>>>>>>> iommu events but also vfio_group events, so also register the
>>>>>>>> notifier_block on vfio_group.
>>>>>>>>
>>>>>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>>>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>>>>>>> ---
>>>>>>>>  drivers/vfio/vfio.c | 4 ++++
>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>>>>>> index b149ced..2c0eedb 100644
>>>>>>>> --- a/drivers/vfio/vfio.c
>>>>>>>> +++ b/drivers/vfio/vfio.c
>>>>>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>>>>>>>>  	else
>>>>>>>>  		ret = -ENOTTY;
>>>>>>>>  
>>>>>>>> +	vfio_group_register_notifier(group, nb);
>>>>>>>> +
>>>>>>>>  	up_read(&container->group_lock);
>>>>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>>>>  
>>>>>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>>>>>>>>  	else
>>>>>>>>  		ret = -ENOTTY;
>>>>>>>>  
>>>>>>>> +	vfio_group_unregister_notifier(group, nb);
>>>>>>>> +
>>>>>>>>  	up_read(&container->group_lock);
>>>>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>>>>      
>>>>>>>
>>>>>>> You haven't addressed the error paths, if the iommu driver returns
>>>>>>> error and therefore the {un}register returns error, what is the caller
>>>>>>> to expect about the group registration?
>>>>>>>     
>>>>>>
>>>>>> Will change to:
>>>>>>
>>>>>> 	driver = container->iommu_driver;
>>>>>> 	if (likely(driver && driver->ops->register_notifier))
>>>>>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
>>>>>> 	else
>>>>>> 		ret = -ENOTTY;
>>>>>> 	if (ret)
>>>>>> 		goto err_register_iommu;
>>>>>>
>>>>>> 	ret = vfio_group_register_notifier(group, nb);
>>>>>> 	if (ret)
>>>>>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
>>>>>>
>>>>>> err_register_iommu:
>>>>>> 	up_read(&container->group_lock);
>>>>>> 	vfio_group_try_dissolve_container(group);
>>>>>>
>>>>>> err_register_nb:
>>>>>> 	vfio_group_put(group);
>>>>>> 	return ret;  
>>>>>
>>>>>
>>>>>
>>>>> What if a vendor driver only cares about the kvm state and doesn't pin
>>>>> memory (ie. no DMA) or only cares about iommu and not group notifies?
>>>>> If we handled notifier_fn_t 'action' as a bitmask then we could have the
>>>>> registrar specify which notification they wanted (a mask/filter), so if
>>>>> they only want KVM, we only send that notify, if they only want UNMAPs,
>>>>> etc. Then we know whether iommu registration is required.  As a bonus,
>>>>> we could add a pr_info() indicating vendors that ask for KVM
>>>>> notification so that we can interrogate why they think they need it.
>>>>> The downside is that handling action as a bitmask means that we limit
>>>>> the number of actions we have available (32 or 64 bits worth).  That
>>>>> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
>>>>>  
>>>>
>>>> As per my understanding, this bitmask is input to
>>>> vfio_register_notifier() and vfio_unregister_notifier(), right?
>>>>
>>>> These functions are not called from vendor driver directly, these are
>>>> called from vfio_mdev. Then should this bitmask be part of parent_ops
>>>> that vendor driver can specify?  
>>>
>>> I think so, there should be a 'notifiler_filter' in parent_ops to indicate
>>> that. A draft patch to show Alex's proposal:
>>>   
>>
>> In that case how can we use bitmask to register multiple actions from
>> backend iommu module (now we only have VFIO_IOMMU_NOTIFY_DMA_UNMAP
>> action)? Even if we pass bitmask to backend iommu modules
>> register_notifier(), backend module would have to create separate
>> notifier for each action?
>>
>> Instead of taking bitmask as input from vendor driver, vendor driver's
>> callback can send return depending on action if they don't want to get
>> future notification:
>>
>> #define NOTIFY_DONE             0x0000          /* Don't care */
>> #define NOTIFY_OK               0x0001          /* Suits me */
>> #define NOTIFY_STOP_MASK        0x8000          /* Don't call further */
>> #define NOTIFY_BAD              (NOTIFY_STOP_MASK|0x0002)
>>                                                 /* Bad/Veto action */
>> /*
>>  * Clean way to return from the notifier and stop further calls.
>>  */
>> #define NOTIFY_STOP             (NOTIFY_OK|NOTIFY_STOP_MASK)
> 
> I don't think that's a correct interpretation of NOTIFY_STOP_MASK, it
> intends to say "this notification was for me, I've handled it, no need
> to continue through the call chain".  It does not imply "don't tell me
> about this event in the future", entries in the call chain don't have
> the ability to suppress certain events.
> 

OK, thanks for correcting me.

> Also note that NOTIFY_STOP_MASK is returned by
> blocking_notifier_call_chain(), so we can pr_warn if a call chain
> member is potentially preventing other notify-ees from receiving the
> event.
> 
> Should we be revisiting the question of whether notifiers are registered
> as part of the mdev-core?  In the current proposals
> vfio_{un}register_notifier() is exported, so it's really just a
> convenience that the mdev core registers it on behalf of the vendor
> driver.  Vendor drivers could register on open and unregister on
> release (vfio could additionally do housekeeping on release).  We
> already expect vendor drivers to call vfio_{un}pin_pages() directly.
> Then the vendor driver would provide filter flags directly and we'd
> make that part of the mdev-vendor driver API a little more flexible.

Ok. But that still doesn't solve the problem if more actions need to be
added to backend iommu module.

If vendor driver notifier callback is called, vendor driver can return
NOTIFY_DONE or NOTIFY_OK for the action they are not interested in.

Thanks,
Kirti

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Alex Williamson]

On Thu, 17 Nov 2016 00:42:48 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 11/16/2016 11:18 PM, Alex Williamson wrote:
> > On Wed, 16 Nov 2016 16:14:24 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 11/16/2016 3:07 PM, Jike Song wrote:  
> >>> On 11/16/2016 05:14 PM, Kirti Wankhede wrote:    
> >>>> On 11/16/2016 9:13 AM, Alex Williamson wrote:    
> >>>>> On Wed, 16 Nov 2016 11:01:37 +0800
> >>>>> Jike Song <jike.song@intel.com> wrote:
> >>>>>    
> >>>>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:    
> >>>>>>> On Tue, 15 Nov 2016 19:35:46 +0800
> >>>>>>> Jike Song <jike.song@intel.com> wrote:
> >>>>>>>       
> >>>>>>>> The user of vfio_register_notifier might care about not only
> >>>>>>>> iommu events but also vfio_group events, so also register the
> >>>>>>>> notifier_block on vfio_group.
> >>>>>>>>
> >>>>>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
> >>>>>>>> Signed-off-by: Jike Song <jike.song@intel.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/vfio/vfio.c | 4 ++++
> >>>>>>>>  1 file changed, 4 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >>>>>>>> index b149ced..2c0eedb 100644
> >>>>>>>> --- a/drivers/vfio/vfio.c
> >>>>>>>> +++ b/drivers/vfio/vfio.c
> >>>>>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
> >>>>>>>>  	else
> >>>>>>>>  		ret = -ENOTTY;
> >>>>>>>>  
> >>>>>>>> +	vfio_group_register_notifier(group, nb);
> >>>>>>>> +
> >>>>>>>>  	up_read(&container->group_lock);
> >>>>>>>>  	vfio_group_try_dissolve_container(group);
> >>>>>>>>  
> >>>>>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
> >>>>>>>>  	else
> >>>>>>>>  		ret = -ENOTTY;
> >>>>>>>>  
> >>>>>>>> +	vfio_group_unregister_notifier(group, nb);
> >>>>>>>> +
> >>>>>>>>  	up_read(&container->group_lock);
> >>>>>>>>  	vfio_group_try_dissolve_container(group);
> >>>>>>>>        
> >>>>>>>
> >>>>>>> You haven't addressed the error paths, if the iommu driver returns
> >>>>>>> error and therefore the {un}register returns error, what is the caller
> >>>>>>> to expect about the group registration?
> >>>>>>>       
> >>>>>>
> >>>>>> Will change to:
> >>>>>>
> >>>>>> 	driver = container->iommu_driver;
> >>>>>> 	if (likely(driver && driver->ops->register_notifier))
> >>>>>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
> >>>>>> 	else
> >>>>>> 		ret = -ENOTTY;
> >>>>>> 	if (ret)
> >>>>>> 		goto err_register_iommu;
> >>>>>>
> >>>>>> 	ret = vfio_group_register_notifier(group, nb);
> >>>>>> 	if (ret)
> >>>>>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
> >>>>>>
> >>>>>> err_register_iommu:
> >>>>>> 	up_read(&container->group_lock);
> >>>>>> 	vfio_group_try_dissolve_container(group);
> >>>>>>
> >>>>>> err_register_nb:
> >>>>>> 	vfio_group_put(group);
> >>>>>> 	return ret;    
> >>>>>
> >>>>>
> >>>>>
> >>>>> What if a vendor driver only cares about the kvm state and doesn't pin
> >>>>> memory (ie. no DMA) or only cares about iommu and not group notifies?
> >>>>> If we handled notifier_fn_t 'action' as a bitmask then we could have the
> >>>>> registrar specify which notification they wanted (a mask/filter), so if
> >>>>> they only want KVM, we only send that notify, if they only want UNMAPs,
> >>>>> etc. Then we know whether iommu registration is required.  As a bonus,
> >>>>> we could add a pr_info() indicating vendors that ask for KVM
> >>>>> notification so that we can interrogate why they think they need it.
> >>>>> The downside is that handling action as a bitmask means that we limit
> >>>>> the number of actions we have available (32 or 64 bits worth).  That
> >>>>> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
> >>>>>    
> >>>>
> >>>> As per my understanding, this bitmask is input to
> >>>> vfio_register_notifier() and vfio_unregister_notifier(), right?
> >>>>
> >>>> These functions are not called from vendor driver directly, these are
> >>>> called from vfio_mdev. Then should this bitmask be part of parent_ops
> >>>> that vendor driver can specify?    
> >>>
> >>> I think so, there should be a 'notifiler_filter' in parent_ops to indicate
> >>> that. A draft patch to show Alex's proposal:
> >>>     
> >>
> >> In that case how can we use bitmask to register multiple actions from
> >> backend iommu module (now we only have VFIO_IOMMU_NOTIFY_DMA_UNMAP
> >> action)? Even if we pass bitmask to backend iommu modules
> >> register_notifier(), backend module would have to create separate
> >> notifier for each action?
> >>
> >> Instead of taking bitmask as input from vendor driver, vendor driver's
> >> callback can send return depending on action if they don't want to get
> >> future notification:
> >>
> >> #define NOTIFY_DONE             0x0000          /* Don't care */
> >> #define NOTIFY_OK               0x0001          /* Suits me */
> >> #define NOTIFY_STOP_MASK        0x8000          /* Don't call further */
> >> #define NOTIFY_BAD              (NOTIFY_STOP_MASK|0x0002)
> >>                                                 /* Bad/Veto action */
> >> /*
> >>  * Clean way to return from the notifier and stop further calls.
> >>  */
> >> #define NOTIFY_STOP             (NOTIFY_OK|NOTIFY_STOP_MASK)  
> > 
> > I don't think that's a correct interpretation of NOTIFY_STOP_MASK, it
> > intends to say "this notification was for me, I've handled it, no need
> > to continue through the call chain".  It does not imply "don't tell me
> > about this event in the future", entries in the call chain don't have
> > the ability to suppress certain events.
> >   
> 
> OK, thanks for correcting me.
> 
> > Also note that NOTIFY_STOP_MASK is returned by
> > blocking_notifier_call_chain(), so we can pr_warn if a call chain
> > member is potentially preventing other notify-ees from receiving the
> > event.
> > 
> > Should we be revisiting the question of whether notifiers are registered
> > as part of the mdev-core?  In the current proposals
> > vfio_{un}register_notifier() is exported, so it's really just a
> > convenience that the mdev core registers it on behalf of the vendor
> > driver.  Vendor drivers could register on open and unregister on
> > release (vfio could additionally do housekeeping on release).  We
> > already expect vendor drivers to call vfio_{un}pin_pages() directly.
> > Then the vendor driver would provide filter flags directly and we'd
> > make that part of the mdev-vendor driver API a little more flexible.  
> 
> Ok. But that still doesn't solve the problem if more actions need to be
> added to backend iommu module.
> 
> If vendor driver notifier callback is called, vendor driver can return
> NOTIFY_DONE or NOTIFY_OK for the action they are not interested in.

Perhaps calling it a filter is not correct, I was thinking that a
vendor driver would register the notifier with a set of required
events.  The driver would need to handle/ignore additional events
outside of the required mask.  There are certainly some complications
to this model though that I hadn't thought all the way through until
now.  For instance what if we add event XYZ in the future and the
vendor driver adds this to their required mask.  If we run that on an
older kernel where the vfio infrastructure doesn't know about that
event, the vendor driver needs to fail, or at least know that event is
not supported and retry with a set of supported events.

There's another problem with my proposal too, we can't put a single
notifier_block on multiple notifier_block heads, that just doesn't
work.  So we probably need to separate a group notifier from an iommu
notifier, the vendor driver will need to register for each.

Maybe we end up with something like:

int vfio_register_notifier(struct device *dev,
			   vfio_notify_type_t type,
			   unsigned long *required_events,
			   struct notifier_block *nb);

typedef unsigned short vfio_notify_type_t;
enum vfio_notify_type {
	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
};

(stealing this from pci_dev_flags_t, hope it works)

A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
own unique set of events and each would compare their supported events
to the required events by the caller.  Supported events would be
cleared from the callers required_events arg.  If required_events still
has bits set, the notifier_block is not registered, an error is
returned, and the caller can identify the unsupported events by the
remaining bits in the required_events arg.  Can that work?  Thanks,

Alex

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

On 11/17/2016 03:45 AM, Alex Williamson wrote:
> On Thu, 17 Nov 2016 00:42:48 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>> On 11/16/2016 11:18 PM, Alex Williamson wrote:
>>> On Wed, 16 Nov 2016 16:14:24 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>> On 11/16/2016 3:07 PM, Jike Song wrote:  
>>>>> On 11/16/2016 05:14 PM, Kirti Wankhede wrote:    
>>>>>> On 11/16/2016 9:13 AM, Alex Williamson wrote:    
>>>>>>> On Wed, 16 Nov 2016 11:01:37 +0800
>>>>>>> Jike Song <jike.song@intel.com> wrote:
>>>>>>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:    
>>>>>>>>> On Tue, 15 Nov 2016 19:35:46 +0800
>>>>>>>>> Jike Song <jike.song@intel.com> wrote:
>>>>>>>>>       
>>>>>>>>>> The user of vfio_register_notifier might care about not only
>>>>>>>>>> iommu events but also vfio_group events, so also register the
>>>>>>>>>> notifier_block on vfio_group.
>>>>>>>>>>
>>>>>>>>>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>>>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>>>>>>> Signed-off-by: Jike Song <jike.song@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/vfio/vfio.c | 4 ++++
>>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>>>>>>>> index b149ced..2c0eedb 100644
>>>>>>>>>> --- a/drivers/vfio/vfio.c
>>>>>>>>>> +++ b/drivers/vfio/vfio.c
>>>>>>>>>> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb)
>>>>>>>>>>  	else
>>>>>>>>>>  		ret = -ENOTTY;
>>>>>>>>>>  
>>>>>>>>>> +	vfio_group_register_notifier(group, nb);
>>>>>>>>>> +
>>>>>>>>>>  	up_read(&container->group_lock);
>>>>>>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>>>>>>  
>>>>>>>>>> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb)
>>>>>>>>>>  	else
>>>>>>>>>>  		ret = -ENOTTY;
>>>>>>>>>>  
>>>>>>>>>> +	vfio_group_unregister_notifier(group, nb);
>>>>>>>>>> +
>>>>>>>>>>  	up_read(&container->group_lock);
>>>>>>>>>>  	vfio_group_try_dissolve_container(group);
>>>>>>>>>>        
>>>>>>>>>
>>>>>>>>> You haven't addressed the error paths, if the iommu driver returns
>>>>>>>>> error and therefore the {un}register returns error, what is the caller
>>>>>>>>> to expect about the group registration?
>>>>>>>>>       
>>>>>>>>
>>>>>>>> Will change to:
>>>>>>>>
>>>>>>>> 	driver = container->iommu_driver;
>>>>>>>> 	if (likely(driver && driver->ops->register_notifier))
>>>>>>>> 		ret = driver->ops->register_notifier(container->iommu_data, nb);
>>>>>>>> 	else
>>>>>>>> 		ret = -ENOTTY;
>>>>>>>> 	if (ret)
>>>>>>>> 		goto err_register_iommu;
>>>>>>>>
>>>>>>>> 	ret = vfio_group_register_notifier(group, nb);
>>>>>>>> 	if (ret)
>>>>>>>> 		driver->ops->unregister_notifier(container->iommu_data, nb);
>>>>>>>>
>>>>>>>> err_register_iommu:
>>>>>>>> 	up_read(&container->group_lock);
>>>>>>>> 	vfio_group_try_dissolve_container(group);
>>>>>>>>
>>>>>>>> err_register_nb:
>>>>>>>> 	vfio_group_put(group);
>>>>>>>> 	return ret;    
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> What if a vendor driver only cares about the kvm state and doesn't pin
>>>>>>> memory (ie. no DMA) or only cares about iommu and not group notifies?
>>>>>>> If we handled notifier_fn_t 'action' as a bitmask then we could have the
>>>>>>> registrar specify which notification they wanted (a mask/filter), so if
>>>>>>> they only want KVM, we only send that notify, if they only want UNMAPs,
>>>>>>> etc. Then we know whether iommu registration is required.  As a bonus,
>>>>>>> we could add a pr_info() indicating vendors that ask for KVM
>>>>>>> notification so that we can interrogate why they think they need it.
>>>>>>> The downside is that handling action as a bitmask means that we limit
>>>>>>> the number of actions we have available (32 or 64 bits worth).  That
>>>>>>> limit is hopefully far enough off to be ok though.  Thoughts?  Thanks,
>>>>>>>    
>>>>>>
>>>>>> As per my understanding, this bitmask is input to
>>>>>> vfio_register_notifier() and vfio_unregister_notifier(), right?
>>>>>>
>>>>>> These functions are not called from vendor driver directly, these are
>>>>>> called from vfio_mdev. Then should this bitmask be part of parent_ops
>>>>>> that vendor driver can specify?    
>>>>>
>>>>> I think so, there should be a 'notifiler_filter' in parent_ops to indicate
>>>>> that. A draft patch to show Alex's proposal:
>>>>>     
>>>>
>>>> In that case how can we use bitmask to register multiple actions from
>>>> backend iommu module (now we only have VFIO_IOMMU_NOTIFY_DMA_UNMAP
>>>> action)? Even if we pass bitmask to backend iommu modules
>>>> register_notifier(), backend module would have to create separate
>>>> notifier for each action?
>>>>
>>>> Instead of taking bitmask as input from vendor driver, vendor driver's
>>>> callback can send return depending on action if they don't want to get
>>>> future notification:
>>>>
>>>> #define NOTIFY_DONE             0x0000          /* Don't care */
>>>> #define NOTIFY_OK               0x0001          /* Suits me */
>>>> #define NOTIFY_STOP_MASK        0x8000          /* Don't call further */
>>>> #define NOTIFY_BAD              (NOTIFY_STOP_MASK|0x0002)
>>>>                                                 /* Bad/Veto action */
>>>> /*
>>>>  * Clean way to return from the notifier and stop further calls.
>>>>  */
>>>> #define NOTIFY_STOP             (NOTIFY_OK|NOTIFY_STOP_MASK)  
>>>
>>> I don't think that's a correct interpretation of NOTIFY_STOP_MASK, it
>>> intends to say "this notification was for me, I've handled it, no need
>>> to continue through the call chain".  It does not imply "don't tell me
>>> about this event in the future", entries in the call chain don't have
>>> the ability to suppress certain events.
>>>   
>>
>> OK, thanks for correcting me.
>>
>>> Also note that NOTIFY_STOP_MASK is returned by
>>> blocking_notifier_call_chain(), so we can pr_warn if a call chain
>>> member is potentially preventing other notify-ees from receiving the
>>> event.
>>>
>>> Should we be revisiting the question of whether notifiers are registered
>>> as part of the mdev-core?  In the current proposals
>>> vfio_{un}register_notifier() is exported, so it's really just a
>>> convenience that the mdev core registers it on behalf of the vendor
>>> driver.  Vendor drivers could register on open and unregister on
>>> release (vfio could additionally do housekeeping on release).  We
>>> already expect vendor drivers to call vfio_{un}pin_pages() directly.
>>> Then the vendor driver would provide filter flags directly and we'd
>>> make that part of the mdev-vendor driver API a little more flexible.  
>>
>> Ok. But that still doesn't solve the problem if more actions need to be
>> added to backend iommu module.
>>
>> If vendor driver notifier callback is called, vendor driver can return
>> NOTIFY_DONE or NOTIFY_OK for the action they are not interested in.
> 
> Perhaps calling it a filter is not correct, I was thinking that a
> vendor driver would register the notifier with a set of required
> events.  The driver would need to handle/ignore additional events
> outside of the required mask.  There are certainly some complications
> to this model though that I hadn't thought all the way through until
> now.  For instance what if we add event XYZ in the future and the
> vendor driver adds this to their required mask.  If we run that on an
> older kernel where the vfio infrastructure doesn't know about that
> event, the vendor driver needs to fail, or at least know that event is
> not supported and retry with a set of supported events.
> 
> There's another problem with my proposal too, we can't put a single
> notifier_block on multiple notifier_block heads, that just doesn't
> work.  So we probably need to separate a group notifier from an iommu
> notifier, the vendor driver will need to register for each.
> 
> Maybe we end up with something like:
> 
> int vfio_register_notifier(struct device *dev,
> 			   vfio_notify_type_t type,
> 			   unsigned long *required_events,
> 			   struct notifier_block *nb);
> 
> typedef unsigned short vfio_notify_type_t;
> enum vfio_notify_type {
> 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
> 	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
> };
> 
> (stealing this from pci_dev_flags_t, hope it works)
> 
> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
> VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
> own unique set of events and each would compare their supported events
> to the required events by the caller.  Supported events would be
> cleared from the callers required_events arg.  If required_events still
> has bits set, the notifier_block is not registered, an error is
> returned, and the caller can identify the unsupported events by the
> remaining bits in the required_events arg.  Can that work?  Thanks,

Let me summarize the discussion:

- There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
- vfio_{un}register_notifier() has the type specified in parameter
- In vfio_register_notifier, maybe:

	static vfio_iommu_register_notifier() {..}
	static vfio_group_register_notifier() {..}
	int vfio_register_notififer(type..
	{
		if (type == VFIO_IOMMU_NOTIFY)
			return vfio_iommu_register_notifier();
		if (type == VFIO_GROUP_NOTIFY)
			return vfio_group_register_notifier();
	}



What's more, if we still want registration to be done in mdev framework,
we should change parent_ops:

- rename 'notifier' to 'iommu_notifier'
- add "group_notifier"
- Add a group_events and a iommu_events to indicate what events vendor is
  interested in, respectively

or otherwise don't touch it from mdev framework at all?

--
Thanks,
Jike

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Alex Williamson]

On Thu, 17 Nov 2016 13:24:59 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/17/2016 03:45 AM, Alex Williamson wrote:
> > Perhaps calling it a filter is not correct, I was thinking that a
> > vendor driver would register the notifier with a set of required
> > events.  The driver would need to handle/ignore additional events
> > outside of the required mask.  There are certainly some complications
> > to this model though that I hadn't thought all the way through until
> > now.  For instance what if we add event XYZ in the future and the
> > vendor driver adds this to their required mask.  If we run that on an
> > older kernel where the vfio infrastructure doesn't know about that
> > event, the vendor driver needs to fail, or at least know that event is
> > not supported and retry with a set of supported events.
> > 
> > There's another problem with my proposal too, we can't put a single
> > notifier_block on multiple notifier_block heads, that just doesn't
> > work.  So we probably need to separate a group notifier from an iommu
> > notifier, the vendor driver will need to register for each.
> > 
> > Maybe we end up with something like:
> > 
> > int vfio_register_notifier(struct device *dev,
> > 			   vfio_notify_type_t type,
> > 			   unsigned long *required_events,
> > 			   struct notifier_block *nb);
> > 
> > typedef unsigned short vfio_notify_type_t;
> > enum vfio_notify_type {
> > 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
> > 	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
> > };
> > 
> > (stealing this from pci_dev_flags_t, hope it works)
> > 
> > A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
> > VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
> > own unique set of events and each would compare their supported events
> > to the required events by the caller.  Supported events would be
> > cleared from the callers required_events arg.  If required_events still
> > has bits set, the notifier_block is not registered, an error is
> > returned, and the caller can identify the unsupported events by the
> > remaining bits in the required_events arg.  Can that work?  Thanks,  
> 
> Let me summarize the discussion:
> 
> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
> - vfio_{un}register_notifier() has the type specified in parameter
> - In vfio_register_notifier, maybe:
> 
> 	static vfio_iommu_register_notifier() {..}
> 	static vfio_group_register_notifier() {..}
> 	int vfio_register_notififer(type..
> 	{
> 		if (type == VFIO_IOMMU_NOTIFY)
> 			return vfio_iommu_register_notifier();
> 		if (type == VFIO_GROUP_NOTIFY)
> 			return vfio_group_register_notifier();
> 	}
> 
> 
> 
> What's more, if we still want registration to be done in mdev framework,
> we should change parent_ops:
> 
> - rename 'notifier' to 'iommu_notifier'
> - add "group_notifier"
> - Add a group_events and a iommu_events to indicate what events vendor is
>   interested in, respectively
> 
> or otherwise don't touch it from mdev framework at all?

I think we should remove the notifier from the mdev framework and have
the vendor drivers call vfio_{un}register_notifier() directly.  Note:

 - vfio_group_release() should be modified to remove any notifier
   blocks remaining to prevent a stale call chain for the next user.

 - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining
   notifier blocks on the vfio_iommu (ie. vendor drivers will need to
   actively remove iommu notifiers since the vfio_iommu can persist
   beyond the attachment of the mdev group, the WARN_ON will promote a
   proactive approach to surfacing such issues).

I'd like to get Kirti's current series in linux-next ASAP, so please
submit a follow-on series to make these changes.  I hope we can get
that finalized and added on top of Kirti's series before the v4.10
merge window opens. Thanks,

Alex

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

On 11/17/2016 02:03 PM, Alex Williamson wrote:
> On Thu, 17 Nov 2016 13:24:59 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 11/17/2016 03:45 AM, Alex Williamson wrote:
>>> Perhaps calling it a filter is not correct, I was thinking that a
>>> vendor driver would register the notifier with a set of required
>>> events.  The driver would need to handle/ignore additional events
>>> outside of the required mask.  There are certainly some complications
>>> to this model though that I hadn't thought all the way through until
>>> now.  For instance what if we add event XYZ in the future and the
>>> vendor driver adds this to their required mask.  If we run that on an
>>> older kernel where the vfio infrastructure doesn't know about that
>>> event, the vendor driver needs to fail, or at least know that event is
>>> not supported and retry with a set of supported events.
>>>
>>> There's another problem with my proposal too, we can't put a single
>>> notifier_block on multiple notifier_block heads, that just doesn't
>>> work.  So we probably need to separate a group notifier from an iommu
>>> notifier, the vendor driver will need to register for each.
>>>
>>> Maybe we end up with something like:
>>>
>>> int vfio_register_notifier(struct device *dev,
>>> 			   vfio_notify_type_t type,
>>> 			   unsigned long *required_events,
>>> 			   struct notifier_block *nb);
>>>
>>> typedef unsigned short vfio_notify_type_t;
>>> enum vfio_notify_type {
>>> 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
>>> 	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
>>> };
>>>
>>> (stealing this from pci_dev_flags_t, hope it works)
>>>
>>> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
>>> VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
>>> own unique set of events and each would compare their supported events
>>> to the required events by the caller.  Supported events would be
>>> cleared from the callers required_events arg.  If required_events still
>>> has bits set, the notifier_block is not registered, an error is
>>> returned, and the caller can identify the unsupported events by the
>>> remaining bits in the required_events arg.  Can that work?  Thanks,  
>>
>> Let me summarize the discussion:
>>
>> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
>> - vfio_{un}register_notifier() has the type specified in parameter
>> - In vfio_register_notifier, maybe:
>>
>> 	static vfio_iommu_register_notifier() {..}
>> 	static vfio_group_register_notifier() {..}
>> 	int vfio_register_notififer(type..
>> 	{
>> 		if (type == VFIO_IOMMU_NOTIFY)
>> 			return vfio_iommu_register_notifier();
>> 		if (type == VFIO_GROUP_NOTIFY)
>> 			return vfio_group_register_notifier();
>> 	}
>>
>>
>>
>> What's more, if we still want registration to be done in mdev framework,
>> we should change parent_ops:
>>
>> - rename 'notifier' to 'iommu_notifier'
>> - add "group_notifier"
>> - Add a group_events and a iommu_events to indicate what events vendor is
>>   interested in, respectively
>>
>> or otherwise don't touch it from mdev framework at all?
> 
> I think we should remove the notifier from the mdev framework and have
> the vendor drivers call vfio_{un}register_notifier() directly.  Note:
> 
>  - vfio_group_release() should be modified to remove any notifier
>    blocks remaining to prevent a stale call chain for the next user.
> 
>  - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining
>    notifier blocks on the vfio_iommu (ie. vendor drivers will need to
>    actively remove iommu notifiers since the vfio_iommu can persist
>    beyond the attachment of the mdev group, the WARN_ON will promote a
>    proactive approach to surfacing such issues).
> 
> I'd like to get Kirti's current series in linux-next ASAP, so please
> submit a follow-on series to make these changes.  I hope we can get
> that finalized and added on top of Kirti's series before the v4.10
> merge window opens. Thanks,

I'm glad that you'd like to pickup this series, but to make the it clear:
will you merge the intact v14 , or as you said, removing the iommu notifier
from it? Thanks :)

--
Thanks,
Jike

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

On 11/17/2016 02:03 PM, Alex Williamson wrote:
> On Thu, 17 Nov 2016 13:24:59 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 11/17/2016 03:45 AM, Alex Williamson wrote:
>>> Perhaps calling it a filter is not correct, I was thinking that a
>>> vendor driver would register the notifier with a set of required
>>> events.  The driver would need to handle/ignore additional events
>>> outside of the required mask.  There are certainly some complications
>>> to this model though that I hadn't thought all the way through until
>>> now.  For instance what if we add event XYZ in the future and the
>>> vendor driver adds this to their required mask.  If we run that on an
>>> older kernel where the vfio infrastructure doesn't know about that
>>> event, the vendor driver needs to fail, or at least know that event is
>>> not supported and retry with a set of supported events.
>>>
>>> There's another problem with my proposal too, we can't put a single
>>> notifier_block on multiple notifier_block heads, that just doesn't
>>> work.  So we probably need to separate a group notifier from an iommu
>>> notifier, the vendor driver will need to register for each.
>>>
>>> Maybe we end up with something like:
>>>
>>> int vfio_register_notifier(struct device *dev,
>>> 			   vfio_notify_type_t type,
>>> 			   unsigned long *required_events,
>>> 			   struct notifier_block *nb);
>>>
>>> typedef unsigned short vfio_notify_type_t;
>>> enum vfio_notify_type {
>>> 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
>>> 	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
>>> };
>>>
>>> (stealing this from pci_dev_flags_t, hope it works)
>>>
>>> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
>>> VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
>>> own unique set of events and each would compare their supported events
>>> to the required events by the caller.  Supported events would be
>>> cleared from the callers required_events arg.  If required_events still
>>> has bits set, the notifier_block is not registered, an error is
>>> returned, and the caller can identify the unsupported events by the
>>> remaining bits in the required_events arg.  Can that work?  Thanks,  
>>
>> Let me summarize the discussion:
>>
>> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
>> - vfio_{un}register_notifier() has the type specified in parameter
>> - In vfio_register_notifier, maybe:
>>
>> 	static vfio_iommu_register_notifier() {..}
>> 	static vfio_group_register_notifier() {..}
>> 	int vfio_register_notififer(type..
>> 	{
>> 		if (type == VFIO_IOMMU_NOTIFY)
>> 			return vfio_iommu_register_notifier();
>> 		if (type == VFIO_GROUP_NOTIFY)
>> 			return vfio_group_register_notifier();
>> 	}
>>
>>
>>
>> What's more, if we still want registration to be done in mdev framework,
>> we should change parent_ops:
>>
>> - rename 'notifier' to 'iommu_notifier'
>> - add "group_notifier"
>> - Add a group_events and a iommu_events to indicate what events vendor is
>>   interested in, respectively
>>
>> or otherwise don't touch it from mdev framework at all?
> 
> I think we should remove the notifier from the mdev framework and have
> the vendor drivers call vfio_{un}register_notifier() directly.  Note:
> 
>  - vfio_group_release() should be modified to remove any notifier
>    blocks remaining to prevent a stale call chain for the next user.

vfio_group_release calls vfio_group_unlock_and_free, which in turn calls 
kfree(group), so I guess a WARN_ON(group->notifier.head) before kfree
is enough?

>  - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining
>    notifier blocks on the vfio_iommu (ie. vendor drivers will need to
>    actively remove iommu notifiers since the vfio_iommu can persist
>    beyond the attachment of the mdev group, the WARN_ON will promote a
>    proactive approach to surfacing such issues).

I guess Kirti will prefer to pick up this? if not I also can do it :-)

> I'd like to get Kirti's current series in linux-next ASAP, so please
> submit a follow-on series to make these changes.  I hope we can get
> that finalized and added on top of Kirti's series before the v4.10
> merge window opens. Thanks,

Yes, I'll send out the follow-on series ASAP, since we also have KVMGT
depending on it to get notified by vfio...


--
Thanks,
Jike

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Alex Williamson]

On Thu, 17 Nov 2016 20:31:07 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/17/2016 02:03 PM, Alex Williamson wrote:
> > On Thu, 17 Nov 2016 13:24:59 +0800
> > Jike Song <jike.song@intel.com> wrote:
> >   
> >> On 11/17/2016 03:45 AM, Alex Williamson wrote:  
> >>> Perhaps calling it a filter is not correct, I was thinking that a
> >>> vendor driver would register the notifier with a set of required
> >>> events.  The driver would need to handle/ignore additional events
> >>> outside of the required mask.  There are certainly some complications
> >>> to this model though that I hadn't thought all the way through until
> >>> now.  For instance what if we add event XYZ in the future and the
> >>> vendor driver adds this to their required mask.  If we run that on an
> >>> older kernel where the vfio infrastructure doesn't know about that
> >>> event, the vendor driver needs to fail, or at least know that event is
> >>> not supported and retry with a set of supported events.
> >>>
> >>> There's another problem with my proposal too, we can't put a single
> >>> notifier_block on multiple notifier_block heads, that just doesn't
> >>> work.  So we probably need to separate a group notifier from an iommu
> >>> notifier, the vendor driver will need to register for each.
> >>>
> >>> Maybe we end up with something like:
> >>>
> >>> int vfio_register_notifier(struct device *dev,
> >>> 			   vfio_notify_type_t type,
> >>> 			   unsigned long *required_events,
> >>> 			   struct notifier_block *nb);
> >>>
> >>> typedef unsigned short vfio_notify_type_t;
> >>> enum vfio_notify_type {
> >>> 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
> >>> 	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
> >>> };
> >>>
> >>> (stealing this from pci_dev_flags_t, hope it works)
> >>>
> >>> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
> >>> VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
> >>> own unique set of events and each would compare their supported events
> >>> to the required events by the caller.  Supported events would be
> >>> cleared from the callers required_events arg.  If required_events still
> >>> has bits set, the notifier_block is not registered, an error is
> >>> returned, and the caller can identify the unsupported events by the
> >>> remaining bits in the required_events arg.  Can that work?  Thanks,    
> >>
> >> Let me summarize the discussion:
> >>
> >> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
> >> - vfio_{un}register_notifier() has the type specified in parameter
> >> - In vfio_register_notifier, maybe:
> >>
> >> 	static vfio_iommu_register_notifier() {..}
> >> 	static vfio_group_register_notifier() {..}
> >> 	int vfio_register_notififer(type..
> >> 	{
> >> 		if (type == VFIO_IOMMU_NOTIFY)
> >> 			return vfio_iommu_register_notifier();
> >> 		if (type == VFIO_GROUP_NOTIFY)
> >> 			return vfio_group_register_notifier();
> >> 	}
> >>
> >>
> >>
> >> What's more, if we still want registration to be done in mdev framework,
> >> we should change parent_ops:
> >>
> >> - rename 'notifier' to 'iommu_notifier'
> >> - add "group_notifier"
> >> - Add a group_events and a iommu_events to indicate what events vendor is
> >>   interested in, respectively
> >>
> >> or otherwise don't touch it from mdev framework at all?  
> > 
> > I think we should remove the notifier from the mdev framework and have
> > the vendor drivers call vfio_{un}register_notifier() directly.  Note:
> > 
> >  - vfio_group_release() should be modified to remove any notifier
> >    blocks remaining to prevent a stale call chain for the next user.  
> 
> vfio_group_release calls vfio_group_unlock_and_free, which in turn calls 
> kfree(group), so I guess a WARN_ON(group->notifier.head) before kfree
> is enough?

Sorry, vfio_group_fops_release() is the code where I was thinking we
should unregister any notifiers.  The group will still exist after
that.  I was thinking we do not need to WARN_ON if the vendor driver
doesn't de-populate the notifier list on the group because the group is
tied to the device.  On the other hand if the vendor driver registers
on device open, a device could be opened and closed multiple times
within the same open instance of the group, so we could end up with
duplicate call chain entries if we take that approach.  What do you
think, should we require the vendor driver to unregister the group
notifier on device release and therefore WARN_ON if any remain in
vfio_group_fops_release()?  This is at least consistent with what we
must require for the iommu notifier, so I tend to lean that way.

> 
> >  - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining
> >    notifier blocks on the vfio_iommu (ie. vendor drivers will need to
> >    actively remove iommu notifiers since the vfio_iommu can persist
> >    beyond the attachment of the mdev group, the WARN_ON will promote a
> >    proactive approach to surfacing such issues).  
> 
> I guess Kirti will prefer to pick up this? if not I also can do it :-)
> 
> > I'd like to get Kirti's current series in linux-next ASAP, so please
> > submit a follow-on series to make these changes.  I hope we can get
> > that finalized and added on top of Kirti's series before the v4.10
> > merge window opens. Thanks,  
> 
> Yes, I'll send out the follow-on series ASAP, since we also have KVMGT
> depending on it to get notified by vfio...

Thanks,
Alex

Re: [v4 2/3] vfio_register_notifier: also register on the group notifier

[Jike Song]

On 11/18/2016 12:22 AM, Alex Williamson wrote:
> On Thu, 17 Nov 2016 20:31:07 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 11/17/2016 02:03 PM, Alex Williamson wrote:
>>> On Thu, 17 Nov 2016 13:24:59 +0800
>>> Jike Song <jike.song@intel.com> wrote:
>>>   
>>>> On 11/17/2016 03:45 AM, Alex Williamson wrote:  
>>>>> Perhaps calling it a filter is not correct, I was thinking that a
>>>>> vendor driver would register the notifier with a set of required
>>>>> events.  The driver would need to handle/ignore additional events
>>>>> outside of the required mask.  There are certainly some complications
>>>>> to this model though that I hadn't thought all the way through until
>>>>> now.  For instance what if we add event XYZ in the future and the
>>>>> vendor driver adds this to their required mask.  If we run that on an
>>>>> older kernel where the vfio infrastructure doesn't know about that
>>>>> event, the vendor driver needs to fail, or at least know that event is
>>>>> not supported and retry with a set of supported events.
>>>>>
>>>>> There's another problem with my proposal too, we can't put a single
>>>>> notifier_block on multiple notifier_block heads, that just doesn't
>>>>> work.  So we probably need to separate a group notifier from an iommu
>>>>> notifier, the vendor driver will need to register for each.
>>>>>
>>>>> Maybe we end up with something like:
>>>>>
>>>>> int vfio_register_notifier(struct device *dev,
>>>>> 			   vfio_notify_type_t type,
>>>>> 			   unsigned long *required_events,
>>>>> 			   struct notifier_block *nb);
>>>>>
>>>>> typedef unsigned short vfio_notify_type_t;
>>>>> enum vfio_notify_type {
>>>>> 	VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0,
>>>>> 	VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1,
>>>>> };
>>>>>
>>>>> (stealing this from pci_dev_flags_t, hope it works)
>>>>>
>>>>> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a
>>>>> VFIO_IOMMU_NOTIFY would add it to vfio_iommu.  Each would have their
>>>>> own unique set of events and each would compare their supported events
>>>>> to the required events by the caller.  Supported events would be
>>>>> cleared from the callers required_events arg.  If required_events still
>>>>> has bits set, the notifier_block is not registered, an error is
>>>>> returned, and the caller can identify the unsupported events by the
>>>>> remaining bits in the required_events arg.  Can that work?  Thanks,    
>>>>
>>>> Let me summarize the discussion:
>>>>
>>>> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group;
>>>> - vfio_{un}register_notifier() has the type specified in parameter
>>>> - In vfio_register_notifier, maybe:
>>>>
>>>> 	static vfio_iommu_register_notifier() {..}
>>>> 	static vfio_group_register_notifier() {..}
>>>> 	int vfio_register_notififer(type..
>>>> 	{
>>>> 		if (type == VFIO_IOMMU_NOTIFY)
>>>> 			return vfio_iommu_register_notifier();
>>>> 		if (type == VFIO_GROUP_NOTIFY)
>>>> 			return vfio_group_register_notifier();
>>>> 	}
>>>>
>>>>
>>>>
>>>> What's more, if we still want registration to be done in mdev framework,
>>>> we should change parent_ops:
>>>>
>>>> - rename 'notifier' to 'iommu_notifier'
>>>> - add "group_notifier"
>>>> - Add a group_events and a iommu_events to indicate what events vendor is
>>>>   interested in, respectively
>>>>
>>>> or otherwise don't touch it from mdev framework at all?  
>>>
>>> I think we should remove the notifier from the mdev framework and have
>>> the vendor drivers call vfio_{un}register_notifier() directly.  Note:
>>>
>>>  - vfio_group_release() should be modified to remove any notifier
>>>    blocks remaining to prevent a stale call chain for the next user.  
>>
>> vfio_group_release calls vfio_group_unlock_and_free, which in turn calls 
>> kfree(group), so I guess a WARN_ON(group->notifier.head) before kfree
>> is enough?
> 
> Sorry, vfio_group_fops_release() is the code where I was thinking we
> should unregister any notifiers.  The group will still exist after
> that.  I was thinking we do not need to WARN_ON if the vendor driver
> doesn't de-populate the notifier list on the group because the group is
> tied to the device.  On the other hand if the vendor driver registers
> on device open, a device could be opened and closed multiple times
> within the same open instance of the group, so we could end up with
> duplicate call chain entries if we take that approach.  What do you
> think, should we require the vendor driver to unregister the group
> notifier on device release and therefore WARN_ON if any remain in
> vfio_group_fops_release()?  This is at least consistent with what we
> must require for the iommu notifier, so I tend to lean that way.

I agree, a WARN_ON() is needed in vfio_group_fops_release, in case of
any possible usage violation from vendor drivers. Will add that in next
version :)


--
Thanks,
Jike

>>>  - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining
>>>    notifier blocks on the vfio_iommu (ie. vendor drivers will need to
>>>    actively remove iommu notifiers since the vfio_iommu can persist
>>>    beyond the attachment of the mdev group, the WARN_ON will promote a
>>>    proactive approach to surfacing such issues).  
>>
>> I guess Kirti will prefer to pick up this? if not I also can do it :-)
>>
>>> I'd like to get Kirti's current series in linux-next ASAP, so please
>>> submit a follow-on series to make these changes.  I hope we can get
>>> that finalized and added on top of Kirti's series before the v4.10
>>> merge window opens. Thanks,  
>>
>> Yes, I'll send out the follow-on series ASAP, since we also have KVMGT
>> depending on it to get notified by vfio...
> 
> Thanks,
> Alex