On Wed, Aug 12, 2020 at 4:24 AM Jason Wang <jasow...@redhat.com> wrote: > > > On 2020/8/12 上午1:55, Eugenio Pérez wrote: > > Signed-off-by: Eugenio Pérez <epere...@redhat.com> > > --- > > hw/virtio/vhost.c | 2 +- > > include/exec/memory.h | 2 ++ > > softmmu/memory.c | 10 ++++++++-- > > 3 files changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 1a1384e7a6..e74ad9e09b 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_UNMAP | IOMMU_NOTIFIER_IOTLB, > > > I think we can safely drop IOMMU_NOTIFIER_UNMAP here since device IOTLB > is sufficient. > > Btw, IOMMU_NOTIFIER_IOTLB is kind of confusing, maybe something like > IOMMU_NOTIFIER_DEVIOTLB. >
Got it, will change. > > > section->offset_within_region, > > int128_get64(end), > > iommu_idx); > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 307e527835..4d94c1e984 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 IOTLB entries */ > > + IOMMU_NOTIFIER_IOTLB = 0x04, > > } IOMMUNotifierFlag; > > > > #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > > index af25987518..e2c5f6d0e7 100644 > > --- a/softmmu/memory.c > > +++ b/softmmu/memory.c > > @@ -1895,6 +1895,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier, > > > (we probably need a better name of this function, at least something > like "memory_region_iommu_notify_one"). > Ok will change. > > > { > > IOMMUNotifierFlag request_flags; > > hwaddr entry_end = entry->iova + entry->addr_mask; > > + IOMMUTLBEntry tmp = *entry; > > > > /* > > * Skip the notification if the notification does not overlap > > @@ -1904,7 +1905,12 @@ void memory_region_notify_one(IOMMUNotifier > > *notifier, > > return; > > } > > > > - assert(entry->iova >= notifier->start && entry_end <= notifier->end); > > + if (notifier->notifier_flags & IOMMU_NOTIFIER_IOTLB) { > > + tmp.iova = MAX(tmp.iova, notifier->start); > > + tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova; > > > Any reason for doing such re-calculation here, a comment would be helpful. > It was proposed by Peter, but I understand as limiting the address+range we pass to the notifier. Although vhost seems to support it as long as it contains (notifier->start, notifier->end) in range, a future notifier might not. It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const IOMMUNotifier *notifier) though. > > > + } else { > > + assert(entry->iova >= notifier->start && entry_end <= > > notifier->end); > > > I wonder if it's better to convert the assert so some kind of log or > warn here. > I think that if we transform that assert to a log, we should also tell the guest that something went wrong. Or would it be enough notifying the bad range? If a malicious guest cannot reach that point, I think that leaving it as an assertion allows us to detect earlier the fail in my opinion (Assert early and assert often). Thanks! > Thanks > > > > + } > > > > if (entry->perm & IOMMU_RW) { > > request_flags = IOMMU_NOTIFIER_MAP; > > @@ -1913,7 +1919,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier, > > } > > > > if (notifier->notifier_flags & request_flags) { > > - notifier->notify(notifier, entry); > > + notifier->notify(notifier, &tmp); > > } > > } > > >