| Message ID | 20230302082343.560446-37-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/53] hw/smbios: fix field corruption in type 4 table | expand |
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(¬ifier->n, tcg_iommu_unmap_notify, IOMMU_NOTIFIER_UNMAP, 0, HWADDR_MAX, iommu_idx); memory_region_register_iommu_notifier(notifier->mr, ¬ifier->n, &error_fatal); thanks -- PMM
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(¬ifier->n, > tcg_iommu_unmap_notify, > IOMMU_NOTIFIER_UNMAP, > 0, > HWADDR_MAX, > iommu_idx); > memory_region_register_iommu_notifier(notifier->mr, ¬ifier->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!
On Tue, 4 Apr 2023 at 20:13, Michael S. Tsirkin <mst@redhat.com> wrote: > > 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? > 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! Would somebody also like to send the 'revert' patch, please? I had that all ready to go, but my git send-email setup seems to have mysteriously broken and I don't have time to fix it this evening :-( This is the commit message I wrote: Revert "memory: Optimize replay of guest mapping" This reverts commit 6da24341866fa940fd7d575788a2319514941c77 ("memory: Optimize replay of guest mapping"). This change breaks the mps3-an547 board under TCG (and probably other TCG boards using an IOMMU), which now assert: $ ./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. This is because tcg_register_iommu_notifier() registers an IOMMU notifier which covers the entire address space, so the assertion added in this commit is not correct. For the 8.0 release, just revert this commit as it is only an optimization. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Tue, Apr 04, 2023 at 09:23:21PM +0100, Peter Maydell wrote: > On Tue, 4 Apr 2023 at 20:13, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > 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? > > > 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! > > Would somebody also like to send the 'revert' patch, please? > I had that all ready to go, but my git send-email setup > seems to have mysteriously broken and I don't have time to > fix it this evening :-( Attached. > > This is the commit message I wrote: > > > Revert "memory: Optimize replay of guest mapping" > > This reverts commit 6da24341866fa940fd7d575788a2319514941c77 > ("memory: Optimize replay of guest mapping"). > > This change breaks the mps3-an547 board under TCG (and > probably other TCG boards using an IOMMU), which now > assert: > > $ ./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. > > This is because tcg_register_iommu_notifier() registers > an IOMMU notifier which covers the entire address space, > so the assertion added in this commit is not correct. > > For the 8.0 release, just revert this commit as it is > only an optimization. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > thanks > -- PMM >
On Tue, Apr 04, 2023 at 09:23:21PM +0100, Peter Maydell wrote: > On Tue, 4 Apr 2023 at 20:13, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > 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? > > > 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! > > Would somebody also like to send the 'revert' patch, please? Assuming it's just the technicality of sending it I sent it but it's night here, didn't test at all. Just check it's same as yours and we are good to go ;) > I had that all ready to go, but my git send-email setup > seems to have mysteriously broken and I don't have time to > fix it this evening :-( > > This is the commit message I wrote: > > > Revert "memory: Optimize replay of guest mapping" > > This reverts commit 6da24341866fa940fd7d575788a2319514941c77 > ("memory: Optimize replay of guest mapping"). > > This change breaks the mps3-an547 board under TCG (and > probably other TCG boards using an IOMMU), which now > assert: > > $ ./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. > > This is because tcg_register_iommu_notifier() registers > an IOMMU notifier which covers the entire address space, > so the assertion added in this commit is not correct. > > For the 8.0 release, just revert this commit as it is > only an optimization. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > thanks > -- PMM
>-----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(¬ifier->n, >> tcg_iommu_unmap_notify, >> IOMMU_NOTIFIER_UNMAP, >> 0, >> HWADDR_MAX, >> iommu_idx); >> memory_region_register_iommu_notifier(notifier->mr, ¬ifier->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
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 98a5c304a7..6b1de80e85 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3831,7 +3831,7 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) .domain_id = vtd_get_domain_id(s, &ce, vtd_as->pasid), }; - vtd_page_walk(s, &ce, 0, ~0ULL, &info, vtd_as->pasid); + vtd_page_walk(s, &ce, n->start, n->end, &info, vtd_as->pasid); } } else { trace_vtd_replay_ce_invalid(bus_n, PCI_SLOT(vtd_as->devfn), 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)); assert(n->iommu_idx >= 0 && n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr)); @@ -1923,7 +1924,6 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr) void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) { - MemoryRegion *mr = MEMORY_REGION(iommu_mr); IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr); hwaddr addr, granularity; IOMMUTLBEntry iotlb; @@ -1936,7 +1936,7 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) granularity = memory_region_iommu_get_min_page_size(iommu_mr); - for (addr = 0; addr < memory_region_size(mr); addr += granularity) { + for (addr = n->start; addr < n->end; addr += granularity) { iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx); if (iotlb.perm != IOMMU_NONE) { n->notify(n, &iotlb);