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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Michael S. Tsirkin <mst@redhat.com>
>Sent: Wednesday, April 5, 2023 3:13 AM
>To: Peter Maydell <peter.maydell@linaro.org>
>Cc: qemu-devel@nongnu.org; Duan, Zhenzhong
><zhenzhong.duan@intel.com>; Peter Xu <peterx@redhat.com>; Jason Wang
><jasowang@redhat.com>; Marcel Apfelbaum
><marcel.apfelbaum@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>;
>Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost
><eduardo@habkost.net>; David Hildenbrand <david@redhat.com>; Philippe
>Mathieu-Daudé <philmd@linaro.org>
>Subject: Re: [PULL 36/53] memory: Optimize replay of guest mapping
>
>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!

Sorry for the trouble, I'll fix and repost a new version later with wider test.
Initial thought is to pick the intersection of iommu_mr and iommu notifier
in memory_region_iommu_replay(), then the assert() could be dropped.

Regards
Zhenzhong