On Thu, Apr 27, 2017 at 06:25:37PM +0800, Liu, Yi L wrote:
> 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.
>
> Hi Peter,
>
> yes, you're right. the newly added notifier is per-iommu.
>
> > 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?
>
> Honestly, I'm not expert on ppc. According to the current code,
> I can only find one MR initialized with memory_region_init_iommu()
> in spapr_tce_table_realize(). So to better get your point, let me
> check. Do you mean there may be multiple of iommu MRs behind a iommu?
I am not either. :)
But yes, that's what I mean. At least that's how I understand it.
>
> I admit it must be considered if there are multiple iommu MRs. I may
> choose to register for one of them since the notifier is per-iommu as
> you've pointed. Then vIOMMU emulator need to trigger the notifier with
> the correct MR. Not sure if ppc vIOMMU is fine with it.
>
> > 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.
>
> I agree that it would be helpful to split the two kinds of notifiers. I
> marked it as a FIXME in patch 0006 of this series. Just saw your RFC patch
> for common IOMMUObject. Thx for your work, would try to review it.
Thanks, looking forward to your review comments.
>
> Besides the notifier registration, pls also help to review the SVM
> virtualization itself. Would be glad to know your comments.
Yes. It's on my list. Thanks,
--
Peter Xu
