> From: Song, Jike
> Sent: Wednesday, November 23, 2016 2:30 PM
>
> 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?
>
Should vendor driver care about underlying IOMMU difference? Assume
those notification events should be IOMMU vendor agnostic...
Thanks
Kevin
��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
