From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [v4 2/3] vfio_register_notifier: also register on the group notifier Date: Tue, 15 Nov 2016 20:43:29 -0700 Message-ID: <20161115204329.6245cacb@t450s.home> References: <1479209747-5564-1-git-send-email-jike.song@intel.com> <1479209747-5564-3-git-send-email-jike.song@intel.com> <20161115161125.7d75fb49@t450s.home> <582BCC11.80202@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]:52298 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933423AbcKPDnb (ORCPT ); Tue, 15 Nov 2016 22:43:31 -0500 In-Reply-To: <582BCC11.80202@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 16 Nov 2016 11:01:37 +0800 Jike Song wrote: > On 11/16/2016 07:11 AM, Alex Williamson wrote: > > On Tue, 15 Nov 2016 19:35:46 +0800 > > Jike Song wrote: > > > >> The user of vfio_register_notifier might care about not only > >> iommu events but also vfio_group events, so also register the > >> notifier_block on vfio_group. > >> > >> Cc: Xiao Guangrong > >> Cc: Paolo Bonzini > >> Cc: Alex Williamson > >> Signed-off-by: Jike Song > >> --- > >> drivers/vfio/vfio.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >> index b149ced..2c0eedb 100644 > >> --- a/drivers/vfio/vfio.c > >> +++ b/drivers/vfio/vfio.c > >> @@ -2065,6 +2065,8 @@ int vfio_register_notifier(struct device *dev, struct notifier_block *nb) > >> else > >> ret = -ENOTTY; > >> > >> + vfio_group_register_notifier(group, nb); > >> + > >> up_read(&container->group_lock); > >> vfio_group_try_dissolve_container(group); > >> > >> @@ -2102,6 +2104,8 @@ int vfio_unregister_notifier(struct device *dev, struct notifier_block *nb) > >> else > >> ret = -ENOTTY; > >> > >> + vfio_group_unregister_notifier(group, nb); > >> + > >> up_read(&container->group_lock); > >> vfio_group_try_dissolve_container(group); > >> > > > > You haven't addressed the error paths, if the iommu driver returns > > error and therefore the {un}register returns error, what is the caller > > to expect about the group registration? > > > > Will change to: > > driver = container->iommu_driver; > if (likely(driver && driver->ops->register_notifier)) > ret = driver->ops->register_notifier(container->iommu_data, nb); > else > ret = -ENOTTY; > if (ret) > goto err_register_iommu; > > ret = vfio_group_register_notifier(group, nb); > if (ret) > driver->ops->unregister_notifier(container->iommu_data, nb); > > err_register_iommu: > up_read(&container->group_lock); > vfio_group_try_dissolve_container(group); > > err_register_nb: > vfio_group_put(group); > return ret; What if a vendor driver only cares about the kvm state and doesn't pin memory (ie. no DMA) or only cares about iommu and not group notifies? If we handled notifier_fn_t 'action' as a bitmask then we could have the registrar specify which notification they wanted (a mask/filter), so if they only want KVM, we only send that notify, if they only want UNMAPs, etc. Then we know whether iommu registration is required. As a bonus, we could add a pr_info() indicating vendors that ask for KVM notification so that we can interrogate why they think they need it. The downside is that handling action as a bitmask means that we limit the number of actions we have available (32 or 64 bits worth). That limit is hopefully far enough off to be ok though. Thoughts? Thanks, Alex