Re: [PATCH v1] memory: remove assert to avoid unnecessary coredump

[Yan Zhao]

On Tue, Mar 03, 2020 at 01:22:26PM +0800, Yi Sun wrote:
> On 20-03-02 22:36:39, Yan Zhao wrote:
> > On Tue, Mar 03, 2020 at 11:11:14AM +0800, Yi Sun wrote:
> > > It is too strict to use assert to make qemu coredump if
> > > the notification does not overlap with registered range.
> > > Skip it is fine enough.
> > > 
> > > During test, we found such a case for vhost net device:
> > >     memory_region_notify_one: entry->iova=0xfee00000, 
> > > entry_end=0xfeffffff, notifier->start=0xfef00000, 
> > > notifier->end=0xffffffffffffffff
> > >
> > so for range from 0xfef00000 to 0xfefffff,  would notification for this
> > range get lost?
> > 
> Yes, that is an issue although there is no any problem found during test
> with this fix.
> 
> I think we should notify the intersection between entry and notifier. How
> do you think?
> 
no. please refer to the link below.
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04218.html

Thanks
Yan

> > Thanks
> > Yan
> > 
> > > Skip this notification but not coredump makes everything
> > > work well.
> > > 
> > > Signed-off-by: Yi Sun <address@hidden>
> > > ---
> > >  memory.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/memory.c b/memory.c
> > > index 06484c2bff..62ad0f3377 100644
> > > --- a/memory.c
> > > +++ b/memory.c
> > > @@ -1921,12 +1921,11 @@ void memory_region_notify_one(IOMMUNotifier 
> > > *notifier,
> > >       * Skip the notification if the notification does not overlap
> > >       * with registered range.
> > >       */
> > > -    if (notifier->start > entry_end || notifier->end < entry->iova) {
> > > +    if (notifier->start > entry_end || notifier->end < entry->iova ||
> > > +        entry->iova < notifier->start || entry_end > notifier->end) {
> > >          return;
> > >      }
> > >  
> > > -    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> > > -
> > >      if (entry->perm & IOMMU_RW) {
> > >          request_flags = IOMMU_NOTIFIER_MAP;
> > >      } else {
> > > -- 
> > > 2.15.1
> > >