Re: [PULL 36/53] memory: Optimize replay of guest mapping

[Michael S. Tsirkin]

On Tue, Apr 04, 2023 at 07:00:04PM +0100, Peter Maydell wrote:
> On Thu, 2 Mar 2023 at 08:26, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Zhenzhong Duan <zhenzhong.duan@intel.com>
> >
> > On x86, there are two notifiers registered due to vtd-ir memory region
> > splitting the whole address space. During replay of the address space
> > for each notifier, the whole address space is scanned which is
> > unnecessory.
> >
> > We only need to scan the space belong to notifier montiored space.
> >
> > Assert when notifier is used to monitor beyond iommu memory region's
> > address space.
> 
> Hi. This patch seems to have regressed the mps3-an547 board,
> which now asserts on startup:
> 
> $ ./build/x86/qemu-system-arm --machine mps3-an547 -serial stdio
> -kernel /tmp/an547-mwe/build/test.elf
> qemu-system-arm: ../../softmmu/memory.c:1903:
> memory_region_register_iommu_notifier: Assertion `n->end <=
> memory_region_size(mr)' failed.
> Aborted (core dumped)
> 
> (reported under https://gitlab.com/qemu-project/qemu/-/issues/1488)
> 
> Since this commit says it's just an optimization, for the 8.0
> release can we simply revert it without breaking anything?
> 
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 9d64efca26..da7d846619 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1900,6 +1900,7 @@ int 
> > memory_region_register_iommu_notifier(MemoryRegion *mr,
> >      iommu_mr = IOMMU_MEMORY_REGION(mr);
> >      assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
> >      assert(n->start <= n->end);
> > +    assert(n->end <= memory_region_size(mr));
> 
> In the mps3-an547 case we assert here because n->end is -1.
> This is because tcg_register_iommu_notifier() registers an iommu
> notifier that covers the entire address space:
> 
>         iommu_notifier_init(&notifier->n,
>                             tcg_iommu_unmap_notify,
>                             IOMMU_NOTIFIER_UNMAP,
>                             0,
>                             HWADDR_MAX,
>                             iommu_idx);
>         memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
>                                               &error_fatal);
> 
> thanks
> -- PMM


Fine to revert by me.  Zhenzhong Duan  can you pls fix up
this regression and repost? Maybe fix typos in commit log
when reposting. Thanks!

-- 
MST