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
