From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kirti Wankhede Subject: Re: [v4 2/3] vfio_register_notifier: also register on the group notifier Date: Wed, 16 Nov 2016 14:44:10 +0530 Message-ID: <3f95b79b-c834-65c8-4cc0-93c688e6efc4@nvidia.com> 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> <20161115204329.6245cacb@t450s.home> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: , , , , To: Alex Williamson , Jike Song Return-path: Received: from hqemgate15.nvidia.com ([216.228.121.64]:3037 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752258AbcKPJPr (ORCPT ); Wed, 16 Nov 2016 04:15:47 -0500 In-Reply-To: <20161115204329.6245cacb@t450s.home> Sender: kvm-owner@vger.kernel.org List-ID: On 11/16/2016 9:13 AM, Alex Williamson wrote: > 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, > As per my understanding, this bitmask is input to vfio_register_notifier() and vfio_unregister_notifier(), right? These functions are not called from vendor driver directly, these are called from vfio_mdev. Then should this bitmask be part of parent_ops that vendor driver can specify? Thanks, Kirti