Re: [RFC v8 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB type

[Eugenio Perez Martin]

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