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
signature.asc
Description: PGP signature
