From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [Qemu-devel] [RFC PATCH 12/20] Memory: Add func to fire pasidt_bind notifier Date: Thu, 27 Apr 2017 18:09:29 +0800 Message-ID: <20170427100929.GC1542@pxdev.xzpeter.org> References: <1493201210-14357-1-git-send-email-yi.l.liu@linux.intel.com> <1493201210-14357-13-git-send-email-yi.l.liu@linux.intel.com> <0f2966cf-4e5a-a2cc-5eb3-7e7d4f62bb85@redhat.com> <20170427023719.GA14925@sky-dev> <20170427061427.GA1542@pxdev.xzpeter.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Paolo Bonzini , qemu-devel@nongnu.org, alex.williamson@redhat.com, tianyu.lan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, ashok.raj@intel.com, kvm@vger.kernel.org, jean-philippe.brucker@arm.com, jasowang@redhat.com, iommu@lists.linux-foundation.org, jacob.jun.pan@intel.com To: "Liu, Yi L" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47188 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751458AbdD0KJk (ORCPT ); Thu, 27 Apr 2017 06:09:40 -0400 Content-Disposition: inline In-Reply-To: <20170427061427.GA1542@pxdev.xzpeter.org> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > + void *data) > > > > +{ > > > > + IOMMUNotifier *iommu_notifier; > > > > + IOMMUNotifierFlag request_flags; > > > > + > > > > + assert(memory_region_is_iommu(mr)); > > > > + > > > > + /*TODO: support other bind requests with smaller gran, > > > > + * e.g. bind signle pasid entry > > > > + */ > > > > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > + > > > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > + if (iommu_notifier->notifier_flags & request_flags) { > > > > + iommu_notifier->notify(iommu_notifier, data); > > > > + break; > > > > + } > > > > + } > > > > > > Peter, > > > > > > should this reuse ->notify, or should it be different function pointer > > > in IOMMUNotifier? > > > > Hi Paolo, > > > > Thx for your review. > > > > I think it should be “->notify” here. In this patchset, the new notifier > > is registered with the existing notifier registration API. So the all the > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > When the flag meets, trigger it by “->notify”. The diagram below shows > > my understanding , wish it helps to make me understood. > > > > VFIOContainer > > | > > giommu_list(VFIOGuestIOMMU) > > \ > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > > | | | > > mr->iommu_notify: IOMMUNotifier -> IOMMUNotifier -> IOMMUNotifier > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb invalidate) > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > > no start/end check, and there may be other types of bind notfier flag in > > future, so I added a separate fire func for SVM bind notifier. > > I agree with Paolo that this interface might not be the suitable place > for the SVM notifiers (just like what I worried about in previous > discussions). > > The biggest problem is that, if you see current notifier mechanism, > it's per-memory-region. However iiuc your messages should be > per-iommu, or say, per translation unit. While, for each iommu, there > can be more than one memory regions (ppc can be an example). When > there are more than one MRs binded to the same iommu unit, which > memory region should you register to? Any one of them, or all? > > So my conclusion is, it just has nothing to do with memory regions... > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > even move a step further, to isolate IOTLB notifications (targeted at > memory regions and with start/end ranges) out of SVM/other > notifications, since they are different in general. So we basically > need two notification mechanism: > > - one for memory regions, currently what I can see is IOTLB > notifications > > - one for translation units, currently I see all the rest of > notifications needed in virt-svm in this category > > Maybe some RFC patches would be good to show what I mean... I'll see > whether I can prepare some. Here it is (on qemu-devel): [RFC PATCH 0/8] IOMMU: introduce common IOMMUObject Thanks, -- Peter Xu