Re: [v7 2/3] vfio: support notifier chain in vfio_group

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

On 11/22/2016 10:50 PM, Alex Williamson wrote:
> On Tue, 22 Nov 2016 20:09:32 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 11/22/2016 7:32 PM, Alex Williamson wrote:
>>> On Tue, 22 Nov 2016 19:05:16 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> 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
> 
> No, the vendor driver is not necessarily compiled against the given
> kernel.  We can also have different backends in vfio, what if we have
> IOMMU_SET_FOO supported by type1 and IOMMU_SET_BAR supported by spapr,
> the vendor driver might only support one of these and can't tell at
> compile time which is active.  Thanks,

Yes, if we rely on macros only, driver not built against current kernel
won't work correctly. But have IOMMU backends considered, seems that we
don't differentiating them, having only one VFIO_IOMMU_NOTIFY?

--
Thanks,
Jike