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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4719 bytes --]

On Thu, Sep 22, 2016 at 03:17:46PM +0800, Peter Xu wrote:
> 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-----------

Thanks, that looks about as good as we can get with a comment.

> 
> [...]
> 
> > > -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
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]