* [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct @ 2016-09-09 2:57 Peter Xu 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps Peter Xu ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Peter Xu @ 2016-09-09 2:57 UTC (permalink / raw) To: qemu-devel Cc: dgibson, jasowang, mst, pbonzini, cornelia.huck, wexu, vkaplans, alex.williamson, peterx V4: - change "notifier_caps" into "notifier_flags" [David] - rename IOMMU_NOTIFIER_{CHANGE|INVALIDATION} with MAP/UNMAP [David] - introduce IOMMUOps.notify_flag_changed, to replace notify_started and notify_stopped [David, Paolo] V3: - use QLIST instead of embedding Notifier into IOMMUNotifier [Paolo] - fix a build error for ppc64-softmmu The idea originates from one of Alex's reply: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00254.html But after further discussions, it seems that only adding a simple type for notifier is not enough. This series introduced IOMMUNotifier struct to replace the old Notifier interface. Along with it, we can provide registration for one (or multiple) of the IOMMU notifications: - cache invalidations - entry changes This is a support material for Jason's vhost dmar patchset. Please read commit messages for detailed information. Thanks, Peter Xu (3): memory: introduce IOMMUNotifier and its caps memory: introduce IOMMUOps.notify_flag_changed intel_iommu: allow UNMAP notifiers hw/i386/intel_iommu.c | 18 ++++++++----- hw/ppc/spapr_iommu.c | 14 ++++++++-- hw/vfio/common.c | 3 ++- include/exec/memory.h | 47 +++++++++++++++++++++++++-------- include/hw/vfio/vfio-common.h | 2 +- memory.c | 60 +++++++++++++++++++++++++++++++++---------- 6 files changed, 109 insertions(+), 35 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-09 2:57 [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct Peter Xu @ 2016-09-09 2:57 ` Peter Xu 2016-09-14 5:48 ` David Gibson 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed Peter Xu ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-09 2:57 UTC (permalink / raw) To: qemu-devel Cc: dgibson, jasowang, mst, pbonzini, cornelia.huck, wexu, vkaplans, alex.williamson, peterx 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> --- hw/vfio/common.c | 3 ++- include/exec/memory.h | 38 +++++++++++++++++++++++++++++++------- include/hw/vfio/vfio-common.h | 2 +- memory.c | 37 ++++++++++++++++++++++++++++--------- 4 files changed, 62 insertions(+), 18 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index b313e7c..41b6a13 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_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..e69e984 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 + * register with one or multiple IOMMU Notifier capability bit(s). + */ +typedef enum { + IOMMU_NOTIFIER_NONE = 0, + /* Notify cache invalidations */ + IOMMU_NOTIFIER_UNMAP = 0x1, + /* Notify entry changes (newly created entries) */ + IOMMU_NOTIFIER_MAP = 0x2, +} IOMMUNotifierFlag; + +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) + +struct IOMMUNotifier { + void (*notify)(struct IOMMUNotifier *notifier, void *data); + IOMMUNotifierFlag notifier_flags; + 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 +222,7 @@ struct MemoryRegion { const char *name; unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; - NotifierList iommu_notify; + QLIST_HEAD(, IOMMUNotifier) iommu_notify; }; /** @@ -620,11 +641,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 +658,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 +669,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..f65c600 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_flags != 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; + IOMMUNotifierFlag request_flags; + assert(memory_region_is_iommu(mr)); - notifier_list_notify(&mr->iommu_notify, &entry); + + if (entry.perm & IOMMU_RW) { + request_flags = IOMMU_NOTIFIER_MAP; + } else { + request_flags = IOMMU_NOTIFIER_UNMAP; + } + + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { + if (iommu_notifier->notifier_flags & request_flags) { + iommu_notifier->notify(iommu_notifier, &entry); + } + } } void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps Peter Xu @ 2016-09-14 5:48 ` David Gibson 2016-09-14 7:15 ` David Gibson 0 siblings, 1 reply; 19+ messages in thread From: David Gibson @ 2016-09-14 5:48 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson [-- Attachment #1: Type: text/plain, Size: 9697 bytes --] On Fri, Sep 09, 2016 at 10:57:42AM +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> > --- > hw/vfio/common.c | 3 ++- > include/exec/memory.h | 38 +++++++++++++++++++++++++++++++------- > include/hw/vfio/vfio-common.h | 2 +- > memory.c | 37 ++++++++++++++++++++++++++++--------- > 4 files changed, 62 insertions(+), 18 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index b313e7c..41b6a13 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_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..e69e984 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 > + * register with one or multiple IOMMU Notifier capability bit(s). > + */ > +typedef enum { > + IOMMU_NOTIFIER_NONE = 0, > + /* Notify cache invalidations */ > + IOMMU_NOTIFIER_UNMAP = 0x1, > + /* Notify entry changes (newly created entries) */ > + IOMMU_NOTIFIER_MAP = 0x2, > +} IOMMUNotifierFlag; > + > +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) > + > +struct IOMMUNotifier { > + void (*notify)(struct IOMMUNotifier *notifier, void *data); > + IOMMUNotifierFlag notifier_flags; > + 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 +222,7 @@ struct MemoryRegion { > const char *name; > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > - NotifierList iommu_notify; > + QLIST_HEAD(, IOMMUNotifier) iommu_notify; > }; > > /** > @@ -620,11 +641,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 +658,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 +669,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..f65c600 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_flags != 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; > + IOMMUNotifierFlag request_flags; > + > assert(memory_region_is_iommu(mr)); > - notifier_list_notify(&mr->iommu_notify, &entry); > + > + if (entry.perm & IOMMU_RW) { > + request_flags = IOMMU_NOTIFIER_MAP; > + } else { > + request_flags = IOMMU_NOTIFIER_UNMAP; > + } This is still wrong. UNMAP depends on the *previous* state of the mapping, not the new state. > + > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + if (iommu_notifier->notifier_flags & request_flags) { > + iommu_notifier->notify(iommu_notifier, &entry); > + } > + } > } > > void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) -- 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-14 5:48 ` David Gibson @ 2016-09-14 7:15 ` David Gibson 2016-09-14 7:40 ` Peter Xu 2016-09-14 8:17 ` Peter Xu 0 siblings, 2 replies; 19+ messages in thread From: David Gibson @ 2016-09-14 7:15 UTC (permalink / raw) To: Peter Xu Cc: mst, jasowang, qemu-devel, cornelia.huck, alex.williamson, wexu, dgibson, vkaplans, pbonzini [-- Attachment #1: Type: text/plain, Size: 11702 bytes --] On Wed, Sep 14, 2016 at 03:48:32PM +1000, David Gibson wrote: > On Fri, Sep 09, 2016 at 10:57:42AM +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> > > --- > > hw/vfio/common.c | 3 ++- > > include/exec/memory.h | 38 +++++++++++++++++++++++++++++++------- > > include/hw/vfio/vfio-common.h | 2 +- > > memory.c | 37 ++++++++++++++++++++++++++++--------- > > 4 files changed, 62 insertions(+), 18 deletions(-) > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > index b313e7c..41b6a13 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_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..e69e984 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 > > + * register with one or multiple IOMMU Notifier capability bit(s). > > + */ > > +typedef enum { > > + IOMMU_NOTIFIER_NONE = 0, > > + /* Notify cache invalidations */ > > + IOMMU_NOTIFIER_UNMAP = 0x1, > > + /* Notify entry changes (newly created entries) */ > > + IOMMU_NOTIFIER_MAP = 0x2, > > +} IOMMUNotifierFlag; > > + > > +#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) > > + > > +struct IOMMUNotifier { > > + void (*notify)(struct IOMMUNotifier *notifier, void *data); > > + IOMMUNotifierFlag notifier_flags; > > + 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 +222,7 @@ struct MemoryRegion { > > const char *name; > > unsigned ioeventfd_nb; > > MemoryRegionIoeventfd *ioeventfds; > > - NotifierList iommu_notify; > > + QLIST_HEAD(, IOMMUNotifier) iommu_notify; > > }; > > > > /** > > @@ -620,11 +641,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 +658,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 +669,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..f65c600 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_flags != 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; > > + IOMMUNotifierFlag request_flags; > > + > > assert(memory_region_is_iommu(mr)); > > - notifier_list_notify(&mr->iommu_notify, &entry); > > + > > + if (entry.perm & IOMMU_RW) { > > + request_flags = IOMMU_NOTIFIER_MAP; > > + } else { > > + request_flags = IOMMU_NOTIFIER_UNMAP; > > + } > > This is still wrong. UNMAP depends on the *previous* state of the > mapping, not the new state. Peter pointed out to be on IRC that VFIO already assumes that it's only an unmap if the new permissions are NONE. So one can argue that it's an existing constraint of the IOMMUTLBEntry interface that a mapping can only ever transition from valid->invalid or invalid->valid. Changing one valid entry to another would require two notifications one switching it to a blank entry with NONE permissions, then another notifying the new valid mapping. Assuming that constraint, Peter's patch is correct. I'm pretty uneasy about that constraint, because it's not necessarily obvious to someone implementing a new vIOMMU device, which is responsible for triggering the notifies. From just the callback, it looks like it should be fine to just fire the notify with the new mapping which replaced the old. Peter suggested commenting this next to the IOTLBEntry definition, and I think that's probably ok for now. I do think we should consider changing the notify interface to make this more obvious. I can see one of two ways to do that: * Fully allow in-place changes to be notified - the callback would need to be passed both the new entry and at least the old permissions, if not the old entry. * Instead have separate map and unmap notifier chains with separate callbacks. That should make it obvious to a vIOMMU author that an in-place change would need first an unmap notify, then a map notify. > > > + > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > + if (iommu_notifier->notifier_flags & request_flags) { > > + iommu_notifier->notify(iommu_notifier, &entry); > > + } > > + } > > } > > > > void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) > -- 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-14 7:15 ` David Gibson @ 2016-09-14 7:40 ` Peter Xu 2016-09-14 10:47 ` David Gibson 2016-09-14 8:17 ` Peter Xu 1 sibling, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-14 7:40 UTC (permalink / raw) To: David Gibson Cc: mst, jasowang, qemu-devel, cornelia.huck, alex.williamson, wexu, dgibson, vkaplans, pbonzini On Wed, Sep 14, 2016 at 05:15:03PM +1000, David Gibson wrote: [...] > > > @@ -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; > > > + IOMMUNotifierFlag request_flags; > > > + > > > assert(memory_region_is_iommu(mr)); > > > - notifier_list_notify(&mr->iommu_notify, &entry); > > > + > > > + if (entry.perm & IOMMU_RW) { > > > + request_flags = IOMMU_NOTIFIER_MAP; > > > + } else { > > > + request_flags = IOMMU_NOTIFIER_UNMAP; > > > + } > > > > This is still wrong. UNMAP depends on the *previous* state of the > > mapping, not the new state. > > Peter pointed out to be on IRC that VFIO already assumes that it's > only an unmap if the new permissions are NONE. So one can argue that > it's an existing constraint of the IOMMUTLBEntry interface that a > mapping can only ever transition from valid->invalid or > invalid->valid. Changing one valid entry to another would require two > notifications one switching it to a blank entry with NONE permissions, > then another notifying the new valid mapping. > > Assuming that constraint, Peter's patch is correct. > > I'm pretty uneasy about that constraint, because it's not necessarily > obvious to someone implementing a new vIOMMU device, which is > responsible for triggering the notifies. From just the callback, it > looks like it should be fine to just fire the notify with the new > mapping which replaced the old. > > Peter suggested commenting this next to the IOTLBEntry definition, and > I think that's probably ok for now. I do think we should consider > changing the notify interface to make this more obvious. I can see > one of two ways to do that: > > * Fully allow in-place changes to be notified - the callback would > need to be passed both the new entry and at least the old > permissions, if not the old entry. > > * Instead have separate map and unmap notifier chains with > separate callbacks. That should make it obvious to a vIOMMU > author that an in-place change would need first an unmap > notify, then a map notify. Thanks for the summary! Since we are at this... I am still curious about when we will need this CHANGE interface. Not to talk about Linux kernel, yes we can have other guest OS running. However for any guests, IMHO changing IOMMU PTE is extremely dangerous. For example, if we have mapped an area of memory, e.g. three DMA pages, each 4K (which really doesn't matter): page1 (0-4k) page2 (4k-8k) page3 (8k-12k) If we want to modify the 12K mapping (e.g., change in-place from page1 to page3 in order), the result can be undefined. Since IOMMU might still be using these page mappings during the modification. The problem is that, we cannot do this change for the three pages in an atomic operation. So if IOMMU uses these pages during the modification (e.g., CPU just changed page1, but not yet for page2 and page3), IOMMU will see an inconsistent view of memory. That's trouble. I guess this is why Linux is using unmap_page() and map_page() for it? Or say, we just do not allow to change the content of it directly. Not sure. Also, I assume we may need something like "quiesce" IOMMU operation (or not operation, but existing procedures?) to finally make sure that the pages we are removing will never be touched by IOMMU any more before freeing them. All above is wild guess of me, just want to know when and why we will need this CHANGE stuff. And before we finally realize we need this, I would still suggest to keep the old interface (as long as it can work for us, no extra effort needed), and as David has mentioned, we can add comment for IOMMUTLBEntry to make sure people can know its meaning easier (before starting to read vfio_iommu_map_notify() codes). Thanks, -- peterx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-14 7:40 ` Peter Xu @ 2016-09-14 10:47 ` David Gibson 0 siblings, 0 replies; 19+ messages in thread From: David Gibson @ 2016-09-14 10:47 UTC (permalink / raw) To: Peter Xu Cc: mst, jasowang, qemu-devel, cornelia.huck, alex.williamson, wexu, dgibson, vkaplans, pbonzini [-- Attachment #1: Type: text/plain, Size: 5637 bytes --] On Wed, Sep 14, 2016 at 03:40:08PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 05:15:03PM +1000, David Gibson wrote: > > [...] > > > > > @@ -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; > > > > + IOMMUNotifierFlag request_flags; > > > > + > > > > assert(memory_region_is_iommu(mr)); > > > > - notifier_list_notify(&mr->iommu_notify, &entry); > > > > + > > > > + if (entry.perm & IOMMU_RW) { > > > > + request_flags = IOMMU_NOTIFIER_MAP; > > > > + } else { > > > > + request_flags = IOMMU_NOTIFIER_UNMAP; > > > > + } > > > > > > This is still wrong. UNMAP depends on the *previous* state of the > > > mapping, not the new state. > > > > Peter pointed out to be on IRC that VFIO already assumes that it's > > only an unmap if the new permissions are NONE. So one can argue that > > it's an existing constraint of the IOMMUTLBEntry interface that a > > mapping can only ever transition from valid->invalid or > > invalid->valid. Changing one valid entry to another would require two > > notifications one switching it to a blank entry with NONE permissions, > > then another notifying the new valid mapping. > > > > Assuming that constraint, Peter's patch is correct. > > > > I'm pretty uneasy about that constraint, because it's not necessarily > > obvious to someone implementing a new vIOMMU device, which is > > responsible for triggering the notifies. From just the callback, it > > looks like it should be fine to just fire the notify with the new > > mapping which replaced the old. > > > > Peter suggested commenting this next to the IOTLBEntry definition, and > > I think that's probably ok for now. I do think we should consider > > changing the notify interface to make this more obvious. I can see > > one of two ways to do that: > > > > * Fully allow in-place changes to be notified - the callback would > > need to be passed both the new entry and at least the old > > permissions, if not the old entry. > > > > * Instead have separate map and unmap notifier chains with > > separate callbacks. That should make it obvious to a vIOMMU > > author that an in-place change would need first an unmap > > notify, then a map notify. > > Thanks for the summary! > > Since we are at this... I am still curious about when we will need > this CHANGE interface. It doesn't require any new interface, just removal of a subtle constraint on the current one. And we don't, strictly speaking, need it. However, leaving subtle constraints in how you can use an interface that aren't obvious from the interface itself, and can't be verified by the interface is just leaving booby traps for future developers. > Not to talk about Linux kernel, yes we can have other guest OS > running. However for any guests, IMHO changing IOMMU PTE is extremely > dangerous. For example, if we have mapped an area of memory, e.g. > three DMA pages, each 4K (which really doesn't matter): > > page1 (0-4k) > page2 (4k-8k) > page3 (8k-12k) > > If we want to modify the 12K mapping (e.g., change in-place from page1 > to page3 in order), the result can be undefined. Since IOMMU might > still be using these page mappings during the modification. The > problem is that, we cannot do this change for the three pages in an > atomic operation. So if IOMMU uses these pages during the modification > (e.g., CPU just changed page1, but not yet for page2 and page3), IOMMU > will see an inconsistent view of memory. That's trouble. Yes, changing the IOMMU mappings will require synchronization with device drivers and hardware, but what that involves is between the guest and the vIOMMU implementation. Having "simultaneous" unmap/map is a somewhat unlikely in pratice, I'll grant, but it's absolutely possible that a vIOMMU could do this safely. I could even imagine it happening if the guest always maps and unmaps separately, if the vIOMMU did some sort of operation batching. > I guess this is why Linux is using unmap_page() and map_page() for it? > Or say, we just do not allow to change the content of it directly. Not > sure. I'm not sure what you mean by that. Sounds kind of like my option 2 above - only allow separate maps and unmaps, but change the notify interface to make that more obvious to the vIOMMU implementor. > Also, I assume we may need something like "quiesce" IOMMU > operation (or not operation, but existing procedures?) to finally make > sure that the pages we are removing will never be touched by IOMMU any > more before freeing them. Yes.. I'm not sure if you're thinking of the guest side or host side here. > All above is wild guess of me, just want to know when and why we will > need this CHANGE stuff. > > And before we finally realize we need this, I would still suggest to > keep the old interface (as long as it can work for us, no extra effort > needed), and as David has mentioned, we can add comment for > IOMMUTLBEntry to make sure people can know its meaning easier (before > starting to read vfio_iommu_map_notify() codes). > > 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-14 7:15 ` David Gibson 2016-09-14 7:40 ` Peter Xu @ 2016-09-14 8:17 ` Peter Xu 2016-09-14 10:50 ` David Gibson 1 sibling, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-14 8:17 UTC (permalink / raw) To: David Gibson Cc: mst, jasowang, qemu-devel, cornelia.huck, alex.williamson, wexu, dgibson, vkaplans, pbonzini On Wed, Sep 14, 2016 at 05:15:03PM +1000, David Gibson wrote: [...] > Peter suggested commenting this next to the IOTLBEntry definition, and > I think that's probably ok for now. Looks like we have something already (just not that obvious): /** * memory_region_notify_iommu: notify a change in an IOMMU translation entry. * * @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. * Deleted entries have .@perm == 0. */ void memory_region_notify_iommu(MemoryRegion *mr, IOMMUTLBEntry entry); Though it's quite simple, it did explain that perm==0 is for deleted entries. Thanks, -- peterx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps 2016-09-14 8:17 ` Peter Xu @ 2016-09-14 10:50 ` David Gibson 0 siblings, 0 replies; 19+ messages in thread From: David Gibson @ 2016-09-14 10:50 UTC (permalink / raw) To: Peter Xu Cc: mst, jasowang, qemu-devel, cornelia.huck, alex.williamson, wexu, dgibson, vkaplans, pbonzini [-- Attachment #1: Type: text/plain, Size: 1363 bytes --] On Wed, Sep 14, 2016 at 04:17:26PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 05:15:03PM +1000, David Gibson wrote: > > [...] > > > Peter suggested commenting this next to the IOTLBEntry definition, and > > I think that's probably ok for now. > > Looks like we have something already (just not that obvious): > > /** > * memory_region_notify_iommu: notify a change in an IOMMU translation entry. > * > * @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. > * Deleted entries have .@perm == 0. > */ > void memory_region_notify_iommu(MemoryRegion *mr, > IOMMUTLBEntry entry); > > Though it's quite simple, it did explain that perm==0 is for deleted > entries. That is definitely not sufficient. It misses the crucial point is that @perm != 0 is NOT ALLOWED if there was an existing mapping at that address. It's not that we really _need_ to support in-place change. The point is that we want to keep our interface contracts simple and clear. -- 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-09 2:57 [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct Peter Xu 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps Peter Xu @ 2016-09-09 2:57 ` Peter Xu 2016-09-14 5:55 ` David Gibson 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers Peter Xu 2016-09-09 4:05 ` [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct no-reply 3 siblings, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-09 2:57 UTC (permalink / raw) To: qemu-devel Cc: dgibson, jasowang, mst, pbonzini, cornelia.huck, wexu, vkaplans, alex.williamson, peterx The new interface can be used to replace the old notify_started() and notify_stopped(). Meanwhile it provides explicit flags so that IOMMUs can know what kind of notifications it is requested for. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 6 ++++-- hw/ppc/spapr_iommu.c | 14 ++++++++++++-- include/exec/memory.h | 9 +++++---- memory.c | 29 +++++++++++++++++++++-------- 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 28c31a2..9d49be7 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1974,7 +1974,9 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr, return ret; } -static void vtd_iommu_notify_started(MemoryRegion *iommu) +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new) { VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); @@ -2348,7 +2350,7 @@ static void vtd_init(IntelIOMMUState *s) memset(s->womask, 0, DMAR_REG_SIZE); s->iommu_ops.translate = vtd_iommu_translate; - s->iommu_ops.notify_started = vtd_iommu_notify_started; + s->iommu_ops.notify_flag_changed = vtd_iommu_notify_flag_changed; s->root = 0; s->root_extended = false; s->dmar_enabled = false; diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 6bc4d4d..313f53f 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -166,6 +166,17 @@ static void spapr_tce_notify_stopped(MemoryRegion *iommu) spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false); } +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new) +{ + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { + spapr_tce_notify_started(iommu); + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { + spapr_tce_notify_stopped(iommu); + } +} + static int spapr_tce_table_post_load(void *opaque, int version_id) { sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); @@ -246,8 +257,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { static MemoryRegionIOMMUOps spapr_iommu_ops = { .translate = spapr_tce_translate_iommu, .get_min_page_size = spapr_tce_get_min_page_size, - .notify_started = spapr_tce_notify_started, - .notify_stopped = spapr_tce_notify_stopped, + .notify_flag_changed = spapr_tce_notify_flag_changed, }; static int spapr_tce_table_realize(DeviceState *dev) diff --git a/include/exec/memory.h b/include/exec/memory.h index e69e984..e04b752 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -174,10 +174,10 @@ struct MemoryRegionIOMMUOps { IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write); /* Returns minimum supported page size */ uint64_t (*get_min_page_size)(MemoryRegion *iommu); - /* Called when the first notifier is set */ - void (*notify_started)(MemoryRegion *iommu); - /* Called when the last notifier is removed */ - void (*notify_stopped)(MemoryRegion *iommu); + /* Called when IOMMU Notifier flag changed */ + void (*notify_flag_changed)(MemoryRegion *iommu, + IOMMUNotifierFlag old_flags, + IOMMUNotifierFlag new_flags); }; typedef struct CoalescedMemoryRange CoalescedMemoryRange; @@ -223,6 +223,7 @@ struct MemoryRegion { unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; QLIST_HEAD(, IOMMUNotifier) iommu_notify; + IOMMUNotifierFlag iommu_notify_flags; }; /** diff --git a/memory.c b/memory.c index f65c600..4faf1d9 100644 --- a/memory.c +++ b/memory.c @@ -1419,6 +1419,7 @@ void memory_region_init_iommu(MemoryRegion *mr, mr->iommu_ops = ops, mr->terminates = true; /* then re-forwards */ QLIST_INIT(&mr->iommu_notify); + mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE; } static void memory_region_finalize(Object *obj) @@ -1513,16 +1514,31 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) return memory_region_get_dirty_log_mask(mr) & (1 << client); } +static void memory_region_update_iommu_notify_flags(MemoryRegion *mr) +{ + IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE; + IOMMUNotifier *iommu_notifier; + + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { + flags |= iommu_notifier->notifier_flags; + } + + if (flags != mr->iommu_notify_flags && + mr->iommu_ops->notify_flag_changed) { + mr->iommu_ops->notify_flag_changed(mr, mr->iommu_notify_flags, + flags); + } + + mr->iommu_notify_flags = flags; +} + void memory_region_register_iommu_notifier(MemoryRegion *mr, IOMMUNotifier *n) { /* We need to register for at least one bitfield */ assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); - if (mr->iommu_ops->notify_started && - QLIST_EMPTY(&mr->iommu_notify)) { - mr->iommu_ops->notify_started(mr); - } QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); + memory_region_update_iommu_notify_flags(mr); } uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) @@ -1560,10 +1576,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, IOMMUNotifier *n) { QLIST_REMOVE(n, node); - if (mr->iommu_ops->notify_stopped && - QLIST_EMPTY(&mr->iommu_notify)) { - mr->iommu_ops->notify_stopped(mr); - } + memory_region_update_iommu_notify_flags(mr); } void memory_region_notify_iommu(MemoryRegion *mr, -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed Peter Xu @ 2016-09-14 5:55 ` David Gibson 2016-09-14 7:12 ` Peter Xu 0 siblings, 1 reply; 19+ messages in thread From: David Gibson @ 2016-09-14 5:55 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson [-- Attachment #1: Type: text/plain, Size: 6948 bytes --] On Fri, Sep 09, 2016 at 10:57:43AM +0800, Peter Xu wrote: > The new interface can be used to replace the old notify_started() and > notify_stopped(). Meanwhile it provides explicit flags so that IOMMUs > can know what kind of notifications it is requested for. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/intel_iommu.c | 6 ++++-- > hw/ppc/spapr_iommu.c | 14 ++++++++++++-- > include/exec/memory.h | 9 +++++---- > memory.c | 29 +++++++++++++++++++++-------- > 4 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 28c31a2..9d49be7 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1974,7 +1974,9 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr, > return ret; > } > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > + IOMMUNotifierFlag old, > + IOMMUNotifierFlag new) > { > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); Shouldn't this have a sanity check that the new flags doesn't include MAP actions? > @@ -2348,7 +2350,7 @@ static void vtd_init(IntelIOMMUState *s) > memset(s->womask, 0, DMAR_REG_SIZE); > > s->iommu_ops.translate = vtd_iommu_translate; > - s->iommu_ops.notify_started = vtd_iommu_notify_started; > + s->iommu_ops.notify_flag_changed = vtd_iommu_notify_flag_changed; > s->root = 0; > s->root_extended = false; > s->dmar_enabled = false; > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 6bc4d4d..313f53f 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -166,6 +166,17 @@ static void spapr_tce_notify_stopped(MemoryRegion *iommu) > spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false); > } > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > + IOMMUNotifierFlag old, > + IOMMUNotifierFlag new) > +{ > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > + spapr_tce_notify_started(iommu); > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > + spapr_tce_notify_stopped(iommu); > + } This is wrong. We need to do the notify_start and stop actions if *any* bits are set in the new/old flags, not just if all of them are set. I'd also prefer to see notify_started and notify_stopped folded into this function. > +} > + > static int spapr_tce_table_post_load(void *opaque, int version_id) > { > sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); > @@ -246,8 +257,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { > static MemoryRegionIOMMUOps spapr_iommu_ops = { > .translate = spapr_tce_translate_iommu, > .get_min_page_size = spapr_tce_get_min_page_size, > - .notify_started = spapr_tce_notify_started, > - .notify_stopped = spapr_tce_notify_stopped, > + .notify_flag_changed = spapr_tce_notify_flag_changed, > }; > > static int spapr_tce_table_realize(DeviceState *dev) > diff --git a/include/exec/memory.h b/include/exec/memory.h > index e69e984..e04b752 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -174,10 +174,10 @@ struct MemoryRegionIOMMUOps { > IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write); > /* Returns minimum supported page size */ > uint64_t (*get_min_page_size)(MemoryRegion *iommu); > - /* Called when the first notifier is set */ > - void (*notify_started)(MemoryRegion *iommu); > - /* Called when the last notifier is removed */ > - void (*notify_stopped)(MemoryRegion *iommu); > + /* Called when IOMMU Notifier flag changed */ > + void (*notify_flag_changed)(MemoryRegion *iommu, > + IOMMUNotifierFlag old_flags, > + IOMMUNotifierFlag new_flags); > }; > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -223,6 +223,7 @@ struct MemoryRegion { > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > QLIST_HEAD(, IOMMUNotifier) iommu_notify; > + IOMMUNotifierFlag iommu_notify_flags; > }; > > /** > diff --git a/memory.c b/memory.c > index f65c600..4faf1d9 100644 > --- a/memory.c > +++ b/memory.c > @@ -1419,6 +1419,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > mr->iommu_ops = ops, > mr->terminates = true; /* then re-forwards */ > QLIST_INIT(&mr->iommu_notify); > + mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE; > } > > static void memory_region_finalize(Object *obj) > @@ -1513,16 +1514,31 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) > return memory_region_get_dirty_log_mask(mr) & (1 << client); > } > > +static void memory_region_update_iommu_notify_flags(MemoryRegion *mr) > +{ > + IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE; > + IOMMUNotifier *iommu_notifier; > + > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + flags |= iommu_notifier->notifier_flags; > + } > + > + if (flags != mr->iommu_notify_flags && > + mr->iommu_ops->notify_flag_changed) { > + mr->iommu_ops->notify_flag_changed(mr, mr->iommu_notify_flags, > + flags); > + } > + > + mr->iommu_notify_flags = flags; > +} > + > void memory_region_register_iommu_notifier(MemoryRegion *mr, > IOMMUNotifier *n) > { > /* We need to register for at least one bitfield */ > assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); > - if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_started(mr); > - } > QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > + memory_region_update_iommu_notify_flags(mr); > } > > uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) > @@ -1560,10 +1576,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > IOMMUNotifier *n) > { > QLIST_REMOVE(n, node); > - if (mr->iommu_ops->notify_stopped && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_stopped(mr); > - } > + memory_region_update_iommu_notify_flags(mr); > } > > void memory_region_notify_iommu(MemoryRegion *mr, -- 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-14 5:55 ` David Gibson @ 2016-09-14 7:12 ` Peter Xu 2016-09-14 7:22 ` David Gibson 0 siblings, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-14 7:12 UTC (permalink / raw) To: David Gibson Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: [...] > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > + IOMMUNotifierFlag old, > > + IOMMUNotifierFlag new) > > { > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > Shouldn't this have a sanity check that the new flags doesn't include > MAP actions? See your r-b for patch 3, thanks! So skipping this one. [...] > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > + IOMMUNotifierFlag old, > > + IOMMUNotifierFlag new) > > +{ > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > + spapr_tce_notify_started(iommu); > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > + spapr_tce_notify_stopped(iommu); > > + } > > This is wrong. We need to do the notify_start and stop actions if > *any* bits are set in the new/old flags, not just if all of them are > set. Power should need both, right? I can switch all "== IOMMU_NOTIFIER_ALL" into: "!= IOMMU_NOTIFIER_NONE" in the next version if you like, but AFAICT they are totally the same. > > I'd also prefer to see notify_started and notify_stopped folded into > this function. I can do that for v5. Thanks, -- peterx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-14 7:12 ` Peter Xu @ 2016-09-14 7:22 ` David Gibson 2016-09-14 7:49 ` Peter Xu 0 siblings, 1 reply; 19+ messages in thread From: David Gibson @ 2016-09-14 7:22 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson [-- Attachment #1: Type: text/plain, Size: 2094 bytes --] On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > [...] > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > + IOMMUNotifierFlag old, > > > + IOMMUNotifierFlag new) > > > { > > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > > > Shouldn't this have a sanity check that the new flags doesn't include > > MAP actions? > > See your r-b for patch 3, thanks! So skipping this one. > > [...] > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > + IOMMUNotifierFlag old, > > > + IOMMUNotifierFlag new) > > > +{ > > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > > + spapr_tce_notify_started(iommu); > > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > > + spapr_tce_notify_stopped(iommu); > > > + } > > > > This is wrong. We need to do the notify_start and stop actions if > > *any* bits are set in the new/old flags, not just if all of them are > > set. > > Power should need both, right? I can switch all > > "== IOMMU_NOTIFIER_ALL" > > into: > > "!= IOMMU_NOTIFIER_NONE" Yes, that should be right. > in the next version if you like, but AFAICT they are totally the > same. Again, only if you assume things about how the notifiers will be used, which is not a good look when designing an interface. > > > > > I'd also prefer to see notify_started and notify_stopped folded into > > this function. > > I can do that for v5. > > 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-14 7:22 ` David Gibson @ 2016-09-14 7:49 ` Peter Xu 2016-09-14 10:37 ` David Gibson 0 siblings, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-14 7:49 UTC (permalink / raw) To: David Gibson Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson On Wed, Sep 14, 2016 at 05:22:40PM +1000, David Gibson wrote: > On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > > > [...] > > > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > > + IOMMUNotifierFlag old, > > > > + IOMMUNotifierFlag new) > > > > { > > > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > > > > > Shouldn't this have a sanity check that the new flags doesn't include > > > MAP actions? > > > > See your r-b for patch 3, thanks! So skipping this one. > > > > [...] > > > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > > + IOMMUNotifierFlag old, > > > > + IOMMUNotifierFlag new) > > > > +{ > > > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > > > + spapr_tce_notify_started(iommu); > > > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > > > + spapr_tce_notify_stopped(iommu); > > > > + } > > > > > > This is wrong. We need to do the notify_start and stop actions if > > > *any* bits are set in the new/old flags, not just if all of them are > > > set. > > > > Power should need both, right? I can switch all > > > > "== IOMMU_NOTIFIER_ALL" > > > > into: > > > > "!= IOMMU_NOTIFIER_NONE" > > Yes, that should be right. > > > in the next version if you like, but AFAICT they are totally the > > same. > > Again, only if you assume things about how the notifiers will be used, > which is not a good look when designing an interface. This should not be related to the interface at all? I was based on the assumption that "Power cannot support either one of MAP/UNMAP, but only if both exist". To be more specific, we possibly can have this at the beginning of flags_changed(): assert(old == IOMMU_NOTIFIER_NONE || old == IOMMU_NOTIFIER_ALL); assert(new == IOMMU_NOTIFIER_NONE || new == IOMMU_NOTIFIER_ALL); To make sure nothing will go wrong. Thanks, -- peterx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-14 7:49 ` Peter Xu @ 2016-09-14 10:37 ` David Gibson 2016-09-14 11:25 ` Peter Xu 0 siblings, 1 reply; 19+ messages in thread From: David Gibson @ 2016-09-14 10:37 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson [-- Attachment #1: Type: text/plain, Size: 3210 bytes --] On Wed, Sep 14, 2016 at 03:49:41PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 05:22:40PM +1000, David Gibson wrote: > > On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > > > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > > > > > [...] > > > > > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > > > + IOMMUNotifierFlag old, > > > > > + IOMMUNotifierFlag new) > > > > > { > > > > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > > > > > > > Shouldn't this have a sanity check that the new flags doesn't include > > > > MAP actions? > > > > > > See your r-b for patch 3, thanks! So skipping this one. > > > > > > [...] > > > > > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > > > + IOMMUNotifierFlag old, > > > > > + IOMMUNotifierFlag new) > > > > > +{ > > > > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > > > > + spapr_tce_notify_started(iommu); > > > > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > > > > + spapr_tce_notify_stopped(iommu); > > > > > + } > > > > > > > > This is wrong. We need to do the notify_start and stop actions if > > > > *any* bits are set in the new/old flags, not just if all of them are > > > > set. > > > > > > Power should need both, right? I can switch all > > > > > > "== IOMMU_NOTIFIER_ALL" > > > > > > into: > > > > > > "!= IOMMU_NOTIFIER_NONE" > > > > Yes, that should be right. > > > > > in the next version if you like, but AFAICT they are totally the > > > same. > > > > Again, only if you assume things about how the notifiers will be used, > > which is not a good look when designing an interface. > > This should not be related to the interface at all? > > I was based on the assumption that "Power cannot support either one of > MAP/UNMAP, but only if both exist". Huh? I have no idea what you mean by that. Power can support notifying both map and unmap events just fine - but in order to provide *any* notifications, it has to disable KVM acceleration of the guest-side IOMMU (otherwise qemu won't know about any changes to the IOMMU state). So the change you you suggested before to != IOMMU_NOTIFIER_NONE is exactly correct, anything else is not. > To be more specific, we possibly > can have this at the beginning of flags_changed(): > > assert(old == IOMMU_NOTIFIER_NONE || old == IOMMU_NOTIFIER_ALL); > assert(new == IOMMU_NOTIFIER_NONE || new == IOMMU_NOTIFIER_ALL); > > To make sure nothing will go wrong. Hmm.. I really get the feeling you're confusing constraints of the guest side with constraints of the host side. -- 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed 2016-09-14 10:37 ` David Gibson @ 2016-09-14 11:25 ` Peter Xu 0 siblings, 0 replies; 19+ messages in thread From: Peter Xu @ 2016-09-14 11:25 UTC (permalink / raw) To: David Gibson Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson On Wed, Sep 14, 2016 at 08:37:34PM +1000, David Gibson wrote: [...] > > This should not be related to the interface at all? > > > > I was based on the assumption that "Power cannot support either one of > > MAP/UNMAP, but only if both exist". > > Huh? I have no idea what you mean by that. > > Power can support notifying both map and unmap events just fine - but > in order to provide *any* notifications, it has to disable KVM > acceleration of the guest-side IOMMU (otherwise qemu won't know about > any changes to the IOMMU state). > > So the change you you suggested before to != IOMMU_NOTIFIER_NONE is > exactly correct, anything else is not. Sorry I was wrong. Yes user can register with only one (MAP or UNMAP) notifier type for any platform. Thanks. -- peterx ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers 2016-09-09 2:57 [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct Peter Xu 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps Peter Xu 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed Peter Xu @ 2016-09-09 2:57 ` Peter Xu 2016-09-14 5:56 ` David Gibson 2016-09-09 4:05 ` [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct no-reply 3 siblings, 1 reply; 19+ messages in thread From: Peter Xu @ 2016-09-09 2:57 UTC (permalink / raw) To: qemu-devel Cc: dgibson, jasowang, mst, pbonzini, cornelia.huck, wexu, vkaplans, alex.williamson, peterx Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism. Before that is achieved, let's open a door for vhost DMAR support, which only requires cache invalidations (UNMAP operations). Meanwhile, converting hw_error() to error_report() and exit(1), to make the error messages clean and obvious (so no CPU registers will be dumped). Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 9d49be7..e4c3681 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1980,10 +1980,14 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, { VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); - hw_error("Device at bus %s addr %02x.%d requires iommu notifier which " - "is currently not supported by intel-iommu emulation", - vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn), - PCI_FUNC(vtd_as->devfn)); + if (new & IOMMU_NOTIFIER_MAP) { + error_report("Device at bus %s addr %02x.%d requires iommu " + "notifier which is currently not supported by " + "intel-iommu emulation", + vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn), + PCI_FUNC(vtd_as->devfn)); + exit(1); + } } static const VMStateDescription vtd_vmstate = { -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers Peter Xu @ 2016-09-14 5:56 ` David Gibson 0 siblings, 0 replies; 19+ messages in thread From: David Gibson @ 2016-09-14 5:56 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson [-- Attachment #1: Type: text/plain, Size: 2027 bytes --] On Fri, Sep 09, 2016 at 10:57:44AM +0800, Peter Xu wrote: > Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism. > Before that is achieved, let's open a door for vhost DMAR support, which > only requires cache invalidations (UNMAP operations). > > Meanwhile, converting hw_error() to error_report() and exit(1), to make > the error messages clean and obvious (so no CPU registers will be > dumped). Ah, you can scratch the comment I made about this in the previous patch, I hadn't realised this just threw an error at present. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/i386/intel_iommu.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 9d49be7..e4c3681 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1980,10 +1980,14 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > { > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > - hw_error("Device at bus %s addr %02x.%d requires iommu notifier which " > - "is currently not supported by intel-iommu emulation", > - vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn), > - PCI_FUNC(vtd_as->devfn)); > + if (new & IOMMU_NOTIFIER_MAP) { > + error_report("Device at bus %s addr %02x.%d requires iommu " > + "notifier which is currently not supported by " > + "intel-iommu emulation", > + vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn), > + PCI_FUNC(vtd_as->devfn)); > + exit(1); > + } > } > > static const VMStateDescription vtd_vmstate = { -- 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: 801 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct 2016-09-09 2:57 [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct Peter Xu ` (2 preceding siblings ...) 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers Peter Xu @ 2016-09-09 4:05 ` no-reply 2016-09-09 6:41 ` Peter Xu 3 siblings, 1 reply; 19+ messages in thread From: no-reply @ 2016-09-09 4:05 UTC (permalink / raw) To: peterx Cc: famz, qemu-devel, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson Hi, Your series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Type: series Message-id: 1473389864-19694-1-git-send-email-peterx@redhat.com Subject: [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc make J=8 docker-test-quick@centos6 make J=8 docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1473389864-19694-1-git-send-email-peterx@redhat.com -> patchew/1473389864-19694-1-git-send-email-peterx@redhat.com Switched to a new branch 'test' 639dbf6 intel_iommu: allow UNMAP notifiers 16bcaf3 memory: introduce IOMMUOps.notify_flag_changed 063de83 memory: introduce IOMMUNotifier and its caps === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into 'dtc'... Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf' BUILD centos6 ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPY RUNNER RUN test-quick in centos6 === OUTPUT END === Abort: command timeout (>3600 seconds) --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct 2016-09-09 4:05 ` [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct no-reply @ 2016-09-09 6:41 ` Peter Xu 0 siblings, 0 replies; 19+ messages in thread From: Peter Xu @ 2016-09-09 6:41 UTC (permalink / raw) To: qemu-devel Cc: famz, mst, jasowang, vkaplans, alex.williamson, wexu, pbonzini, cornelia.huck, dgibson On Thu, Sep 08, 2016 at 09:05:43PM -0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote: > Hi, > > Your series failed automatic build test. Please find the testing commands and > their output below. If you have docker installed, you can probably reproduce it > locally. > > Type: series > Message-id: 1473389864-19694-1-git-send-email-peterx@redhat.com > Subject: [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct > > === TEST SCRIPT BEGIN === > #!/bin/bash > set -e > git submodule update --init dtc > make J=8 docker-test-quick@centos6 > make J=8 docker-test-mingw@fedora > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > * [new tag] patchew/1473389864-19694-1-git-send-email-peterx@redhat.com -> patchew/1473389864-19694-1-git-send-email-peterx@redhat.com > Switched to a new branch 'test' > 639dbf6 intel_iommu: allow UNMAP notifiers > 16bcaf3 memory: introduce IOMMUOps.notify_flag_changed > 063de83 memory: introduce IOMMUNotifier and its caps > > === OUTPUT BEGIN === > Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' > Cloning into 'dtc'... > Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf' > BUILD centos6 > ARCHIVE qemu.tgz > ARCHIVE dtc.tgz > COPY RUNNER > RUN test-quick in centos6 > === OUTPUT END === > > Abort: command timeout (>3600 seconds) docker quick test for centos6 worked on my laptop (and Fam's as well). So I suppose this is a false positive. Thanks. -- peterx ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-09-14 11:25 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-09 2:57 [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct Peter Xu 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps Peter Xu 2016-09-14 5:48 ` David Gibson 2016-09-14 7:15 ` David Gibson 2016-09-14 7:40 ` Peter Xu 2016-09-14 10:47 ` David Gibson 2016-09-14 8:17 ` Peter Xu 2016-09-14 10:50 ` David Gibson 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed Peter Xu 2016-09-14 5:55 ` David Gibson 2016-09-14 7:12 ` Peter Xu 2016-09-14 7:22 ` David Gibson 2016-09-14 7:49 ` Peter Xu 2016-09-14 10:37 ` David Gibson 2016-09-14 11:25 ` Peter Xu 2016-09-09 2:57 ` [Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers Peter Xu 2016-09-14 5:56 ` David Gibson 2016-09-09 4:05 ` [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct no-reply 2016-09-09 6:41 ` Peter Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.