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

[Jike Song <jike.song@intel.com>]

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