On Mon, Sep 28, 2020 at 11:05:01AM +0200, Eugenio Perez Martin wrote: > On Fri, Sep 4, 2020 at 6:34 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > On 2020/9/4 上午12:14, Eugenio Pérez wrote: > > > Device IOTLB invalidations can unmap arbitrary ranges, eiter outside of > > > the memory region or even [0, ~0ULL] for all the space. The assertion > > > could be hit by a guest, and rhel7 guest effectively hit it. > > > > > > Signed-off-by: Eugenio Pérez <epere...@redhat.com> > > > Reviewed-by: Peter Xu <pet...@redhat.com> > > > Reviewed-by: Juan Quintela <quint...@redhat.com> > > > --- > > > softmmu/memory.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/softmmu/memory.c b/softmmu/memory.c > > > index 8694fc7cf7..e723fcbaa1 100644 > > > --- a/softmmu/memory.c > > > +++ b/softmmu/memory.c > > > @@ -1895,6 +1895,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier > > > *notifier, > > > { > > > IOMMUTLBEntry *entry = &event->entry; > > > hwaddr entry_end = entry->iova + entry->addr_mask; > > > + IOMMUTLBEntry tmp = *entry; > > > > > > if (event->type == IOMMU_NOTIFIER_UNMAP) { > > > assert(entry->perm == IOMMU_NONE); > > > @@ -1908,10 +1909,18 @@ void memory_region_notify_iommu_one(IOMMUNotifier > > > *notifier, > > > return; > > > } > > > > > > - assert(entry->iova >= notifier->start && entry_end <= notifier->end); > > > + if (notifier->notifier_flags & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) { > > > + /* 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); > > > > > > It's still not clear to me why we need such assert. Consider > > notifier->end is the possible IOVA range but not possible device IOTLB > > invalidation range (e.g it allows [0, ULLONG_MAX]). > > > > Thanks > > > > As far as I understood the device should admit that out of bounds > notifications in that case, > and the assert just makes sure that there was no underflow in > tmp.addr_mask, i.e., that something > very wrong that should never happen in production happened. > > Peter, would you mind to confirm/correct it?
I think Jason is right - since we have checked at the entry that the two regions cross over each other: /* * Skip the notification if the notification does not overlap * with registered range. */ if (notifier->start > entry_end || notifier->end < entry->iova) { return; } Then I don't see how this assertion can fail any more. But imho not a big problem either, and it shouldn't hurt to even keep the assertion of above isn't that straightforward. > > Is there anything else needed to pull this patch? I didn't post a pull for this only because I shouldn't :) - the plan was that all vt-d patches will still go via Michael's tree, iiuc. Though at least to me I think this series is acceptable for merging. Though it would always be good too if Jason would still like to review it. Jason, what's your opinion? Thanks, -- Peter Xu