On Thu, Sep 22, 2016 at 03:20:48PM +1000, David Gibson wrote:
> On Wed, Sep 21, 2016 at 12:58:54PM +0800, Peter Xu wrote:
> > IOMMU Notifier list is used for notifying IO address mapping changes.
> > Currently VFIO is the only user.
> >
> > However it is possible that future consumer like vhost would like to
> > only listen to part of its notifications (e.g., cache invalidations).
> >
> > This patch introduced IOMMUNotifier and IOMMUNotfierFlag bits for a
> > finer grained control of it.
> >
> > IOMMUNotifier contains a bitfield for the notify consumer describing
> > what kind of notification it is interested in. Currently two kinds of
> > notifications are defined:
> >
> > - IOMMU_NOTIFIER_MAP: for newly mapped entries (additions)
> > - IOMMU_NOTIFIER_UNMAP: for entries to be removed (cache invalidates)
> >
> > When registering the IOMMU notifier, we need to specify one or multiple
> > types of messages to listen to.
> >
> > When notifications are triggered, its type will be checked against the
> > notifier's type bits, and only notifiers with registered bits will be
> > notified.
> >
> > (For any IOMMU implementation, an in-place mapping change should be
> > notified with an UNMAP followed by a MAP.)
>
> Ok, I wasn't clear. I meant a big fat comment in the *code*, not just
> in the commit message. It should not be necessary to look at the
> commit history to figure out how to use an interface correctly
>
> Even a comment in the code is barely adequate, compared to designing
> the interface signatures such that it's obvious.
>
> Please bear in mind:
> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> and http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html
>
Thanks for the links.
Maybe a better solution is to re-design the IOTLB interface. However
that's out of the scope of this series, and another patchset can be
opened for the refactoring work IMHO if there is a strong willingness.
For now I can add this into comments:
---------8<-----------
@@ -607,6 +628,15 @@ uint64_t
memory_region_iommu_get_min_page_size(MemoryRegion *mr);
/**
* memory_region_notify_iommu: notify a change in an IOMMU translation entry.
*
+ * The notification type will be decided by entry.perm bits:
+ *
+ * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE.
+ * - For MAP (newly added entry) notifies: set entry.perm to the
+ * permission of the page (which is definitely !IOMMU_NONE).
+ *
+ * Note: for any IOMMU implementation, an in-place mapping change
+ * should be notified with an UNMAP followed by a MAP.
+ *
* @mr: the memory region that was changed
* @entry: the new entry in the IOMMU translation table. The entry
* replaces all old entries for the same virtual I/O address range.
--------->8-----------
[...]
> > -static void vfio_iommu_map_notify(Notifier *n, void *data)
> > +static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *data)
>
> This change leaves a now pointless IOMMUTLBEntry *iotlb = data a few
> lines below.
Yes, will fix.
>
> > {
> > VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > VFIOContainer *container = giommu->container;
> > @@ -454,6 +454,7 @@ static void vfio_listener_region_add(MemoryListener
> > *listener,
> > section->offset_within_region;
> > giommu->container = container;
> > giommu->n.notify = vfio_iommu_map_notify;
> > + giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
> > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> >
> > memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 3e4d416..a3ec7aa 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -67,6 +67,27 @@ struct IOMMUTLBEntry {
> > IOMMUAccessFlags perm;
> > };
> >
> > +/*
> > + * Bitmap for differnet IOMMUNotifier capabilities. Each notifier can
>
> s/differnet/different/
Will fix. Thanks.
-- peterx
