Re: [Qemu-devel] [RFC PATCH 12/20] Memory: Add func to fire pasidt_bind notifier

[Peter Xu]

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.

Thanks,

-- 
Peter Xu