On 11/22/2016 7:32 PM, Alex Williamson wrote:
> On Tue, 22 Nov 2016 19:05:16 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>
>> On 11/22/2016 11:39 AM, Jike Song wrote:
<snip>
>>> /* events for VFIO_IOMMU_NOTIFY */
>>> #define VFIO_IOMMU_NOTIFY_DMA_UNMAP BIT(0)
>>>
>>> +/* events for VFIO_GROUP_NOTIFY */
>>> +#define VFIO_GROUP_NOTIFY_SET_KVM BIT(0)
>>> +
>>> +struct kvm;
>>> +extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm);
>>>
>>
>> If I understand correctly, notifier for backend iommu driver and
>> notifier for group should be registered with different notifier blocks,
>> as per your current implementation:
>>
>> unsigned long iommu_event = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>> struct notifier_block iommu_nb;
>>
>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_event, &iommu_nb);
>>
>>
>> unsigned long group_event = VFIO_GROUP_NOTIFY_SET_KVM;
>> struct notifier_block group_nb;
>>
>> group_nb.notifier_call = vfio_group_notifier_cb;
>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_event, &group_nb);
>>
>> Then what is the use of having 'events' bitmask as input argument?
>>
>> Its clear that caller should set:
>> - VFIO_IOMMU_NOTIFY_DMA_UNMAP when vfio_notify_type_t is VFIO_IOMMU_NOTIFY
>> - VFIO_GROUP_NOTIFY_SET_KVM when vfio_notify_type_t is VFIO_GROUP_NOTIFY
>>
>> Notifier action could be a integer as earlier and indexing of action
>> would be different for both notifiers.
>>
>> Then caller don't have to sent event bitmask, for caller it would look like:
>>
>> struct notifier_block iommu_nb;
>>
>> iommu_nb.notifier_call = vfio_iommu_notifier_cb;
>> vfio_register_notifier(dev, VFIO_IOMMU_NOTIFY, &iommu_nb);
>>
>>
>> struct notifier_block group_nb;
>>
>> group_nb.notifier_call = vfio_group_notifier_cb;
>> vfio_register_notifier(dev, VFIO_GROUP_NOTIFY, &group_nb);
>
> If we did that then we'd effectively need to add a new notifier any
> time we want a new signal because the vendor driver would have no
> ability to query the notifications available. For instance, say we
> added VFIO_IOMMU_NOTIFY_SET_FOO and a vendor driver was dependent on
> that and would either refuse to work or would take a backup path if
> that notification isn't available. How could it operate in your
> proposal? By passing a bitmask of events, the vendor driver can
> specify the set of required events and see which event signaling is
> unavailable. The purpose of using a bitmask and passing the set of
> required bits on registration is to support future expansion. Thanks,
>
If we add VFIO_IOMMU_NOTIFY_SET_FOO in future, we have to define this
action in include/linux/vfio.h
#define VFIO_IOMMU_NOTIFY_DMA_UNMAP (1)
+#define VFIO_IOMMU_NOTIFY_SET_FOO (2)
Vendor driver anyways need to be compiled against the kernel to which
its going to run. So vendor driver could use conditional directive:
#if defined(VFIO_IOMMU_NOTIFY_SET_FOO)
/* foo signal available*/
#else
/* foo signal not available*/
#endif
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
