| Message ID | 20170410070950.GK3981@pxdev.xzpeter.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 10, 2017 at 03:09:50PM +0800, Peter Xu wrote: > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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. > > > > > > This patch is solving the problem that vfio-pci devices receive > > > duplicated UNMAP notification on x86 platform when vIOMMU is there. The > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK > > > this (splitted IOMMU region) is only happening on x86. > > > > > > This patch also helps vhost to leverage the new interface as well, so > > > that vhost won't get duplicated cache flushes. In that sense, it's an > > > slight performance improvement. > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > Acked-by: Alex Williamson <alex.williamson@redhat.com> > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > hw/vfio/common.c | 12 +++++++++--- > > > hw/virtio/vhost.c | 10 ++++++++-- > > > include/exec/memory.h | 19 ++++++++++++++++++- > > > memory.c | 9 +++++++++ > > > 4 files changed, 44 insertions(+), 6 deletions(-) > > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > > index f3ba9b9..6b33b9f 100644 > > > --- a/hw/vfio/common.c > > > +++ b/hw/vfio/common.c > > > @@ -478,8 +478,13 @@ static void vfio_listener_region_add(MemoryListener *listener, > > > giommu->iommu_offset = section->offset_within_address_space - > > > section->offset_within_region; > > > giommu->container = container; > > > - giommu->n.notify = vfio_iommu_map_notify; > > > - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; > > > + llend = int128_add(int128_make64(section->offset_within_region), > > > + section->size); > > > + llend = int128_sub(llend, int128_one()); > > > + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, > > > + IOMMU_NOTIFIER_ALL, > > > + section->offset_within_region, > > > + int128_get64(llend)); > > > > Seems to me it would make sense to put the fiddling around to convert > > the MemoryRegionSection into the necessary values would make sense to > > go inside iommu_notifier_init(). > > But will we always get one MemoryRegionSection if we are not in any of > the region_{add|del} callback? E.g., what if we want to init an IOMMU > notifier that covers just the whole IOMMU region range? I suppose so. It's just the only likely users of the interface I can see will be always doing this from region_{add,del}. > Considering above, I would still slightly prefer current interface. Ok, well my opinion on the matter isn't terribly strong. > > > > > > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > > > > > > memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); > > > @@ -550,7 +555,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) { > > > > This test should be sufficient, but I'd be a bit more comfortable if > > there was a test and assert() that the end matches as well. I also > > wonder if remove-matching-notifier helper would be useful here. > > Although vhost doesn't appear to ever remove, so maybe it's premature. > > Oh... vhost does remove it, but I just forgot to touch it up :( ... > Thanks for pointing out. > > Marcel, if this is the only comment, would you mind squash below > change into current patch? Thanks, > > ----8<---- > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 185b95b..0001e60 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -771,7 +771,8 @@ static void vhost_iommu_region_del(MemoryListener *listener, > } > > QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) { > - if (iommu->mr == section->mr) { > + if (iommu->mr == section->mr && > + iommu->n.start == section->offset_within_region) { > memory_region_unregister_iommu_notifier(iommu->mr, > &iommu->n); > QLIST_REMOVE(iommu, iommu_next); > > ---->8---- > > > > > > memory_region_unregister_iommu_notifier(giommu->iommu, > > > &giommu->n); > > > QLIST_REMOVE(giommu, giommu_next); > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index 613494d..185b95b 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -736,14 +736,20 @@ static void vhost_iommu_region_add(MemoryListener *listener, > > > struct vhost_dev *dev = container_of(listener, struct vhost_dev, > > > iommu_listener); > > > struct vhost_iommu *iommu; > > > + Int128 end; > > > > > > if (!memory_region_is_iommu(section->mr)) { > > > return; > > > } > > > > > > iommu = g_malloc0(sizeof(*iommu)); > > > - iommu->n.notify = vhost_iommu_unmap_notify; > > > - iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP; > > > + end = int128_add(int128_make64(section->offset_within_region), > > > + section->size); > > > + end = int128_sub(end, int128_one()); > > > + iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify, > > > + IOMMU_NOTIFIER_UNMAP, > > > + section->offset_within_region, > > > + int128_get64(end)); > > > iommu->mr = section->mr; > > > iommu->iommu_offset = section->offset_within_address_space - > > > section->offset_within_region; > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > index f20b191..0840c89 100644 > > > --- a/include/exec/memory.h > > > +++ b/include/exec/memory.h > > > @@ -77,13 +77,30 @@ typedef enum { > > > > > > #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) > > > > > > +struct IOMMUNotifier; > > > +typedef void (*IOMMUNotify)(struct IOMMUNotifier *notifier, > > > + IOMMUTLBEntry *data); > > > + > > > struct IOMMUNotifier { > > > - void (*notify)(struct IOMMUNotifier *notifier, IOMMUTLBEntry *data); > > > + IOMMUNotify notify; > > > IOMMUNotifierFlag notifier_flags; > > > + /* Notify for address space range start <= addr <= end */ > > > + hwaddr start; > > > + hwaddr end; > > > QLIST_ENTRY(IOMMUNotifier) node; > > > }; > > > typedef struct IOMMUNotifier IOMMUNotifier; > > > > > > +static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, > > > + IOMMUNotifierFlag flags, > > > + hwaddr start, hwaddr end) > > > +{ > > > + n->notify = fn; > > > + n->notifier_flags = flags; > > > + n->start = start; > > > + n->end = end; > > > +} > > > + > > > /* 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 > > > diff --git a/memory.c b/memory.c > > > index 4c95aaf..75ac595 100644 > > > --- a/memory.c > > > +++ b/memory.c > > > @@ -1606,6 +1606,7 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, > > > > > > /* 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); > > > } > > > @@ -1667,6 +1668,14 @@ void memory_region_notify_iommu(MemoryRegion *mr, > > > } > > > > > > QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > + /* > > > + * Skip the notification if the notification does not overlap > > > + * with registered range. > > > + */ > > > + if (iommu_notifier->start > entry.iova + entry.addr_mask + 1 || > > > + iommu_notifier->end < entry.iova) { > > > + continue; > > > + } > > > if (iommu_notifier->notifier_flags & request_flags) { > > > iommu_notifier->notify(iommu_notifier, &entry); > > > } > > > > -- peterx >
On Tue, Apr 11, 2017 at 11:56:54AM +1000, David Gibson wrote: > On Mon, Apr 10, 2017 at 03:09:50PM +0800, Peter Xu wrote: > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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. > > > > > > > > This patch is solving the problem that vfio-pci devices receive > > > > duplicated UNMAP notification on x86 platform when vIOMMU is there. The > > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is > > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK > > > > this (splitted IOMMU region) is only happening on x86. > > > > > > > > This patch also helps vhost to leverage the new interface as well, so > > > > that vhost won't get duplicated cache flushes. In that sense, it's an > > > > slight performance improvement. > > > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > Acked-by: Alex Williamson <alex.williamson@redhat.com> > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > hw/vfio/common.c | 12 +++++++++--- > > > > hw/virtio/vhost.c | 10 ++++++++-- > > > > include/exec/memory.h | 19 ++++++++++++++++++- > > > > memory.c | 9 +++++++++ > > > > 4 files changed, 44 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > > > index f3ba9b9..6b33b9f 100644 > > > > --- a/hw/vfio/common.c > > > > +++ b/hw/vfio/common.c > > > > @@ -478,8 +478,13 @@ static void vfio_listener_region_add(MemoryListener *listener, > > > > giommu->iommu_offset = section->offset_within_address_space - > > > > section->offset_within_region; > > > > giommu->container = container; > > > > - giommu->n.notify = vfio_iommu_map_notify; > > > > - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; > > > > + llend = int128_add(int128_make64(section->offset_within_region), > > > > + section->size); > > > > + llend = int128_sub(llend, int128_one()); > > > > + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, > > > > + IOMMU_NOTIFIER_ALL, > > > > + section->offset_within_region, > > > > + int128_get64(llend)); > > > > > > Seems to me it would make sense to put the fiddling around to convert > > > the MemoryRegionSection into the necessary values would make sense to > > > go inside iommu_notifier_init(). > > > > But will we always get one MemoryRegionSection if we are not in any of > > the region_{add|del} callback? E.g., what if we want to init an IOMMU > > notifier that covers just the whole IOMMU region range? > > I suppose so. It's just the only likely users of the interface I can > see will be always doing this from region_{add,del}. > > > Considering above, I would still slightly prefer current interface. > > Ok, well my opinion on the matter isn't terribly strong. Hi, David, Sorry to respond late (so that context might be lost). Just want to make sure that you are okay with current patch and interface, right? I think Marcel is going to merge it if np, and I would like to have your confirmation on this before the merge. Thanks!
On Tue, Apr 18, 2017 at 05:56:37PM +0800, Peter Xu wrote: > On Tue, Apr 11, 2017 at 11:56:54AM +1000, David Gibson wrote: > > On Mon, Apr 10, 2017 at 03:09:50PM +0800, Peter Xu wrote: > > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: > > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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. > > > > > > > > > > This patch is solving the problem that vfio-pci devices receive > > > > > duplicated UNMAP notification on x86 platform when vIOMMU is there. The > > > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is > > > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK > > > > > this (splitted IOMMU region) is only happening on x86. > > > > > > > > > > This patch also helps vhost to leverage the new interface as well, so > > > > > that vhost won't get duplicated cache flushes. In that sense, it's an > > > > > slight performance improvement. > > > > > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > > Acked-by: Alex Williamson <alex.williamson@redhat.com> > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > --- > > > > > hw/vfio/common.c | 12 +++++++++--- > > > > > hw/virtio/vhost.c | 10 ++++++++-- > > > > > include/exec/memory.h | 19 ++++++++++++++++++- > > > > > memory.c | 9 +++++++++ > > > > > 4 files changed, 44 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > > > > index f3ba9b9..6b33b9f 100644 > > > > > --- a/hw/vfio/common.c > > > > > +++ b/hw/vfio/common.c > > > > > @@ -478,8 +478,13 @@ static void vfio_listener_region_add(MemoryListener *listener, > > > > > giommu->iommu_offset = section->offset_within_address_space - > > > > > section->offset_within_region; > > > > > giommu->container = container; > > > > > - giommu->n.notify = vfio_iommu_map_notify; > > > > > - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; > > > > > + llend = int128_add(int128_make64(section->offset_within_region), > > > > > + section->size); > > > > > + llend = int128_sub(llend, int128_one()); > > > > > + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, > > > > > + IOMMU_NOTIFIER_ALL, > > > > > + section->offset_within_region, > > > > > + int128_get64(llend)); > > > > > > > > Seems to me it would make sense to put the fiddling around to convert > > > > the MemoryRegionSection into the necessary values would make sense to > > > > go inside iommu_notifier_init(). > > > > > > But will we always get one MemoryRegionSection if we are not in any of > > > the region_{add|del} callback? E.g., what if we want to init an IOMMU > > > notifier that covers just the whole IOMMU region range? > > > > I suppose so. It's just the only likely users of the interface I can > > see will be always doing this from region_{add,del}. > > > > > Considering above, I would still slightly prefer current interface. > > > > Ok, well my opinion on the matter isn't terribly strong. > > Hi, David, > > Sorry to respond late (so that context might be lost). Just want to > make sure that you are okay with current patch and interface, right? > > I think Marcel is going to merge it if np, and I would like to have > your confirmation on this before the merge. Thanks! Yes, that's fine.
On 04/10/2017 10:09 AM, Peter Xu wrote: > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: >> On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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. >>> >>> This patch is solving the problem that vfio-pci devices receive >>> duplicated UNMAP notification on x86 platform when vIOMMU is there. The >>> issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is >>> splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK >>> this (splitted IOMMU region) is only happening on x86. >>> >>> This patch also helps vhost to leverage the new interface as well, so >>> that vhost won't get duplicated cache flushes. In that sense, it's an >>> slight performance improvement. >>> >>> Suggested-by: David Gibson <david@gibson.dropbear.id.au> >>> Reviewed-by: Eric Auger <eric.auger@redhat.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Acked-by: Alex Williamson <alex.williamson@redhat.com> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> hw/vfio/common.c | 12 +++++++++--- >>> hw/virtio/vhost.c | 10 ++++++++-- >>> include/exec/memory.h | 19 ++++++++++++++++++- >>> memory.c | 9 +++++++++ >>> 4 files changed, 44 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index f3ba9b9..6b33b9f 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -478,8 +478,13 @@ static void vfio_listener_region_add(MemoryListener *listener, >>> giommu->iommu_offset = section->offset_within_address_space - >>> section->offset_within_region; >>> giommu->container = container; >>> - giommu->n.notify = vfio_iommu_map_notify; >>> - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; >>> + llend = int128_add(int128_make64(section->offset_within_region), >>> + section->size); >>> + llend = int128_sub(llend, int128_one()); >>> + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, >>> + IOMMU_NOTIFIER_ALL, >>> + section->offset_within_region, >>> + int128_get64(llend)); >> >> Seems to me it would make sense to put the fiddling around to convert >> the MemoryRegionSection into the necessary values would make sense to >> go inside iommu_notifier_init(). > > But will we always get one MemoryRegionSection if we are not in any of > the region_{add|del} callback? E.g., what if we want to init an IOMMU > notifier that covers just the whole IOMMU region range? > > Considering above, I would still slightly prefer current interface. > >> >>> QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); >>> >>> memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); >>> @@ -550,7 +555,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) { >> >> This test should be sufficient, but I'd be a bit more comfortable if >> there was a test and assert() that the end matches as well. I also >> wonder if remove-matching-notifier helper would be useful here. >> Although vhost doesn't appear to ever remove, so maybe it's premature. > > Oh... vhost does remove it, but I just forgot to touch it up :( ... > Thanks for pointing out. > > Marcel, if this is the only comment, would you mind squash below > change into current patch? Thanks, Hi Peter, I asked Eduardo to merge the series through this machine tree since I don't have a tree yet. He already squashed the change, thanks Eduardo! Marcel > > ----8<---- > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 185b95b..0001e60 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -771,7 +771,8 @@ static void vhost_iommu_region_del(MemoryListener *listener, > } > > QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) { > - if (iommu->mr == section->mr) { > + if (iommu->mr == section->mr && > + iommu->n.start == section->offset_within_region) { > memory_region_unregister_iommu_notifier(iommu->mr, > &iommu->n); > QLIST_REMOVE(iommu, iommu_next); > > ---->8---- > >> >>> memory_region_unregister_iommu_notifier(giommu->iommu, >>> &giommu->n); >>> QLIST_REMOVE(giommu, giommu_next); >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index 613494d..185b95b 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -736,14 +736,20 @@ static void vhost_iommu_region_add(MemoryListener *listener, >>> struct vhost_dev *dev = container_of(listener, struct vhost_dev, >>> iommu_listener); >>> struct vhost_iommu *iommu; >>> + Int128 end; >>> >>> if (!memory_region_is_iommu(section->mr)) { >>> return; >>> } >>> >>> iommu = g_malloc0(sizeof(*iommu)); >>> - iommu->n.notify = vhost_iommu_unmap_notify; >>> - iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP; >>> + end = int128_add(int128_make64(section->offset_within_region), >>> + section->size); >>> + end = int128_sub(end, int128_one()); >>> + iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify, >>> + IOMMU_NOTIFIER_UNMAP, >>> + section->offset_within_region, >>> + int128_get64(end)); >>> iommu->mr = section->mr; >>> iommu->iommu_offset = section->offset_within_address_space - >>> section->offset_within_region; >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index f20b191..0840c89 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -77,13 +77,30 @@ typedef enum { >>> >>> #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) >>> >>> +struct IOMMUNotifier; >>> +typedef void (*IOMMUNotify)(struct IOMMUNotifier *notifier, >>> + IOMMUTLBEntry *data); >>> + >>> struct IOMMUNotifier { >>> - void (*notify)(struct IOMMUNotifier *notifier, IOMMUTLBEntry *data); >>> + IOMMUNotify notify; >>> IOMMUNotifierFlag notifier_flags; >>> + /* Notify for address space range start <= addr <= end */ >>> + hwaddr start; >>> + hwaddr end; >>> QLIST_ENTRY(IOMMUNotifier) node; >>> }; >>> typedef struct IOMMUNotifier IOMMUNotifier; >>> >>> +static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, >>> + IOMMUNotifierFlag flags, >>> + hwaddr start, hwaddr end) >>> +{ >>> + n->notify = fn; >>> + n->notifier_flags = flags; >>> + n->start = start; >>> + n->end = end; >>> +} >>> + >>> /* 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 >>> diff --git a/memory.c b/memory.c >>> index 4c95aaf..75ac595 100644 >>> --- a/memory.c >>> +++ b/memory.c >>> @@ -1606,6 +1606,7 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, >>> >>> /* 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); >>> } >>> @@ -1667,6 +1668,14 @@ void memory_region_notify_iommu(MemoryRegion *mr, >>> } >>> >>> QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { >>> + /* >>> + * Skip the notification if the notification does not overlap >>> + * with registered range. >>> + */ >>> + if (iommu_notifier->start > entry.iova + entry.addr_mask + 1 || >>> + iommu_notifier->end < entry.iova) { >>> + continue; >>> + } >>> if (iommu_notifier->notifier_flags & request_flags) { >>> iommu_notifier->notify(iommu_notifier, &entry); >>> } >> >> -- >> 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 > > -- peterx >
On Tue, Apr 18, 2017 at 06:20:11PM +0300, Marcel Apfelbaum wrote: > On 04/10/2017 10:09 AM, Peter Xu wrote: > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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. > > > > > > > > This patch is solving the problem that vfio-pci devices receive > > > > duplicated UNMAP notification on x86 platform when vIOMMU is there. The > > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is > > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK > > > > this (splitted IOMMU region) is only happening on x86. > > > > > > > > This patch also helps vhost to leverage the new interface as well, so > > > > that vhost won't get duplicated cache flushes. In that sense, it's an > > > > slight performance improvement. > > > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > Acked-by: Alex Williamson <alex.williamson@redhat.com> > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > hw/vfio/common.c | 12 +++++++++--- > > > > hw/virtio/vhost.c | 10 ++++++++-- > > > > include/exec/memory.h | 19 ++++++++++++++++++- > > > > memory.c | 9 +++++++++ > > > > 4 files changed, 44 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > > > index f3ba9b9..6b33b9f 100644 > > > > --- a/hw/vfio/common.c > > > > +++ b/hw/vfio/common.c > > > > @@ -478,8 +478,13 @@ static void vfio_listener_region_add(MemoryListener *listener, > > > > giommu->iommu_offset = section->offset_within_address_space - > > > > section->offset_within_region; > > > > giommu->container = container; > > > > - giommu->n.notify = vfio_iommu_map_notify; > > > > - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; > > > > + llend = int128_add(int128_make64(section->offset_within_region), > > > > + section->size); > > > > + llend = int128_sub(llend, int128_one()); > > > > + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, > > > > + IOMMU_NOTIFIER_ALL, > > > > + section->offset_within_region, > > > > + int128_get64(llend)); > > > > > > Seems to me it would make sense to put the fiddling around to convert > > > the MemoryRegionSection into the necessary values would make sense to > > > go inside iommu_notifier_init(). > > > > But will we always get one MemoryRegionSection if we are not in any of > > the region_{add|del} callback? E.g., what if we want to init an IOMMU > > notifier that covers just the whole IOMMU region range? > > > > Considering above, I would still slightly prefer current interface. > > > > > > > > > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > > > > > > > > memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); > > > > @@ -550,7 +555,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) { > > > > > > This test should be sufficient, but I'd be a bit more comfortable if > > > there was a test and assert() that the end matches as well. I also > > > wonder if remove-matching-notifier helper would be useful here. > > > Although vhost doesn't appear to ever remove, so maybe it's premature. > > > > Oh... vhost does remove it, but I just forgot to touch it up :( ... > > Thanks for pointing out. > > > > Marcel, if this is the only comment, would you mind squash below > > change into current patch? Thanks, > > Hi Peter, > I asked Eduardo to merge the series through this machine tree > since I don't have a tree yet. > He already squashed the change, thanks Eduardo! > Marcel The series is queued at: git://github.com/ehabkost/qemu-hacks.git machine-next (Including the vhost_iommu_region_del() fixup below). v2.9.0-rc5 was tagged today, v2.9.0 is expected to be tagged in 2 days. I plan to submit a pull request as soon as it is tagged.
On Tue, Apr 18, 2017 at 09:55:11PM +1000, David Gibson wrote: > On Tue, Apr 18, 2017 at 05:56:37PM +0800, Peter Xu wrote: > > On Tue, Apr 11, 2017 at 11:56:54AM +1000, David Gibson wrote: > > > On Mon, Apr 10, 2017 at 03:09:50PM +0800, Peter Xu wrote: > > > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: > > > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, 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 range overlaps with 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. > > > > > > > > > > > > This patch is solving the problem that vfio-pci devices receive > > > > > > duplicated UNMAP notification on x86 platform when vIOMMU is there. The > > > > > > issue is that x86 IOMMU has a (0, 2^64-1) IOMMU region, which is > > > > > > splitted by the (0xfee00000, 0xfeefffff) IRQ region. AFAIK > > > > > > this (splitted IOMMU region) is only happening on x86. > > > > > > > > > > > > This patch also helps vhost to leverage the new interface as well, so > > > > > > that vhost won't get duplicated cache flushes. In that sense, it's an > > > > > > slight performance improvement. > > > > > > > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > Acked-by: Alex Williamson <alex.williamson@redhat.com> > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > --- > > > > > > hw/vfio/common.c | 12 +++++++++--- > > > > > > hw/virtio/vhost.c | 10 ++++++++-- > > > > > > include/exec/memory.h | 19 ++++++++++++++++++- > > > > > > memory.c | 9 +++++++++ > > > > > > 4 files changed, 44 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > > > > > index f3ba9b9..6b33b9f 100644 > > > > > > --- a/hw/vfio/common.c > > > > > > +++ b/hw/vfio/common.c > > > > > > @@ -478,8 +478,13 @@ static void vfio_listener_region_add(MemoryListener *listener, > > > > > > giommu->iommu_offset = section->offset_within_address_space - > > > > > > section->offset_within_region; > > > > > > giommu->container = container; > > > > > > - giommu->n.notify = vfio_iommu_map_notify; > > > > > > - giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL; > > > > > > + llend = int128_add(int128_make64(section->offset_within_region), > > > > > > + section->size); > > > > > > + llend = int128_sub(llend, int128_one()); > > > > > > + iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, > > > > > > + IOMMU_NOTIFIER_ALL, > > > > > > + section->offset_within_region, > > > > > > + int128_get64(llend)); > > > > > > > > > > Seems to me it would make sense to put the fiddling around to convert > > > > > the MemoryRegionSection into the necessary values would make sense to > > > > > go inside iommu_notifier_init(). > > > > > > > > But will we always get one MemoryRegionSection if we are not in any of > > > > the region_{add|del} callback? E.g., what if we want to init an IOMMU > > > > notifier that covers just the whole IOMMU region range? > > > > > > I suppose so. It's just the only likely users of the interface I can > > > see will be always doing this from region_{add,del}. > > > > > > > Considering above, I would still slightly prefer current interface. > > > > > > Ok, well my opinion on the matter isn't terribly strong. > > > > Hi, David, > > > > Sorry to respond late (so that context might be lost). Just want to > > make sure that you are okay with current patch and interface, right? > > > > I think Marcel is going to merge it if np, and I would like to have > > your confirmation on this before the merge. Thanks! > > Yes, that's fine. I see that Edurado has already queued the patches. Thanks for the confirmation!
On Tue, Apr 18, 2017 at 02:30:54PM -0300, Eduardo Habkost wrote: > On Tue, Apr 18, 2017 at 06:20:11PM +0300, Marcel Apfelbaum wrote: > > On 04/10/2017 10:09 AM, Peter Xu wrote: > > > On Mon, Apr 10, 2017 at 02:39:22PM +1000, David Gibson wrote: > > > > On Fri, Apr 07, 2017 at 06:59:07PM +0800, Peter Xu wrote: [...] > > Hi Peter, > > I asked Eduardo to merge the series through this machine tree > > since I don't have a tree yet. > > He already squashed the change, thanks Eduardo! > > Marcel > > The series is queued at: > > git://github.com/ehabkost/qemu-hacks.git machine-next > > (Including the vhost_iommu_region_del() fixup below). > > v2.9.0-rc5 was tagged today, v2.9.0 is expected to be tagged in 2 > days. I plan to submit a pull request as soon as it is tagged. Thanks Marcel and Edurado for your help on the merging!
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 185b95b..0001e60 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -771,7 +771,8 @@ static void vhost_iommu_region_del(MemoryListener *listener, } QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) { - if (iommu->mr == section->mr) { + if (iommu->mr == section->mr && + iommu->n.start == section->offset_within_region) { memory_region_unregister_iommu_notifier(iommu->mr, &iommu->n); QLIST_REMOVE(iommu, iommu_next);