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, Alex -- 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
