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?
Thanks,
Kirti
--
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
