| Message ID | 20200901142608.24481-6-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory: Delete assertion in memory_region_unregister_iommu_notifier | expand |
On Tue, Sep 01, 2020 at 04:26:08PM +0200, Eugenio Pérez wrote: > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com>
Eugenio Pérez <eperezma@redhat.com> wrote: > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
Hi Eugenio, On 9/1/20 4:26 PM, Eugenio Pérez wrote: > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> Please could you explain in the commit message why you need to remove the assert()? I know you described the assert() in the cover letter but the commit msg is the one that remains. > --- > softmmu/memory.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 09b3443eac..3ee99b4dc0 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; > > /* > * Skip the notification if the notification does not overlap > @@ -1904,10 +1905,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) { > + /* 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 (event->type & notifier->notifier_flags) { > - notifier->notify(notifier, entry); > + notifier->notify(notifier, &tmp); > } > } > > Thanks Eric
On Wed, Sep 02, 2020 at 04:24:50PM +0200, Auger Eric wrote: > Hi Eugenio, > > On 9/1/20 4:26 PM, Eugenio Pérez wrote: > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > Please could you explain in the commit message why you need to remove > the assert()? I know you described the assert() in the cover letter but > the commit msg is the one that remains. Right... neither in the cover letter nor the individual patches, AFAICT, does anything actually explain why that assert() could be hit. Nor does it connect the dots from an assert() hitting to adding infrastructure for a new event type. > > --- > > softmmu/memory.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/softmmu/memory.c b/softmmu/memory.c > > index 09b3443eac..3ee99b4dc0 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; > > > > /* > > * Skip the notification if the notification does not overlap > > @@ -1904,10 +1905,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) { > > + /* 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 (event->type & notifier->notifier_flags) { > > - notifier->notify(notifier, entry); > > + notifier->notify(notifier, &tmp); > > } > > } > > > > > Thanks > > Eric >
On Thu, Sep 3, 2020 at 2:05 AM David Gibson <david@gibson.dropbear.id.au> wrote: > > On Wed, Sep 02, 2020 at 04:24:50PM +0200, Auger Eric wrote: > > Hi Eugenio, > > > > On 9/1/20 4:26 PM, Eugenio Pérez wrote: > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > Please could you explain in the commit message why you need to remove > > the assert()? I know you described the assert() in the cover letter but > > the commit msg is the one that remains. > > Right... neither in the cover letter nor the individual patches, > AFAICT, does anything actually explain why that assert() could be > hit. Nor does it connect the dots from an assert() hitting to adding > infrastructure for a new event type. > Hi! You are right. I think I've made it clearer in the new patch sent (now as patch instead of RFC). Please let me know if you think further explanations are needed. Thanks! > > > --- > > > softmmu/memory.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/softmmu/memory.c b/softmmu/memory.c > > > index 09b3443eac..3ee99b4dc0 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; > > > > > > /* > > > * Skip the notification if the notification does not overlap > > > @@ -1904,10 +1905,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) { > > > + /* 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 (event->type & notifier->notifier_flags) { > > > - notifier->notify(notifier, entry); > > > + notifier->notify(notifier, &tmp); > > > } > > > } > > > > > > > > Thanks > > > > Eric > > > > -- > 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
diff --git a/softmmu/memory.c b/softmmu/memory.c index 09b3443eac..3ee99b4dc0 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; /* * Skip the notification if the notification does not overlap @@ -1904,10 +1905,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) { + /* 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 (event->type & notifier->notifier_flags) { - notifier->notify(notifier, entry); + notifier->notify(notifier, &tmp); } }
Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- softmmu/memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)