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: Wed, 16 Nov 2016 23:03:16 -0700 Message-ID: <20161116230316.11f2cde8@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> <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> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Jike Song Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45052 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751059AbcKQGDS (ORCPT ); Thu, 17 Nov 2016 01:03:18 -0500 In-Reply-To: <582D3F2B.5070702@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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_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'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, Alex