| Message ID | 1473226344-28520-2-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 07, 2016 at 01:32:22PM +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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions) > - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates) As noted on the other thread, I think the correct options for your bitmap here are "map" and "unmap". Which are triggered depends on the permissions / existence of the *previous* mapping, as well as the new one. You could in fact have "map-read", "map-write", "unmap-read", "unmap-write" as separate bitmap options (e.g. changing a mapping from RO to WO would be both a read-unmap and write-map event). I can't see any real use for that though, so just "map" and "unmap" are probably sufficient. > When registering the IOMMU notifier, we need to specify one or multiple > capability bit(s) to listen to. > > When notifications are triggered, it will be checked against the > notifier's capability bits, and only notifiers with registered bits will > be notified. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/vfio/common.c | 3 ++- > include/exec/memory.h | 39 ++++++++++++++++++++++++++++++++------- > include/hw/vfio/vfio-common.h | 2 +- > memory.c | 37 ++++++++++++++++++++++++++++--------- > 4 files changed, 63 insertions(+), 18 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index b313e7c..b0cea2c 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -293,7 +293,7 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) > section->offset_within_address_space & (1ULL << 63); > } > > -static void vfio_iommu_map_notify(Notifier *n, void *data) > +static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data) > { > 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_caps = IOMMU_NOTIFIER_ALL; "caps" isn't really right. It's a *requirement* that VFIO get all the notifications, not a capability. "caps" would only make sense on the other side (the vIOMMU implementation). > 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..92f14db 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -67,6 +67,28 @@ struct IOMMUTLBEntry { > IOMMUAccessFlags perm; > }; > > +/* > + * Bitmap for differnet IOMMUNotifier capabilities. Each notifier can > + * register with one or multiple IOMMU Notifier capability bit(s). > + */ > +typedef enum { > + IOMMU_NOTIFIER_NONE = 0, > + /* Notify cache invalidations */ > + IOMMU_NOTIFIER_INVALIDATION = 0x1, > + /* Notify entry changes (newly created entries) */ > + IOMMU_NOTIFIER_CHANGE = 0x2, > +} IOMMUNotifierCap; > + > +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_INVALIDATION | \ > + IOMMU_NOTIFIER_CHANGE) > + > +struct IOMMUNotifier { > + void (*notify)(struct IOMMUNotifier *notifier, void *data); > + IOMMUNotifierCap notifier_caps; > + QLIST_ENTRY(IOMMUNotifier) node; > +}; > +typedef struct IOMMUNotifier IOMMUNotifier; > + > /* New-style MMIO accessors can indicate that the transaction failed. > * A zero (MEMTX_OK) response means success; anything else is a failure > * of some kind. The memory subsystem will bitwise-OR together results > @@ -201,7 +223,7 @@ struct MemoryRegion { > const char *name; > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > - NotifierList iommu_notify; > + QLIST_HEAD(, IOMMUNotifier) iommu_notify; > }; > > /** > @@ -620,11 +642,12 @@ void memory_region_notify_iommu(MemoryRegion *mr, > * IOMMU translation entries. > * > * @mr: the memory region to observe > - * @n: the notifier to be added; the notifier receives a pointer to an > - * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > - * valid on exit from the notifier. > + * @n: the IOMMUNotifier to be added; the notify callback receives a > + * pointer to an #IOMMUTLBEntry as the opaque value; the pointer > + * ceases to be valid on exit from the notifier. > */ > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n); It seems to me that this should be allowed to fail, if the notifier you're trying to register requires notifications that the MR implementation can't supply. That seems cleaner than delaying the checking until the notification actually happens. > /** > * memory_region_iommu_replay: replay existing IOMMU translations to > @@ -636,7 +659,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > * @is_write: Whether to treat the replay as a translate "write" > * through the iommu > */ > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); > +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, > + bool is_write); > > /** > * memory_region_unregister_iommu_notifier: unregister a notifier for > @@ -646,7 +670,8 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); > * needs to be called > * @n: the notifier to be removed. > */ > -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n); > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n); > > /** > * memory_region_name: get a memory region's name > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 94dfae3..c17602e 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -93,7 +93,7 @@ typedef struct VFIOGuestIOMMU { > VFIOContainer *container; > MemoryRegion *iommu; > hwaddr iommu_offset; > - Notifier n; > + IOMMUNotifier n; > QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > } VFIOGuestIOMMU; > > diff --git a/memory.c b/memory.c > index 0eb6895..45a3902 100644 > --- a/memory.c > +++ b/memory.c > @@ -1418,7 +1418,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > memory_region_init(mr, owner, name, size); > mr->iommu_ops = ops, > mr->terminates = true; /* then re-forwards */ > - notifier_list_init(&mr->iommu_notify); > + QLIST_INIT(&mr->iommu_notify); > } > > static void memory_region_finalize(Object *obj) > @@ -1513,13 +1513,16 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) > return memory_region_get_dirty_log_mask(mr) & (1 << client); > } > > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n) > { > + /* We need to register for at least one bitfield */ > + assert(n->notifier_caps != IOMMU_NOTIFIER_NONE); Not sure if it makes sense to implement NOTIFIER_NONE as a no-op just for orthogonality. > if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + QLIST_EMPTY(&mr->iommu_notify)) { > mr->iommu_ops->notify_started(mr); As noted above, I think register_notify should get the ability to fail, which would happen if notify_started() failed (obviously it needs to get a failure mode as well. Basically notify_started is required to check that this vIOMMU is able to supply the notifications that have been requested. > } > - notifier_list_add(&mr->iommu_notify, n); > + QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > } > > uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) > @@ -1531,7 +1534,8 @@ uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) > return TARGET_PAGE_SIZE; > } > > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write) > +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, > + bool is_write) > { > hwaddr addr, granularity; > IOMMUTLBEntry iotlb; > @@ -1552,11 +1556,12 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write) > } > } > > -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > + IOMMUNotifier *n) > { > - notifier_remove(n); > + QLIST_REMOVE(n, node); > if (mr->iommu_ops->notify_stopped && > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > + QLIST_EMPTY(&mr->iommu_notify)) { > mr->iommu_ops->notify_stopped(mr); > } > } > @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) > void memory_region_notify_iommu(MemoryRegion *mr, > IOMMUTLBEntry entry) > { > + IOMMUNotifier *iommu_notifier; > + IOMMUNotifierCap request_cap; > + > assert(memory_region_is_iommu(mr)); > - notifier_list_notify(&mr->iommu_notify, &entry); > + > + if (entry.perm & IOMMU_RW) { > + request_cap = IOMMU_NOTIFIER_CHANGE; > + } else { > + request_cap = IOMMU_NOTIFIER_INVALIDATION; > + } As noted right at the top, I don't think this logic is really right. An in-place change should be treated as both a map and unmap. > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + if (iommu_notifier->notifier_caps & request_cap) { > + iommu_notifier->notify(iommu_notifier, &entry); > + } > + } > } > > void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
On Wed, Sep 07, 2016 at 04:02:39PM +1000, David Gibson wrote: > On Wed, Sep 07, 2016 at 01:32:22PM +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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions) > > - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates) > > As noted on the other thread, I think the correct options for your > bitmap here are "map" and "unmap". Which are triggered depends on the > permissions / existence of the *previous* mapping, as well as the new > one. As mentioned in previous reply, both work for me. :) If you insist on changing it (without anyone that strongly disagree...), I can do it in next spin. > > You could in fact have "map-read", "map-write", "unmap-read", > "unmap-write" as separate bitmap options (e.g. changing a mapping from > RO to WO would be both a read-unmap and write-map event). I can't see > any real use for that though, so just "map" and "unmap" are probably > sufficient. Agreed. We can enhance it in the future if there is any real requirement. Before that, it would be over-engineering. (Btw, we should not need {read|write}_unmap in all cases. IIUC unmap should not need any permission check.) [...] > > -static void vfio_iommu_map_notify(Notifier *n, void *data) > > +static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data) > > { > > 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_caps = IOMMU_NOTIFIER_ALL; > > "caps" isn't really right. It's a *requirement* that VFIO get all the > notifications, not a capability. "caps" would only make sense on the > other side (the vIOMMU implementation). Sounds reasonable. How about "flags"? Or any suggestion? [...] > > /** > > @@ -620,11 +642,12 @@ void memory_region_notify_iommu(MemoryRegion *mr, > > * IOMMU translation entries. > > * > > * @mr: the memory region to observe > > - * @n: the notifier to be added; the notifier receives a pointer to an > > - * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > > - * valid on exit from the notifier. > > + * @n: the IOMMUNotifier to be added; the notify callback receives a > > + * pointer to an #IOMMUTLBEntry as the opaque value; the pointer > > + * ceases to be valid on exit from the notifier. > > */ > > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > > + IOMMUNotifier *n); > > It seems to me that this should be allowed to fail, if the notifier > you're trying to register requires notifications that the MR > implementation can't supply. That seems cleaner than delaying the > checking until the notification actually happens. Do we have real use case for this one? For example, do we have case that VM "registering required notifications that MR cannot support" can still work? Currently there is only one use case AFAIK, which is VFIO+IntelIOMMU. In that case, I take it a configuration error (we should never allow that configuration happen). IMHO All configuration errors should be reported ASAP, and we should never let VM start. > > > /** > > * memory_region_iommu_replay: replay existing IOMMU translations to > > @@ -636,7 +659,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > * @is_write: Whether to treat the replay as a translate "write" > > * through the iommu > > */ > > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); > > +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, > > + bool is_write); > > > > /** > > * memory_region_unregister_iommu_notifier: unregister a notifier for > > @@ -646,7 +670,8 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); > > * needs to be called > > * @n: the notifier to be removed. > > */ > > -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n); > > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > > + IOMMUNotifier *n); > > > > /** > > * memory_region_name: get a memory region's name > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > > index 94dfae3..c17602e 100644 > > --- a/include/hw/vfio/vfio-common.h > > +++ b/include/hw/vfio/vfio-common.h > > @@ -93,7 +93,7 @@ typedef struct VFIOGuestIOMMU { > > VFIOContainer *container; > > MemoryRegion *iommu; > > hwaddr iommu_offset; > > - Notifier n; > > + IOMMUNotifier n; > > QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > > } VFIOGuestIOMMU; > > > > diff --git a/memory.c b/memory.c > > index 0eb6895..45a3902 100644 > > --- a/memory.c > > +++ b/memory.c > > @@ -1418,7 +1418,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > > memory_region_init(mr, owner, name, size); > > mr->iommu_ops = ops, > > mr->terminates = true; /* then re-forwards */ > > - notifier_list_init(&mr->iommu_notify); > > + QLIST_INIT(&mr->iommu_notify); > > } > > > > static void memory_region_finalize(Object *obj) > > @@ -1513,13 +1513,16 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) > > return memory_region_get_dirty_log_mask(mr) & (1 << client); > > } > > > > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > > + IOMMUNotifier *n) > > { > > + /* We need to register for at least one bitfield */ > > + assert(n->notifier_caps != IOMMU_NOTIFIER_NONE); > > Not sure if it makes sense to implement NOTIFIER_NONE as a no-op just > for orthogonality. I would think it is a programming error when registering to IOMMO notifier without any flags set (or cap). So I put an assert() here. Even if we have a no-op thing, anyone registers with IOMMU_NOTIFIER_NONE flag is suspecious and strange. > > > if (mr->iommu_ops->notify_started && > > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > > + QLIST_EMPTY(&mr->iommu_notify)) { > > mr->iommu_ops->notify_started(mr); > > As noted above, I think register_notify should get the ability to > fail, which would happen if notify_started() failed (obviously it > needs to get a failure mode as well. Basically notify_started is > required to check that this vIOMMU is able to supply the notifications > that have been requested. Same as above, I just failed to think out a use case for that yet. As long as we can have a use case for it, it's easy to replace the returned void type into something like int, and report it up along the way. [...] > > @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) > > void memory_region_notify_iommu(MemoryRegion *mr, > > IOMMUTLBEntry entry) > > { > > + IOMMUNotifier *iommu_notifier; > > + IOMMUNotifierCap request_cap; > > + > > assert(memory_region_is_iommu(mr)); > > - notifier_list_notify(&mr->iommu_notify, &entry); > > + > > + if (entry.perm & IOMMU_RW) { > > + request_cap = IOMMU_NOTIFIER_CHANGE; > > + } else { > > + request_cap = IOMMU_NOTIFIER_INVALIDATION; > > + } > > As noted right at the top, I don't think this logic is really right. > An in-place change should be treated as both a map and unmap. IIUC, guest needs to send two invalidations for an in-place change, right? So a "change" will actually trigger this twice. (At least this should be true for Power, and I am guessing the same for Intel, otherwise PowerIOMMU+VFIO should not work before this series...) Thanks! -- peterx
On Wed, Sep 07, 2016 at 03:09:16PM +0800, Peter Xu wrote: > On Wed, Sep 07, 2016 at 04:02:39PM +1000, David Gibson wrote: > > On Wed, Sep 07, 2016 at 01:32:22PM +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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions) > > > - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates) > > > > As noted on the other thread, I think the correct options for your > > bitmap here are "map" and "unmap". Which are triggered depends on the > > permissions / existence of the *previous* mapping, as well as the new > > one. > > As mentioned in previous reply, both work for me. :) > > If you insist on changing it (without anyone that strongly > disagree...), I can do it in next spin. This is not just a name change I'm proposing, but a semantic change (or at least a clarification). > > You could in fact have "map-read", "map-write", "unmap-read", > > "unmap-write" as separate bitmap options (e.g. changing a mapping from > > RO to WO would be both a read-unmap and write-map event). I can't see > > any real use for that though, so just "map" and "unmap" are probably > > sufficient. > > Agreed. We can enhance it in the future if there is any real > requirement. Before that, it would be over-engineering. > > (Btw, we should not need {read|write}_unmap in all cases. IIUC unmap > should not need any permission check.) In practice probably not, but they are distinct operations. read_unmap means a readable mapping has been removed, write_unmap means a writable mapping has been removed. Again - the permissions on the *old* mapping are what matters here. > > [...] > > > > -static void vfio_iommu_map_notify(Notifier *n, void *data) > > > +static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data) > > > { > > > 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_caps = IOMMU_NOTIFIER_ALL; > > > > "caps" isn't really right. It's a *requirement* that VFIO get all the > > notifications, not a capability. "caps" would only make sense on the > > other side (the vIOMMU implementation). > > Sounds reasonable. How about "flags"? Or any suggestion? "flags" would do. I feel like there should be a better name, but I can't think of it. > [...] > > > > /** > > > @@ -620,11 +642,12 @@ void memory_region_notify_iommu(MemoryRegion *mr, > > > * IOMMU translation entries. > > > * > > > * @mr: the memory region to observe > > > - * @n: the notifier to be added; the notifier receives a pointer to an > > > - * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > > > - * valid on exit from the notifier. > > > + * @n: the IOMMUNotifier to be added; the notify callback receives a > > > + * pointer to an #IOMMUTLBEntry as the opaque value; the pointer > > > + * ceases to be valid on exit from the notifier. > > > */ > > > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > > > + IOMMUNotifier *n); > > > > It seems to me that this should be allowed to fail, if the notifier > > you're trying to register requires notifications that the MR > > implementation can't supply. That seems cleaner than delaying the > > checking until the notification actually happens. > > Do we have real use case for this one? For example, do we have case > that VM "registering required notifications that MR cannot support" > can still work? > > Currently there is only one use case AFAIK, which is VFIO+IntelIOMMU. > In that case, I take it a configuration error (we should never allow > that configuration happen). IMHO All configuration errors should be > reported ASAP, and we should never let VM start. Yes... I'm not proposing changing that. I just think it would be cleaner to report the error through the error channels, instead of just aborting. > > > /** > > > * memory_region_iommu_replay: replay existing IOMMU translations to > > > @@ -636,7 +659,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > > * @is_write: Whether to treat the replay as a translate "write" > > > * through the iommu > > > */ > > > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); > > > +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, > > > + bool is_write); > > > > > > /** > > > * memory_region_unregister_iommu_notifier: unregister a notifier for > > > @@ -646,7 +670,8 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); > > > * needs to be called > > > * @n: the notifier to be removed. > > > */ > > > -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n); > > > +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > > > + IOMMUNotifier *n); > > > > > > /** > > > * memory_region_name: get a memory region's name > > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > > > index 94dfae3..c17602e 100644 > > > --- a/include/hw/vfio/vfio-common.h > > > +++ b/include/hw/vfio/vfio-common.h > > > @@ -93,7 +93,7 @@ typedef struct VFIOGuestIOMMU { > > > VFIOContainer *container; > > > MemoryRegion *iommu; > > > hwaddr iommu_offset; > > > - Notifier n; > > > + IOMMUNotifier n; > > > QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > > > } VFIOGuestIOMMU; > > > > > > diff --git a/memory.c b/memory.c > > > index 0eb6895..45a3902 100644 > > > --- a/memory.c > > > +++ b/memory.c > > > @@ -1418,7 +1418,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > > > memory_region_init(mr, owner, name, size); > > > mr->iommu_ops = ops, > > > mr->terminates = true; /* then re-forwards */ > > > - notifier_list_init(&mr->iommu_notify); > > > + QLIST_INIT(&mr->iommu_notify); > > > } > > > > > > static void memory_region_finalize(Object *obj) > > > @@ -1513,13 +1513,16 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) > > > return memory_region_get_dirty_log_mask(mr) & (1 << client); > > > } > > > > > > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > > > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > > > + IOMMUNotifier *n) > > > { > > > + /* We need to register for at least one bitfield */ > > > + assert(n->notifier_caps != IOMMU_NOTIFIER_NONE); > > > > Not sure if it makes sense to implement NOTIFIER_NONE as a no-op just > > for orthogonality. > > I would think it is a programming error when registering to IOMMO > notifier without any flags set (or cap). So I put an assert() here. > Even if we have a no-op thing, anyone registers with > IOMMU_NOTIFIER_NONE flag is suspecious and strange. True. > > > > > if (mr->iommu_ops->notify_started && > > > - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { > > > + QLIST_EMPTY(&mr->iommu_notify)) { > > > mr->iommu_ops->notify_started(mr); > > > > As noted above, I think register_notify should get the ability to > > fail, which would happen if notify_started() failed (obviously it > > needs to get a failure mode as well. Basically notify_started is > > required to check that this vIOMMU is able to supply the notifications > > that have been requested. > > Same as above, I just failed to think out a use case for that yet. As > long as we can have a use case for it, it's easy to replace the > returned void type into something like int, and report it up along the > way. Hm, I suppose. > > [...] > > > > @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) > > > void memory_region_notify_iommu(MemoryRegion *mr, > > > IOMMUTLBEntry entry) > > > { > > > + IOMMUNotifier *iommu_notifier; > > > + IOMMUNotifierCap request_cap; > > > + > > > assert(memory_region_is_iommu(mr)); > > > - notifier_list_notify(&mr->iommu_notify, &entry); > > > + > > > + if (entry.perm & IOMMU_RW) { > > > + request_cap = IOMMU_NOTIFIER_CHANGE; > > > + } else { > > > + request_cap = IOMMU_NOTIFIER_INVALIDATION; > > > + } > > > > As noted right at the top, I don't think this logic is really right. > > An in-place change should be treated as both a map and unmap. > > IIUC, guest needs to send two invalidations for an in-place change, > right? So a "change" will actually trigger this twice. No, or at least not necessarily. How the invalidate is reported really depends on the guest side vIOMMU interface. Maybe it requires an explicit invalidate followed by a set, maybe a direct in place replacement is possible (the PAPR hypercall interface allows this at least in theory). What I had in mind here is that assuming the vIOMMU can detect an in place change, it would then ping all notifiers that have *either* the MAP or UNMAP flag bits set. > (At least this should be true for Power, and I am guessing the same > for Intel, otherwise PowerIOMMU+VFIO should not work before this > series...) I don't really follow what you're saying here.
On Wed, Sep 07, 2016 at 08:20:35PM +1000, David Gibson wrote: > On Wed, Sep 07, 2016 at 03:09:16PM +0800, Peter Xu wrote: > > On Wed, Sep 07, 2016 at 04:02:39PM +1000, David Gibson wrote: > > > On Wed, Sep 07, 2016 at 01:32:22PM +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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions) > > > > - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates) > > > > > > As noted on the other thread, I think the correct options for your > > > bitmap here are "map" and "unmap". Which are triggered depends on the > > > permissions / existence of the *previous* mapping, as well as the new > > > one. > > > > As mentioned in previous reply, both work for me. :) > > > > If you insist on changing it (without anyone that strongly > > disagree...), I can do it in next spin. > > This is not just a name change I'm proposing, but a semantic change > (or at least a clarification). I see that kernel IOMMU driver is using map_page() and unmap_page() for its interface. Now I prefer MAP/UNMAP. > > > > You could in fact have "map-read", "map-write", "unmap-read", > > > "unmap-write" as separate bitmap options (e.g. changing a mapping from > > > RO to WO would be both a read-unmap and write-map event). I can't see > > > any real use for that though, so just "map" and "unmap" are probably > > > sufficient. > > > > Agreed. We can enhance it in the future if there is any real > > requirement. Before that, it would be over-engineering. > > > > (Btw, we should not need {read|write}_unmap in all cases. IIUC unmap > > should not need any permission check.) > > In practice probably not, but they are distinct operations. > read_unmap means a readable mapping has been removed, write_unmap > means a writable mapping has been removed. Again - the permissions on > the *old* mapping are what matters here. Why they are distinct operations? Or could you help explain in what case would we need this flag (read/write) for an unmap() operation? > > > > > [...] > > > > > > -static void vfio_iommu_map_notify(Notifier *n, void *data) > > > > +static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data) > > > > { > > > > 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_caps = IOMMU_NOTIFIER_ALL; > > > > > > "caps" isn't really right. It's a *requirement* that VFIO get all the > > > notifications, not a capability. "caps" would only make sense on the > > > other side (the vIOMMU implementation). > > > > Sounds reasonable. How about "flags"? Or any suggestion? > > "flags" would do. I feel like there should be a better name, but I > can't think of it. Sure. I can switch. > > > [...] > > > > > > /** > > > > @@ -620,11 +642,12 @@ void memory_region_notify_iommu(MemoryRegion *mr, > > > > * IOMMU translation entries. > > > > * > > > > * @mr: the memory region to observe > > > > - * @n: the notifier to be added; the notifier receives a pointer to an > > > > - * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > > > > - * valid on exit from the notifier. > > > > + * @n: the IOMMUNotifier to be added; the notify callback receives a > > > > + * pointer to an #IOMMUTLBEntry as the opaque value; the pointer > > > > + * ceases to be valid on exit from the notifier. > > > > */ > > > > -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > > > +void memory_region_register_iommu_notifier(MemoryRegion *mr, > > > > + IOMMUNotifier *n); > > > > > > It seems to me that this should be allowed to fail, if the notifier > > > you're trying to register requires notifications that the MR > > > implementation can't supply. That seems cleaner than delaying the > > > checking until the notification actually happens. > > > > Do we have real use case for this one? For example, do we have case > > that VM "registering required notifications that MR cannot support" > > can still work? > > > > Currently there is only one use case AFAIK, which is VFIO+IntelIOMMU. > > In that case, I take it a configuration error (we should never allow > > that configuration happen). IMHO All configuration errors should be > > reported ASAP, and we should never let VM start. > > Yes... I'm not proposing changing that. I just think it would be > cleaner to report the error through the error channels, instead of > just aborting. Agree. But since I have no obvious reason to change the return code, I'd prefer to keep it as it is for this series if you don't mind. [...] > > > > @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) > > > > void memory_region_notify_iommu(MemoryRegion *mr, > > > > IOMMUTLBEntry entry) > > > > { > > > > + IOMMUNotifier *iommu_notifier; > > > > + IOMMUNotifierCap request_cap; > > > > + > > > > assert(memory_region_is_iommu(mr)); > > > > - notifier_list_notify(&mr->iommu_notify, &entry); > > > > + > > > > + if (entry.perm & IOMMU_RW) { > > > > + request_cap = IOMMU_NOTIFIER_CHANGE; > > > > + } else { > > > > + request_cap = IOMMU_NOTIFIER_INVALIDATION; > > > > + } > > > > > > As noted right at the top, I don't think this logic is really right. > > > An in-place change should be treated as both a map and unmap. > > > > IIUC, guest needs to send two invalidations for an in-place change, > > right? So a "change" will actually trigger this twice. > > No, or at least not necessarily. How the invalidate is reported > really depends on the guest side vIOMMU interface. Maybe it requires > an explicit invalidate followed by a set, maybe a direct in place > replacement is possible (the PAPR hypercall interface allows this at > least in theory). > > What I had in mind here is that assuming the vIOMMU can detect an in > place change, it would then ping all notifiers that have *either* the > MAP or UNMAP flag bits set. Yes, so I think we don't need a "change" interface. And maybe we should start using MAP/UNMAP for the flags naming to avoid unecessary misunderstandings (when we talk about CHANGE flag, it's actually map(), but not unmap() + map()). > > > (At least this should be true for Power, and I am guessing the same > > for Intel, otherwise PowerIOMMU+VFIO should not work before this > > series...) > > I don't really follow what you're saying here. I meant at least current Power IOMMU should be sending two notifies if a "change" happened, otherwise current code won't work. Thanks, -- peterx
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index b313e7c..b0cea2c 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -293,7 +293,7 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section) section->offset_within_address_space & (1ULL << 63); } -static void vfio_iommu_map_notify(Notifier *n, void *data) +static void vfio_iommu_map_notify(IOMMUNotifier *n, void *data) { 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_caps = 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..92f14db 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -67,6 +67,28 @@ struct IOMMUTLBEntry { IOMMUAccessFlags perm; }; +/* + * Bitmap for differnet IOMMUNotifier capabilities. Each notifier can + * register with one or multiple IOMMU Notifier capability bit(s). + */ +typedef enum { + IOMMU_NOTIFIER_NONE = 0, + /* Notify cache invalidations */ + IOMMU_NOTIFIER_INVALIDATION = 0x1, + /* Notify entry changes (newly created entries) */ + IOMMU_NOTIFIER_CHANGE = 0x2, +} IOMMUNotifierCap; + +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_INVALIDATION | \ + IOMMU_NOTIFIER_CHANGE) + +struct IOMMUNotifier { + void (*notify)(struct IOMMUNotifier *notifier, void *data); + IOMMUNotifierCap notifier_caps; + QLIST_ENTRY(IOMMUNotifier) node; +}; +typedef struct IOMMUNotifier IOMMUNotifier; + /* New-style MMIO accessors can indicate that the transaction failed. * A zero (MEMTX_OK) response means success; anything else is a failure * of some kind. The memory subsystem will bitwise-OR together results @@ -201,7 +223,7 @@ struct MemoryRegion { const char *name; unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; - NotifierList iommu_notify; + QLIST_HEAD(, IOMMUNotifier) iommu_notify; }; /** @@ -620,11 +642,12 @@ void memory_region_notify_iommu(MemoryRegion *mr, * IOMMU translation entries. * * @mr: the memory region to observe - * @n: the notifier to be added; the notifier receives a pointer to an - * #IOMMUTLBEntry as the opaque value; the pointer ceases to be - * valid on exit from the notifier. + * @n: the IOMMUNotifier to be added; the notify callback receives a + * pointer to an #IOMMUTLBEntry as the opaque value; the pointer + * ceases to be valid on exit from the notifier. */ -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); +void memory_region_register_iommu_notifier(MemoryRegion *mr, + IOMMUNotifier *n); /** * memory_region_iommu_replay: replay existing IOMMU translations to @@ -636,7 +659,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); * @is_write: Whether to treat the replay as a translate "write" * through the iommu */ -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, + bool is_write); /** * memory_region_unregister_iommu_notifier: unregister a notifier for @@ -646,7 +670,8 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write); * needs to be called * @n: the notifier to be removed. */ -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n); +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, + IOMMUNotifier *n); /** * memory_region_name: get a memory region's name diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 94dfae3..c17602e 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -93,7 +93,7 @@ typedef struct VFIOGuestIOMMU { VFIOContainer *container; MemoryRegion *iommu; hwaddr iommu_offset; - Notifier n; + IOMMUNotifier n; QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; } VFIOGuestIOMMU; diff --git a/memory.c b/memory.c index 0eb6895..45a3902 100644 --- a/memory.c +++ b/memory.c @@ -1418,7 +1418,7 @@ void memory_region_init_iommu(MemoryRegion *mr, memory_region_init(mr, owner, name, size); mr->iommu_ops = ops, mr->terminates = true; /* then re-forwards */ - notifier_list_init(&mr->iommu_notify); + QLIST_INIT(&mr->iommu_notify); } static void memory_region_finalize(Object *obj) @@ -1513,13 +1513,16 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) return memory_region_get_dirty_log_mask(mr) & (1 << client); } -void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) +void memory_region_register_iommu_notifier(MemoryRegion *mr, + IOMMUNotifier *n) { + /* We need to register for at least one bitfield */ + assert(n->notifier_caps != IOMMU_NOTIFIER_NONE); if (mr->iommu_ops->notify_started && - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { + QLIST_EMPTY(&mr->iommu_notify)) { mr->iommu_ops->notify_started(mr); } - notifier_list_add(&mr->iommu_notify, n); + QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); } uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) @@ -1531,7 +1534,8 @@ uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) return TARGET_PAGE_SIZE; } -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write) +void memory_region_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n, + bool is_write) { hwaddr addr, granularity; IOMMUTLBEntry iotlb; @@ -1552,11 +1556,12 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write) } } -void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, + IOMMUNotifier *n) { - notifier_remove(n); + QLIST_REMOVE(n, node); if (mr->iommu_ops->notify_stopped && - QLIST_EMPTY(&mr->iommu_notify.notifiers)) { + QLIST_EMPTY(&mr->iommu_notify)) { mr->iommu_ops->notify_stopped(mr); } } @@ -1564,8 +1569,22 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n) void memory_region_notify_iommu(MemoryRegion *mr, IOMMUTLBEntry entry) { + IOMMUNotifier *iommu_notifier; + IOMMUNotifierCap request_cap; + assert(memory_region_is_iommu(mr)); - notifier_list_notify(&mr->iommu_notify, &entry); + + if (entry.perm & IOMMU_RW) { + request_cap = IOMMU_NOTIFIER_CHANGE; + } else { + request_cap = IOMMU_NOTIFIER_INVALIDATION; + } + + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { + if (iommu_notifier->notifier_caps & request_cap) { + iommu_notifier->notify(iommu_notifier, &entry); + } + } } void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
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 IOMMUNotfierCap 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_CHANGE: for entry changes (additions) - IOMMU_NOTIFIER_INVALIDATION: for entry removals (cache invalidates) When registering the IOMMU notifier, we need to specify one or multiple capability bit(s) to listen to. When notifications are triggered, it will be checked against the notifier's capability bits, and only notifiers with registered bits will be notified. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/vfio/common.c | 3 ++- include/exec/memory.h | 39 ++++++++++++++++++++++++++++++++------- include/hw/vfio/vfio-common.h | 2 +- memory.c | 37 ++++++++++++++++++++++++++++--------- 4 files changed, 63 insertions(+), 18 deletions(-)