[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/3] memory: introduce IOMMUNotifier and its
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/3] memory: introduce IOMMUNotifier and its caps |
Date: |
Fri, 23 Sep 2016 10:36:25 +1000 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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