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