From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jike Song Subject: Re: [v4 2/3] vfio_register_notifier: also register on the group notifier Date: Thu, 17 Nov 2016 20:31:07 +0800 Message-ID: <582DA30B.6020301@intel.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> <3f95b79b-c834-65c8-4cc0-93c688e6efc4@nvidia.com> <582C28D8.3050905@intel.com> <3279c050-73ec-b594-b9d0-e6d05a5823e5@nvidia.com> <20161116104856.32f29d9e@t450s.home> <92064671-52db-bb13-cea2-29731834b21a@nvidia.com> <20161116124514.4f6c5b93@t450s.home> <582D3F2B.5070702@intel.com> <20161116230316.11f2cde8@t450s.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Kirti Wankhede , pbonzini@redhat.com, guangrong.xiao@linux.intel.com, cjia@nvidia.com, kevin.tian@intel.com, kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mga11.intel.com ([192.55.52.93]:28140 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752752AbcKQQ5O (ORCPT ); Thu, 17 Nov 2016 11:57:14 -0500 In-Reply-To: <20161116230316.11f2cde8@t450s.home> Sender: kvm-owner@vger.kernel.org List-ID: On 11/17/2016 02:03 PM, Alex Williamson wrote: > On Thu, 17 Nov 2016 13:24:59 +0800 > Jike Song wrote: > >> On 11/17/2016 03:45 AM, Alex Williamson wrote: >>> Perhaps calling it a filter is not correct, I was thinking that a >>> vendor driver would register the notifier with a set of required >>> events. The driver would need to handle/ignore additional events >>> outside of the required mask. There are certainly some complications >>> to this model though that I hadn't thought all the way through until >>> now. For instance what if we add event XYZ in the future and the >>> vendor driver adds this to their required mask. If we run that on an >>> older kernel where the vfio infrastructure doesn't know about that >>> event, the vendor driver needs to fail, or at least know that event is >>> not supported and retry with a set of supported events. >>> >>> There's another problem with my proposal too, we can't put a single >>> notifier_block on multiple notifier_block heads, that just doesn't >>> work. So we probably need to separate a group notifier from an iommu >>> notifier, the vendor driver will need to register for each. >>> >>> Maybe we end up with something like: >>> >>> int vfio_register_notifier(struct device *dev, >>> vfio_notify_type_t type, >>> unsigned long *required_events, >>> struct notifier_block *nb); >>> >>> typedef unsigned short vfio_notify_type_t; >>> enum vfio_notify_type { >>> VFIO_IOMMU_NOTIFY = (__force vfio_notify_type_t)0, >>> VFIO_GROUP_NOTIFY = (__force vfio_notify_type_t)1, >>> }; >>> >>> (stealing this from pci_dev_flags_t, hope it works) >>> >>> A VFIO_GROUP_NOTIFY would add the notifier_block to the vfio_group, a >>> VFIO_IOMMU_NOTIFY would add it to vfio_iommu. Each would have their >>> own unique set of events and each would compare their supported events >>> to the required events by the caller. Supported events would be >>> cleared from the callers required_events arg. If required_events still >>> has bits set, the notifier_block is not registered, an error is >>> returned, and the caller can identify the unsupported events by the >>> remaining bits in the required_events arg. Can that work? Thanks, >> >> Let me summarize the discussion: >> >> - There should be 2 notifier heads, one in vfio_iommu another in vfio_group; >> - vfio_{un}register_notifier() has the type specified in parameter >> - In vfio_register_notifier, maybe: >> >> static vfio_iommu_register_notifier() {..} >> static vfio_group_register_notifier() {..} >> int vfio_register_notififer(type.. >> { >> if (type == VFIO_IOMMU_NOTIFY) >> return vfio_iommu_register_notifier(); >> if (type == VFIO_GROUP_NOTIFY) >> return vfio_group_register_notifier(); >> } >> >> >> >> What's more, if we still want registration to be done in mdev framework, >> we should change parent_ops: >> >> - rename 'notifier' to 'iommu_notifier' >> - add "group_notifier" >> - Add a group_events and a iommu_events to indicate what events vendor is >> interested in, respectively >> >> or otherwise don't touch it from mdev framework at all? > > I think we should remove the notifier from the mdev framework and have > the vendor drivers call vfio_{un}register_notifier() directly. Note: > > - vfio_group_release() should be modified to remove any notifier > blocks remaining to prevent a stale call chain for the next user. vfio_group_release calls vfio_group_unlock_and_free, which in turn calls kfree(group), so I guess a WARN_ON(group->notifier.head) before kfree is enough? > - vfio_sanity_check_pfn_list() should be modified to WARN_ON remaining > notifier blocks on the vfio_iommu (ie. vendor drivers will need to > actively remove iommu notifiers since the vfio_iommu can persist > beyond the attachment of the mdev group, the WARN_ON will promote a > proactive approach to surfacing such issues). I guess Kirti will prefer to pick up this? if not I also can do it :-) > I'd like to get Kirti's current series in linux-next ASAP, so please > submit a follow-on series to make these changes. I hope we can get > that finalized and added on top of Kirti's series before the v4.10 > merge window opens. Thanks, Yes, I'll send out the follow-on series ASAP, since we also have KVMGT depending on it to get notified by vfio... -- Thanks, Jike