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@xxxxxxxxx> wrote:
>>
>>> On 11/16/2016 07:11 AM, Alex Williamson wrote:
>>>> On Tue, 15 Nov 2016 19:35:46 +0800
>>>> Jike Song <jike.song@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>>>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>>>> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
>>>>> Signed-off-by: Jike Song <jike.song@xxxxxxxxx>
>>>>> ---
>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
