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