On 11/23/2016 01:56 PM, Alex Williamson wrote: > On Wed, 23 Nov 2016 10:22:37 +0530 > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote: > >> On 11/23/2016 8:50 AM, Jike Song wrote: >>> On 11/22/2016 10:50 PM, Alex Williamson wrote: >>>> On Tue, 22 Nov 2016 20:09:32 +0530 >>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote: >>>> >>>>> 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 >>>> >>>> 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? >>> >> >> Most of the distro's kernel have CONFIG_MODVERSIONS enabled and its >> mostly safe to build driver against the kernel to which its going to load. >> For backend IOMMU module, we have same callback functions or >> VFIO_IOMMU_NOTIFY, but as Alex mentioned different IOMMU backend modules >> might have different events/actions. Then the event check shouldn't be >> in vfio module, it should be in backend module. > > Yes, patch 1/3 is wrong, the required events mask should be passed to > the iommu backend and handled there, not in vfio.c. Thanks, Will change that. BTW, do you think the iommu types should be different? that is to say, having VFIO_IOMMU_NOTIFY_TYPE1 instead of VFIO_IOMMU_NOTIFY? -- 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
