Re: [Qemu-devel] [PATCH v6 1/3] memory: introduce IOMMUNotifier and its caps

[Peter Xu <peterx@redhat.com>]

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