| Message ID | 20200303031114.21111-1-yi.y.sun@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] memory: remove assert to avoid unnecessary coredump | expand |
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? Thanks Yan > Skip this notification but not coredump makes everything > work well. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > 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 >
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? > Thanks > Yan > > > Skip this notification but not coredump makes everything > > work well. > > > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > > --- > > 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 > >
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 <yi.y.sun@linux.intel.com> > > > --- > > > 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 > > >
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 {
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 Skip this notification but not coredump makes everything work well. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- memory.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)