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

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 20, 2016 at 04:12:05PM +1000, David Gibson wrote:
> On Wed, Sep 14, 2016 at 04:25:46PM +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.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I still don't see the big fat comment saying that in-place changes to
> an IOMMU mapping aren't permitted.

IMHO if we are using MAP and UNMAP here then it's fairly clear even we
will support in-place change in the future. I can add one more
paragraph for in-place change like:

  For any IOMMU implementation, an in-place mapping change should be
  notified with an UNMAP following a MAP.

Do you think this works?

[...]

> > +struct IOMMUNotifier {
> > +    void (*notify)(struct IOMMUNotifier *notifier, void *data);
> 
> Given that we now have a special notifier type for this purpose, we
> could actually type this to take an IOMMUTLBEntry instead of a void *.

Yep. I can fix that.

Thanks!

-- peterx