From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jike Song Subject: Re: [v6 2/3] vfio: support notifier chain in vfio_group Date: Sat, 19 Nov 2016 12:14:29 +0800 Message-ID: <582FD1A5.1030207@intel.com> References: <1479466909-31765-1-git-send-email-jike.song@intel.com> <1479466909-31765-3-git-send-email-jike.song@intel.com> <20161118105557.70851cd5@t450s.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, guangrong.xiao@linux.intel.com, kwankhede@nvidia.com, cjia@nvidia.com, kevin.tian@intel.com, kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mga07.intel.com ([134.134.136.100]:9875 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752174AbcKSEOc (ORCPT ); Fri, 18 Nov 2016 23:14:32 -0500 In-Reply-To: <20161118105557.70851cd5@t450s.home> Sender: kvm-owner@vger.kernel.org List-ID: On 11/19/2016 01:55 AM, Alex Williamson wrote: > On Fri, 18 Nov 2016 19:01:48 +0800 > 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 >> Cc: Kirti Wankhede >> Cc: Paolo Bonzini >> Cc: Xiao Guangrong >> Signed-off-by: Jike Song >> --- >> drivers/vfio/vfio.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/vfio.h | 6 +++++ >> 2 files changed, 75 insertions(+) >> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >> index ec62bec..e2bb197 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; >> >> @@ -1015,6 +1018,63 @@ static long vfio_ioctl_check_extension(struct vfio_container *container, >> 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) >> +{ >> + 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; >> + >> + if (!atomic_inc_not_zero(&group->opened)) >> + return -EINVAL; > > vfio_group.opened is only used to make sure we don't allow multiple > opens of the group, incrementing it doesn't currently assure the group > remains opened. What happens if the user process releases the group in > the midst of this? Thanks for pointing out this. It seems okay to think the group is open by checking group->opened, but after that I failed to find any existing method to prevent a concurrent vfio_group_fops_release, could you enlighten me a bit? >> + >> + 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); >> + >> + atomic_dec(&group->opened); >> + >> + return ret; >> +} >> + >> +static int vfio_unregister_group_notifier(struct vfio_group *group, >> + struct notifier_block *nb) >> +{ >> + int ret; >> + >> + if (!atomic_inc_not_zero(&group->opened)) >> + return -EINVAL; >> + >> + ret = blocking_notifier_chain_unregister(&group->notifier, nb); >> + >> + atomic_dec(&group->opened); >> + return ret; >> +} >> + >> /* hold write lock on container->group_lock */ >> static int __vfio_container_attach_groups(struct vfio_container *container, >> struct vfio_iommu_driver *driver, >> @@ -1581,6 +1641,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); > > Have you tested whether the ordering is correct that the vendor driver > sees the device release first and can therefore unregister notifiers > before the group is released when the user process is killed? > Yes, mdev close happens before vfio_group fop close. -- Thanks, Jike >> + >> vfio_group_try_dissolve_container(group); >> >> atomic_dec(&group->opened); >> @@ -2088,6 +2151,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; >> } >> @@ -2114,6 +2180,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