| Message ID | 20200818130151.31678-3-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: Delete assertion in memory_region_unregister_iommu_notifier | expand |
On Tue, Aug 18, 2020 at 03:01:51PM +0200, Eugenio Pérez wrote:
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
The changes on the callers of memory_region_notify_one_iommu() seems to be
still missing (and, to embed the type into the notification process)..
On Wed, Aug 19, 2020 at 6:40 PM Peter Xu <peterx@redhat.com> wrote: > > On Tue, Aug 18, 2020 at 03:01:51PM +0200, Eugenio Pérez wrote: > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > The changes on the callers of memory_region_notify_one_iommu() seems to be > still missing (and, to embed the type into the notification process).. > Hi Peter. I thought that these were left for a future patch series (the main motivation was to avoid for guest code to hit the assertion). Do you want me to put them in this series? Thanks! > -- > Peter Xu >
On Wed, Aug 19, 2020 at 7:47 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Wed, Aug 19, 2020 at 6:40 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Tue, Aug 18, 2020 at 03:01:51PM +0200, Eugenio Pérez wrote: > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > The changes on the callers of memory_region_notify_one_iommu() seems to be > > still missing (and, to embed the type into the notification process).. > > > > Hi Peter. > > I thought that these were left for a future patch series (the main s/that these were left/that we were going to left them/ > motivation was to avoid for guest code to hit the assertion). > > Do you want me to put them in this series? > > Thanks! > > > -- > > Peter Xu > >
On Wed, Aug 19, 2020 at 07:47:32PM +0200, Eugenio Perez Martin wrote: > On Wed, Aug 19, 2020 at 6:40 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Tue, Aug 18, 2020 at 03:01:51PM +0200, Eugenio Pérez wrote: > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > The changes on the callers of memory_region_notify_one_iommu() seems to be > > still missing (and, to embed the type into the notification process).. > > > > Hi Peter. > > I thought that these were left for a future patch series (the main > motivation was to avoid for guest code to hit the assertion). > > Do you want me to put them in this series? Imho it would be good to do that altogether. For example, you have defined: /* Notify changes on device IOTLB entries */ IOMMU_NOTIFIER_DEVIOTLB = 0x04, The comment says we'll only notify on device iotlbs, however with the code change we'll still notify even with normal iotlb invalidations (but always with the type IOMMU_NOTIFIER_DEVIOTLB ). IOW vhost with ats=on will still receive two invalidations for the same range just like before.
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 1a1384e7a6..6ca168b47e 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -729,7 +729,7 @@ static void vhost_iommu_region_add(MemoryListener *listener, iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, MEMTXATTRS_UNSPECIFIED); iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify, - IOMMU_NOTIFIER_UNMAP, + IOMMU_NOTIFIER_DEVIOTLB, section->offset_within_region, int128_get64(end), iommu_idx); diff --git a/include/exec/memory.h b/include/exec/memory.h index ed99a80f17..74ab71543a 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -87,6 +87,8 @@ typedef enum { IOMMU_NOTIFIER_UNMAP = 0x1, /* Notify entry changes (newly created entries) */ IOMMU_NOTIFIER_MAP = 0x2, + /* Notify changes on device IOTLB entries */ + IOMMU_NOTIFIER_DEVIOTLB = 0x04, } IOMMUNotifierFlag; #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) diff --git a/softmmu/memory.c b/softmmu/memory.c index 0043791361..bbc910b129 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1895,6 +1895,7 @@ void memory_region_notify_one_iommu(IOMMUNotifier *notifier, { IOMMUNotifierFlag request_flags; hwaddr entry_end = entry->iova + entry->addr_mask; + IOMMUTLBEntry tmp = *entry; /* * Skip the notification if the notification does not overlap @@ -1904,16 +1905,26 @@ void memory_region_notify_one_iommu(IOMMUNotifier *notifier, return; } - assert(entry->iova >= notifier->start && entry_end <= notifier->end); + if (notifier->notifier_flags & IOMMU_NOTIFIER_DEVIOTLB) { + /* Crop (iova, addr_mask) to range */ + tmp.iova = MAX(tmp.iova, notifier->start); + tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova; + /* Confirm no underflow */ + assert(MIN(entry_end, notifier->end) >= tmp.iova); + } else { + assert(entry->iova >= notifier->start && entry_end <= notifier->end); + } if (entry->perm & IOMMU_RW) { request_flags = IOMMU_NOTIFIER_MAP; + } else if (notifier->notifier_flags == IOMMU_NOTIFIER_DEVIOTLB) { + request_flags = IOMMU_NOTIFIER_DEVIOTLB; } else { request_flags = IOMMU_NOTIFIER_UNMAP; } if (notifier->notifier_flags & request_flags) { - notifier->notify(notifier, entry); + notifier->notify(notifier, &tmp); } }
Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost.c | 2 +- include/exec/memory.h | 2 ++ softmmu/memory.c | 15 +++++++++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)