| Message ID | 20170113092345.GX4450@pxdev.xzpeter.org |
|---|---|
| State | New |
| Headers | show |
On 2017年01月13日 17:23, Peter Xu wrote: > On Fri, Jan 13, 2017 at 03:55:22PM +0800, Jason Wang wrote: >> >> On 2017年01月13日 11:06, Peter Xu wrote: >>> In this patch, IOMMUNotifier.{start|end} are introduced to store section >>> information for a specific notifier. When notification occurs, we not >>> only check the notification type (MAP|UNMAP), but also check whether the >>> notified iova is in the range of specific IOMMU notifier, and skip those >>> notifiers if not in the listened range. >>> >>> When removing an region, we need to make sure we removed the correct >>> VFIOGuestIOMMU by checking the IOMMUNotifier.start address as well. >>> >>> Suggested-by: David Gibson <david@gibson.dropbear.id.au> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> hw/vfio/common.c | 7 ++++++- >>> include/exec/memory.h | 3 +++ >>> memory.c | 4 +++- >>> 3 files changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 801578b..6f648da 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -455,6 +455,10 @@ static void vfio_listener_region_add(MemoryListener *listener, >>> giommu->container = container; >>> giommu->n.notify = vfio_iommu_map_notify; >>> giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; >>> + giommu->n.start = section->offset_within_region; >>> + llend = int128_add(int128_make64(giommu->n.start), section->size); >>> + llend = int128_sub(llend, int128_one()); >>> + giommu->n.end = int128_get64(llend); >>> QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); >>> memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); >>> @@ -525,7 +529,8 @@ static void vfio_listener_region_del(MemoryListener *listener, >>> VFIOGuestIOMMU *giommu; >>> QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) { >>> - if (giommu->iommu == section->mr) { >>> + if (giommu->iommu == section->mr && >>> + giommu->n.start == section->offset_within_region) { >>> memory_region_unregister_iommu_notifier(giommu->iommu, >>> &giommu->n); >>> QLIST_REMOVE(giommu, giommu_next); >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index bec9756..7649e74 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -84,6 +84,9 @@ typedef enum { >>> struct IOMMUNotifier { >>> void (*notify)(struct IOMMUNotifier *notifier, IOMMUTLBEntry *data); >>> IOMMUNotifierFlag notifier_flags; >>> + /* Notify for address space range start <= addr <= end */ >>> + hwaddr start; >>> + hwaddr end; >>> QLIST_ENTRY(IOMMUNotifier) node; >>> }; >>> typedef struct IOMMUNotifier IOMMUNotifier; >>> diff --git a/memory.c b/memory.c >>> index 2bfc37f..e88bb54 100644 >>> --- a/memory.c >>> +++ b/memory.c >>> @@ -1671,7 +1671,9 @@ void memory_region_notify_iommu(MemoryRegion *mr, >>> } >>> QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { >>> - if (iommu_notifier->notifier_flags & request_flags) { >>> + if (iommu_notifier->notifier_flags & request_flags && >>> + iommu_notifier->start <= entry.iova && >>> + iommu_notifier->end >= entry.iova) { >>> iommu_notifier->notify(iommu_notifier, &entry); >>> } >>> } >> This seems breaks vhost device IOTLB. How about keep the the behavior >> somehow? > Thanks to point out. How about I squash this into this patch? > > --------8<-------- > diff --git a/memory.c b/memory.c > index e88bb54..6de02dd 100644 > --- a/memory.c > +++ b/memory.c > @@ -1608,8 +1608,14 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, > return; > } > > + if (n->start == 0 && n->end == 0) { > + /* If these are not specified, we listen to the whole range */ > + n->end = (hwaddr)(-1); > + } > + > /* We need to register for at least one bitfield */ > assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); > + assert(n->start <= n->end); > QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > memory_region_update_iommu_notify_flags(mr); > } > -------->8-------- > > -- peterx This should work, or you can introduce a memory_region_iommu_notifier_init() to force user to explicitly initialize start and end. Thanks
On Fri, Jan 13, 2017 at 05:37:43PM +0800, Jason Wang wrote: [...] > >>>diff --git a/memory.c b/memory.c > >>>index 2bfc37f..e88bb54 100644 > >>>--- a/memory.c > >>>+++ b/memory.c > >>>@@ -1671,7 +1671,9 @@ void memory_region_notify_iommu(MemoryRegion *mr, > >>> } > >>> QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > >>>- if (iommu_notifier->notifier_flags & request_flags) { > >>>+ if (iommu_notifier->notifier_flags & request_flags && > >>>+ iommu_notifier->start <= entry.iova && > >>>+ iommu_notifier->end >= entry.iova) { > >>> iommu_notifier->notify(iommu_notifier, &entry); > >>> } > >>> } > >>This seems breaks vhost device IOTLB. How about keep the the behavior > >>somehow? > >Thanks to point out. How about I squash this into this patch? > > > >--------8<-------- > >diff --git a/memory.c b/memory.c > >index e88bb54..6de02dd 100644 > >--- a/memory.c > >+++ b/memory.c > >@@ -1608,8 +1608,14 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, > > return; > > } > >+ if (n->start == 0 && n->end == 0) { > >+ /* If these are not specified, we listen to the whole range */ > >+ n->end = (hwaddr)(-1); > >+ } > >+ > > /* We need to register for at least one bitfield */ > > assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); > >+ assert(n->start <= n->end); > > QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > > memory_region_update_iommu_notify_flags(mr); > > } > >-------->8-------- > > > >-- peterx > > This should work, or you can introduce a memory_region_iommu_notifier_init() > to force user to explicitly initialize start and end. Hmm, this sounds better, considering that IOMMUNotifier is getting more fields to be inited. Thanks, -- peterx
diff --git a/memory.c b/memory.c index e88bb54..6de02dd 100644 --- a/memory.c +++ b/memory.c @@ -1608,8 +1608,14 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, return; } + if (n->start == 0 && n->end == 0) { + /* If these are not specified, we listen to the whole range */ + n->end = (hwaddr)(-1); + } + /* We need to register for at least one bitfield */ assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); + assert(n->start <= n->end); QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); memory_region_update_iommu_notify_flags(mr); }