From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [v6 2/3] vfio: support notifier chain in vfio_group Date: Mon, 21 Nov 2016 09:56:37 -0700 Message-ID: <20161121095637.44c6f66a@t450s.home> 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> <582FD1A5.1030207@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Jike Song Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60310 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753309AbcKUQ4i (ORCPT ); Mon, 21 Nov 2016 11:56:38 -0500 In-Reply-To: <582FD1A5.1030207@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sat, 19 Nov 2016 12:14:29 +0800 Jike Song wrote: > 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? I don't think we have such a thing. I briefly investigated whether we should add a group mutex rather than the atomic, but at this point I'm just leaning towards using the same conditions as attaching the iommu notifier, ie. call vfio_group_add_container_user(). This is also what we do for vfio_group_get_external_user() so I think it makes sense that any sort of group reference or notifier registration have the same requirements. Thanks, Alex