| Message ID | 1479794973-11910-3-git-send-email-jike.song@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/22/2016 11:39 AM, Jike Song wrote: > Beyond vfio_iommu events, users might also be interested in > vfio_group events. For example, if a vfio_group is used along > with Qemu/KVM, whenever kvm pointer is set to/cleared from the > vfio_group, users could be notified. > > Currently only VFIO_GROUP_NOTIFY_SET_KVM supported. > > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Kirti Wankhede <kwankhede@nvidia.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com> > Signed-off-by: Jike Song <jike.song@intel.com> > --- > drivers/vfio/vfio.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/vfio.h | 6 ++++ > 2 files changed, 88 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 493bbad..3ab5edb 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -86,6 +86,8 @@ struct vfio_group { > struct mutex unbound_lock; > atomic_t opened; > bool noiommu; > + struct kvm *kvm; > + struct blocking_notifier_head notifier; > }; > > struct vfio_device { > @@ -339,6 +341,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) > #ifdef CONFIG_VFIO_NOIOMMU > group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu); > #endif > + BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier); > > group->nb.notifier_call = vfio_iommu_group_notifier; > > @@ -1581,6 +1584,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) > > filep->private_data = NULL; > > + /* Any user didn't unregister? */ > + WARN_ON(group->notifier.head); > + > vfio_group_try_dissolve_container(group); > > atomic_dec(&group->opened); > @@ -2069,6 +2075,76 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group, > return ret; > } > > +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) > +{ > + group->kvm = kvm; > + blocking_notifier_call_chain(&group->notifier, > + VFIO_GROUP_NOTIFY_SET_KVM, kvm); > +} > +EXPORT_SYMBOL_GPL(vfio_group_set_kvm); > + > +static int vfio_register_group_notifier(struct vfio_group *group, > + unsigned long *events, > + struct notifier_block *nb) > +{ > + struct vfio_container *container; > + int ret; > + bool set_kvm = false; > + > + if (*events & VFIO_GROUP_NOTIFY_SET_KVM) > + set_kvm = true; > + > + /* clear known events */ > + *events &= ~VFIO_GROUP_NOTIFY_SET_KVM; > + > + /* refuse to continue if still events remaining */ > + if (*events) > + return -EINVAL; > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + return -EINVAL; > + > + container = group->container; > + down_read(&container->group_lock); > + > + ret = blocking_notifier_chain_register(&group->notifier, nb); > + > + /* > + * The attaching of kvm and vfio_group might already happen, so > + * here we replay once upon registration. > + */ > + if (!ret && set_kvm && group->kvm) > + blocking_notifier_call_chain(&group->notifier, > + VFIO_GROUP_NOTIFY_SET_KVM, group->kvm); > + > + up_read(&container->group_lock); > + vfio_group_try_dissolve_container(group); > + > + return ret; > +} > + > +static int vfio_unregister_group_notifier(struct vfio_group *group, > + struct notifier_block *nb) > +{ > + struct vfio_container *container; > + int ret; > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + return -EINVAL; > + > + container = group->container; > + down_read(&container->group_lock); > + > + ret = blocking_notifier_chain_unregister(&group->notifier, nb); > + > + up_read(&container->group_lock); > + vfio_group_try_dissolve_container(group); > + > + return ret; > +} > + > int vfio_register_notifier(struct device *dev, vfio_notify_type_t type, > unsigned long *events, > struct notifier_block *nb) > @@ -2087,6 +2163,9 @@ int vfio_register_notifier(struct device *dev, vfio_notify_type_t type, > case VFIO_IOMMU_NOTIFY: > ret = vfio_register_iommu_notifier(group, events, nb); > break; > + case VFIO_GROUP_NOTIFY: > + ret = vfio_register_group_notifier(group, events, nb); > + break; > default: > ret = -EINVAL; > } > @@ -2113,6 +2192,9 @@ int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type, > case VFIO_IOMMU_NOTIFY: > ret = vfio_unregister_iommu_notifier(group, nb); > break; > + case VFIO_GROUP_NOTIFY: > + ret = vfio_unregister_group_notifier(group, nb); > + break; > default: > ret = -EINVAL; > } > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 6f3ff31..5d46e3c 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -119,11 +119,17 @@ extern int vfio_unregister_notifier(struct device *dev, > /* each type has independent events */ > enum vfio_notify_type { > VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0, > + VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1, > }; > > /* 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); Thanks, Kirti -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 22 Nov 2016 19:05:16 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 11/22/2016 11:39 AM, Jike Song wrote: > > Beyond vfio_iommu events, users might also be interested in > > vfio_group events. For example, if a vfio_group is used along > > with Qemu/KVM, whenever kvm pointer is set to/cleared from the > > vfio_group, users could be notified. > > > > Currently only VFIO_GROUP_NOTIFY_SET_KVM supported. > > > > Cc: Alex Williamson <alex.williamson@redhat.com> > > Cc: Kirti Wankhede <kwankhede@nvidia.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com> > > Signed-off-by: Jike Song <jike.song@intel.com> > > --- > > drivers/vfio/vfio.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/vfio.h | 6 ++++ > > 2 files changed, 88 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index 493bbad..3ab5edb 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -86,6 +86,8 @@ struct vfio_group { > > struct mutex unbound_lock; > > atomic_t opened; > > bool noiommu; > > + struct kvm *kvm; > > + struct blocking_notifier_head notifier; > > }; > > > > struct vfio_device { > > @@ -339,6 +341,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) > > #ifdef CONFIG_VFIO_NOIOMMU > > group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu); > > #endif > > + BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier); > > > > group->nb.notifier_call = vfio_iommu_group_notifier; > > > > @@ -1581,6 +1584,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) > > > > filep->private_data = NULL; > > > > + /* Any user didn't unregister? */ > > + WARN_ON(group->notifier.head); > > + > > vfio_group_try_dissolve_container(group); > > > > atomic_dec(&group->opened); > > @@ -2069,6 +2075,76 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group, > > return ret; > > } > > > > +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) > > +{ > > + group->kvm = kvm; > > + blocking_notifier_call_chain(&group->notifier, > > + VFIO_GROUP_NOTIFY_SET_KVM, kvm); > > +} > > +EXPORT_SYMBOL_GPL(vfio_group_set_kvm); > > + > > +static int vfio_register_group_notifier(struct vfio_group *group, > > + unsigned long *events, > > + struct notifier_block *nb) > > +{ > > + struct vfio_container *container; > > + int ret; > > + bool set_kvm = false; > > + > > + if (*events & VFIO_GROUP_NOTIFY_SET_KVM) > > + set_kvm = true; > > + > > + /* clear known events */ > > + *events &= ~VFIO_GROUP_NOTIFY_SET_KVM; > > + > > + /* refuse to continue if still events remaining */ > > + if (*events) > > + return -EINVAL; > > + > > + ret = vfio_group_add_container_user(group); > > + if (ret) > > + return -EINVAL; > > + > > + container = group->container; > > + down_read(&container->group_lock); > > + > > + ret = blocking_notifier_chain_register(&group->notifier, nb); > > + > > + /* > > + * The attaching of kvm and vfio_group might already happen, so > > + * here we replay once upon registration. > > + */ > > + if (!ret && set_kvm && group->kvm) > > + blocking_notifier_call_chain(&group->notifier, > > + VFIO_GROUP_NOTIFY_SET_KVM, group->kvm); > > + > > + up_read(&container->group_lock); > > + vfio_group_try_dissolve_container(group); > > + > > + return ret; > > +} > > + > > +static int vfio_unregister_group_notifier(struct vfio_group *group, > > + struct notifier_block *nb) > > +{ > > + struct vfio_container *container; > > + int ret; > > + > > + ret = vfio_group_add_container_user(group); > > + if (ret) > > + return -EINVAL; > > + > > + container = group->container; > > + down_read(&container->group_lock); > > + > > + ret = blocking_notifier_chain_unregister(&group->notifier, nb); > > + > > + up_read(&container->group_lock); > > + vfio_group_try_dissolve_container(group); > > + > > + return ret; > > +} > > + > > int vfio_register_notifier(struct device *dev, vfio_notify_type_t type, > > unsigned long *events, > > struct notifier_block *nb) > > @@ -2087,6 +2163,9 @@ int vfio_register_notifier(struct device *dev, vfio_notify_type_t type, > > case VFIO_IOMMU_NOTIFY: > > ret = vfio_register_iommu_notifier(group, events, nb); > > break; > > + case VFIO_GROUP_NOTIFY: > > + ret = vfio_register_group_notifier(group, events, nb); > > + break; > > default: > > ret = -EINVAL; > > } > > @@ -2113,6 +2192,9 @@ int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type, > > case VFIO_IOMMU_NOTIFY: > > ret = vfio_unregister_iommu_notifier(group, nb); > > break; > > + case VFIO_GROUP_NOTIFY: > > + ret = vfio_unregister_group_notifier(group, nb); > > + break; > > default: > > ret = -EINVAL; > > } > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index 6f3ff31..5d46e3c 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -119,11 +119,17 @@ extern int vfio_unregister_notifier(struct device *dev, > > /* each type has independent events */ > > enum vfio_notify_type { > > VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0, > > + VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1, > > }; > > > > /* 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, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/22/2016 7:32 PM, Alex Williamson wrote: > On Tue, 22 Nov 2016 19:05:16 +0530 > Kirti Wankhede <kwankhede@nvidia.com> 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 Thanks, Kirti -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 22 Nov 2016 20:09:32 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 11/22/2016 7:32 PM, Alex Williamson wrote: > > On Tue, 22 Nov 2016 19:05:16 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/22/2016 10:50 PM, Alex Williamson wrote: > On Tue, 22 Nov 2016 20:09:32 +0530 > Kirti Wankhede <kwankhede@nvidia.com> wrote: > >> On 11/22/2016 7:32 PM, Alex Williamson wrote: >>> On Tue, 22 Nov 2016 19:05:16 +0530 >>> Kirti Wankhede <kwankhede@nvidia.com> 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? -- Thanks, Jike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> wrote: >> >>> On 11/22/2016 7:32 PM, Alex Williamson wrote: >>>> On Tue, 22 Nov 2016 19:05:16 +0530 >>>> Kirti Wankhede <kwankhede@nvidia.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 23 Nov 2016 10:22:37 +0530 Kirti Wankhede <kwankhede@nvidia.com> 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@nvidia.com> wrote: > >> > >>> On 11/22/2016 7:32 PM, Alex Williamson wrote: > >>>> On Tue, 22 Nov 2016 19:05:16 +0530 > >>>> Kirti Wankhede <kwankhede@nvidia.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/23/2016 01:56 PM, Alex Williamson wrote: > On Wed, 23 Nov 2016 10:22:37 +0530 > Kirti Wankhede <kwankhede@nvidia.com> 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@nvidia.com> wrote: >>>> >>>>> On 11/22/2016 7:32 PM, Alex Williamson wrote: >>>>>> On Tue, 22 Nov 2016 19:05:16 +0530 >>>>>> Kirti Wankhede <kwankhede@nvidia.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@nvidia.com> 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@nvidia.com> wrote: > >>>> > >>>>> On 11/22/2016 7:32 PM, Alex Williamson wrote: > >>>>>> On Tue, 22 Nov 2016 19:05:16 +0530 > >>>>>> Kirti Wankhede <kwankhede@nvidia.com> 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
On 11/23/2016 02:33 PM, Tian, Kevin wrote: >> 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@nvidia.com> wrote: >>>> >>>> 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... I also lean towards keeping VFIO_IOMMU_NOTIFY, will update [1/3] without changing that part. -- Thanks, Jike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 23 Nov 2016 15:53:23 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/23/2016 02:33 PM, Tian, Kevin wrote: > >> 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@nvidia.com> wrote: > >>>> > >>>> 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... > > I also lean towards keeping VFIO_IOMMU_NOTIFY, will update [1/3] without > changing that part. Agreed, the vendor driver has no visibility to the iommu model selected by the user. The type identifies where the notifier is attached. The event mask allows the vendor driver to set required notifications. If an iommu backend does not support a compatible notification, it doesn't matter whether it's type1 vs spapr vs any future iommu backend type. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 493bbad..3ab5edb 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -86,6 +86,8 @@ struct vfio_group { struct mutex unbound_lock; atomic_t opened; bool noiommu; + struct kvm *kvm; + struct blocking_notifier_head notifier; }; struct vfio_device { @@ -339,6 +341,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) #ifdef CONFIG_VFIO_NOIOMMU group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu); #endif + BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier); group->nb.notifier_call = vfio_iommu_group_notifier; @@ -1581,6 +1584,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) filep->private_data = NULL; + /* Any user didn't unregister? */ + WARN_ON(group->notifier.head); + vfio_group_try_dissolve_container(group); atomic_dec(&group->opened); @@ -2069,6 +2075,76 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group, return ret; } +void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) +{ + group->kvm = kvm; + blocking_notifier_call_chain(&group->notifier, + VFIO_GROUP_NOTIFY_SET_KVM, kvm); +} +EXPORT_SYMBOL_GPL(vfio_group_set_kvm); + +static int vfio_register_group_notifier(struct vfio_group *group, + unsigned long *events, + struct notifier_block *nb) +{ + struct vfio_container *container; + int ret; + bool set_kvm = false; + + if (*events & VFIO_GROUP_NOTIFY_SET_KVM) + set_kvm = true; + + /* clear known events */ + *events &= ~VFIO_GROUP_NOTIFY_SET_KVM; + + /* refuse to continue if still events remaining */ + if (*events) + return -EINVAL; + + ret = vfio_group_add_container_user(group); + if (ret) + return -EINVAL; + + container = group->container; + down_read(&container->group_lock); + + ret = blocking_notifier_chain_register(&group->notifier, nb); + + /* + * The attaching of kvm and vfio_group might already happen, so + * here we replay once upon registration. + */ + if (!ret && set_kvm && group->kvm) + blocking_notifier_call_chain(&group->notifier, + VFIO_GROUP_NOTIFY_SET_KVM, group->kvm); + + up_read(&container->group_lock); + vfio_group_try_dissolve_container(group); + + return ret; +} + +static int vfio_unregister_group_notifier(struct vfio_group *group, + struct notifier_block *nb) +{ + struct vfio_container *container; + int ret; + + ret = vfio_group_add_container_user(group); + if (ret) + return -EINVAL; + + container = group->container; + down_read(&container->group_lock); + + ret = blocking_notifier_chain_unregister(&group->notifier, nb); + + up_read(&container->group_lock); + vfio_group_try_dissolve_container(group); + + return ret; +} + int vfio_register_notifier(struct device *dev, vfio_notify_type_t type, unsigned long *events, struct notifier_block *nb) @@ -2087,6 +2163,9 @@ int vfio_register_notifier(struct device *dev, vfio_notify_type_t type, case VFIO_IOMMU_NOTIFY: ret = vfio_register_iommu_notifier(group, events, nb); break; + case VFIO_GROUP_NOTIFY: + ret = vfio_register_group_notifier(group, events, nb); + break; default: ret = -EINVAL; } @@ -2113,6 +2192,9 @@ int vfio_unregister_notifier(struct device *dev, vfio_notify_type_t type, case VFIO_IOMMU_NOTIFY: ret = vfio_unregister_iommu_notifier(group, nb); break; + case VFIO_GROUP_NOTIFY: + ret = vfio_unregister_group_notifier(group, nb); + break; default: ret = -EINVAL; } diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 6f3ff31..5d46e3c 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -119,11 +119,17 @@ extern int vfio_unregister_notifier(struct device *dev, /* each type has independent events */ enum vfio_notify_type { VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0, + VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1, }; /* 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); /* * Sub-module helpers
Beyond vfio_iommu events, users might also be interested in vfio_group events. For example, if a vfio_group is used along with Qemu/KVM, whenever kvm pointer is set to/cleared from the vfio_group, users could be notified. Currently only VFIO_GROUP_NOTIFY_SET_KVM supported. Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com> Signed-off-by: Jike Song <jike.song@intel.com> --- drivers/vfio/vfio.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/vfio.h | 6 ++++ 2 files changed, 88 insertions(+)