* [PATCH v3] memory: Optimize replay of guest mapping @ 2023-04-13 11:00 Zhenzhong Duan 2023-04-13 18:53 ` Peter Xu 2023-04-18 10:13 ` Peter Maydell 0 siblings, 2 replies; 6+ messages in thread From: Zhenzhong Duan @ 2023-04-13 11:00 UTC (permalink / raw) To: qemu-devel Cc: mst, peterx, jasowang, marcel.apfelbaum, pbonzini, richard.henderson, eduardo, david, philmd, peter.maydell On x86, there are two notifiers registered due to vtd-ir memory region splitting the entire address space. During replay of the address space for each notifier, the whole address space is scanned which is unnecessary. We only need to scan the space belong to notifier monitored space. While on x86 IOMMU memory region spans over entire address space, but on some other platforms(e.g. arm mps3-an547), IOMMU memory region is only a window in the whole address space. user could register a notifier with arbitrary scope beyond IOMMU memory region. Though in current implementation replay is only triggered by VFIO and dirty page sync with notifiers derived from memory region section, but this isn't guaranteed in the future. So, we replay the intersection part of IOMMU memory region and IOMMU notifier in memory_region_iommu_replay(). Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- v3: Fix assert failure on mps3-an547 v2: Add an assert per Peter Tested on x86 with a net card passed to guest(kvm/tcg), ping/ssh pass. Also did simple bootup test with mps3-an547 hw/i386/intel_iommu.c | 2 +- softmmu/memory.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index a62896759c78..faade7def867 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3850,7 +3850,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 b1a6cae6f583..f7af691991de 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1925,7 +1925,7 @@ 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; + hwaddr addr, end, granularity; IOMMUTLBEntry iotlb; /* If the IOMMU has its own replay callback, override */ @@ -1935,8 +1935,9 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) } granularity = memory_region_iommu_get_min_page_size(iommu_mr); + end = MIN(n->end, memory_region_size(mr)); - for (addr = 0; addr < memory_region_size(mr); addr += granularity) { + for (addr = n->start; addr < end; addr += granularity) { iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx); if (iotlb.perm != IOMMU_NONE) { n->notify(n, &iotlb); -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] memory: Optimize replay of guest mapping 2023-04-13 11:00 [PATCH v3] memory: Optimize replay of guest mapping Zhenzhong Duan @ 2023-04-13 18:53 ` Peter Xu 2023-04-18 10:13 ` Peter Maydell 1 sibling, 0 replies; 6+ messages in thread From: Peter Xu @ 2023-04-13 18:53 UTC (permalink / raw) To: Zhenzhong Duan Cc: qemu-devel, mst, jasowang, marcel.apfelbaum, pbonzini, richard.henderson, eduardo, david, philmd, peter.maydell On Thu, Apr 13, 2023 at 07:00:19PM +0800, Zhenzhong Duan wrote: > On x86, there are two notifiers registered due to vtd-ir memory > region splitting the entire address space. During replay of the > address space for each notifier, the whole address space is > scanned which is unnecessary. We only need to scan the space > belong to notifier monitored space. > > While on x86 IOMMU memory region spans over entire address space, > but on some other platforms(e.g. arm mps3-an547), IOMMU memory > region is only a window in the whole address space. user could > register a notifier with arbitrary scope beyond IOMMU memory > region. Though in current implementation replay is only triggered > by VFIO and dirty page sync with notifiers derived from memory > region section, but this isn't guaranteed in the future. > > So, we replay the intersection part of IOMMU memory region and > IOMMU notifier in memory_region_iommu_replay(). > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] memory: Optimize replay of guest mapping 2023-04-13 11:00 [PATCH v3] memory: Optimize replay of guest mapping Zhenzhong Duan 2023-04-13 18:53 ` Peter Xu @ 2023-04-18 10:13 ` Peter Maydell 2023-04-18 13:56 ` Peter Xu 1 sibling, 1 reply; 6+ messages in thread From: Peter Maydell @ 2023-04-18 10:13 UTC (permalink / raw) To: Zhenzhong Duan Cc: qemu-devel, mst, peterx, jasowang, marcel.apfelbaum, pbonzini, richard.henderson, eduardo, david, philmd On Thu, 13 Apr 2023 at 12:12, Zhenzhong Duan <zhenzhong.duan@intel.com> wrote: > > On x86, there are two notifiers registered due to vtd-ir memory > region splitting the entire address space. During replay of the > address space for each notifier, the whole address space is > scanned which is unnecessary. We only need to scan the space > belong to notifier monitored space. > > While on x86 IOMMU memory region spans over entire address space, > but on some other platforms(e.g. arm mps3-an547), IOMMU memory > region is only a window in the whole address space. user could > register a notifier with arbitrary scope beyond IOMMU memory > region. Though in current implementation replay is only triggered > by VFIO and dirty page sync with notifiers derived from memory > region section, but this isn't guaranteed in the future. > > So, we replay the intersection part of IOMMU memory region and > IOMMU notifier in memory_region_iommu_replay(). > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > v3: Fix assert failure on mps3-an547 > v2: Add an assert per Peter > Tested on x86 with a net card passed to guest(kvm/tcg), ping/ssh pass. > Also did simple bootup test with mps3-an547 > > hw/i386/intel_iommu.c | 2 +- > softmmu/memory.c | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index a62896759c78..faade7def867 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3850,7 +3850,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 b1a6cae6f583..f7af691991de 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1925,7 +1925,7 @@ 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; > + hwaddr addr, end, granularity; > IOMMUTLBEntry iotlb; > > /* If the IOMMU has its own replay callback, override */ > @@ -1935,8 +1935,9 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) > } > > granularity = memory_region_iommu_get_min_page_size(iommu_mr); > + end = MIN(n->end, memory_region_size(mr)); > > - for (addr = 0; addr < memory_region_size(mr); addr += granularity) { > + for (addr = n->start; addr < end; addr += granularity) { > iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx); > if (iotlb.perm != IOMMU_NONE) { > n->notify(n, &iotlb); The documentation for the replay method of IOMMUMemoryRegionClass says: * The default implementation of memory_region_iommu_replay() is to * call the IOMMU translate method for every page in the address space * with flag == IOMMU_NONE and then call the notifier if translate * returns a valid mapping. If this method is implemented then it * overrides the default behaviour, and must provide the full semantics * of memory_region_iommu_replay(), by calling @notifier for every * translation present in the IOMMU. This commit changes the default implementation so it's no longer doing this for every page in the address space. If the change is correct, we should update the doc comment too. Oddly, the doc comment for memory_region_iommu_replay() itself doesn't very clearly state what its semantics are; it could probably be improved. Anyway, this change is OK for the TCG use of iommu notifiers, because that doesn't care about replay. thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] memory: Optimize replay of guest mapping 2023-04-18 10:13 ` Peter Maydell @ 2023-04-18 13:56 ` Peter Xu 2023-04-19 2:38 ` Duan, Zhenzhong 0 siblings, 1 reply; 6+ messages in thread From: Peter Xu @ 2023-04-18 13:56 UTC (permalink / raw) To: Peter Maydell Cc: Zhenzhong Duan, qemu-devel, mst, jasowang, marcel.apfelbaum, pbonzini, richard.henderson, eduardo, david, philmd On Tue, Apr 18, 2023 at 11:13:57AM +0100, Peter Maydell wrote: > On Thu, 13 Apr 2023 at 12:12, Zhenzhong Duan <zhenzhong.duan@intel.com> wrote: > > > > On x86, there are two notifiers registered due to vtd-ir memory > > region splitting the entire address space. During replay of the > > address space for each notifier, the whole address space is > > scanned which is unnecessary. We only need to scan the space > > belong to notifier monitored space. > > > > While on x86 IOMMU memory region spans over entire address space, > > but on some other platforms(e.g. arm mps3-an547), IOMMU memory > > region is only a window in the whole address space. user could > > register a notifier with arbitrary scope beyond IOMMU memory > > region. Though in current implementation replay is only triggered > > by VFIO and dirty page sync with notifiers derived from memory > > region section, but this isn't guaranteed in the future. > > > > So, we replay the intersection part of IOMMU memory region and > > IOMMU notifier in memory_region_iommu_replay(). > > > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > --- > > v3: Fix assert failure on mps3-an547 > > v2: Add an assert per Peter > > Tested on x86 with a net card passed to guest(kvm/tcg), ping/ssh pass. > > Also did simple bootup test with mps3-an547 > > > > hw/i386/intel_iommu.c | 2 +- > > softmmu/memory.c | 5 +++-- > > 2 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index a62896759c78..faade7def867 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -3850,7 +3850,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 b1a6cae6f583..f7af691991de 100644 > > --- a/softmmu/memory.c > > +++ b/softmmu/memory.c > > @@ -1925,7 +1925,7 @@ 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; > > + hwaddr addr, end, granularity; > > IOMMUTLBEntry iotlb; > > > > /* If the IOMMU has its own replay callback, override */ > > @@ -1935,8 +1935,9 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) > > } > > > > granularity = memory_region_iommu_get_min_page_size(iommu_mr); > > + end = MIN(n->end, memory_region_size(mr)); > > > > - for (addr = 0; addr < memory_region_size(mr); addr += granularity) { > > + for (addr = n->start; addr < end; addr += granularity) { > > iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx); > > if (iotlb.perm != IOMMU_NONE) { > > n->notify(n, &iotlb); > > > The documentation for the replay method of IOMMUMemoryRegionClass > says: > * The default implementation of memory_region_iommu_replay() is to > * call the IOMMU translate method for every page in the address space > * with flag == IOMMU_NONE and then call the notifier if translate > * returns a valid mapping. If this method is implemented then it > * overrides the default behaviour, and must provide the full semantics > * of memory_region_iommu_replay(), by calling @notifier for every > * translation present in the IOMMU. > > This commit changes the default implementation so it's no longer > doing this for every page in the address space. If the change is > correct, we should update the doc comment too. > > Oddly, the doc comment for memory_region_iommu_replay() itself > doesn't very clearly state what its semantics are; it could > probably be improved. > > Anyway, this change is OK for the TCG use of iommu notifiers, > because that doesn't care about replay. Since the notifier contains the range information I'd say the change shouldn't affect any caller but only a pure performance difference. Indeed it'll be nicer the documentation can be updated too. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3] memory: Optimize replay of guest mapping 2023-04-18 13:56 ` Peter Xu @ 2023-04-19 2:38 ` Duan, Zhenzhong 2023-04-19 9:49 ` Peter Maydell 0 siblings, 1 reply; 6+ messages in thread From: Duan, Zhenzhong @ 2023-04-19 2:38 UTC (permalink / raw) To: Peter Xu, Peter Maydell Cc: qemu-devel, mst, jasowang, marcel.apfelbaum, pbonzini, richard.henderson, eduardo, david, philmd >-----Original Message----- >From: Peter Xu <peterx@redhat.com> >Sent: Tuesday, April 18, 2023 9:56 PM >To: Peter Maydell <peter.maydell@linaro.org> >Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >devel@nongnu.org; mst@redhat.com; jasowang@redhat.com; >marcel.apfelbaum@gmail.com; pbonzini@redhat.com; >richard.henderson@linaro.org; eduardo@habkost.net; david@redhat.com; >philmd@linaro.org >Subject: Re: [PATCH v3] memory: Optimize replay of guest mapping > >On Tue, Apr 18, 2023 at 11:13:57AM +0100, Peter Maydell wrote: >> On Thu, 13 Apr 2023 at 12:12, Zhenzhong Duan ><zhenzhong.duan@intel.com> wrote: >> > >> > On x86, there are two notifiers registered due to vtd-ir memory >> > region splitting the entire address space. During replay of the >> > address space for each notifier, the whole address space is scanned >> > which is unnecessary. We only need to scan the space belong to >> > notifier monitored space. >> > >> > While on x86 IOMMU memory region spans over entire address space, >> > but on some other platforms(e.g. arm mps3-an547), IOMMU memory >> > region is only a window in the whole address space. user could >> > register a notifier with arbitrary scope beyond IOMMU memory region. >> > Though in current implementation replay is only triggered by VFIO >> > and dirty page sync with notifiers derived from memory region >> > section, but this isn't guaranteed in the future. >> > >> > So, we replay the intersection part of IOMMU memory region and IOMMU >> > notifier in memory_region_iommu_replay(). >> > >> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> > --- >> > v3: Fix assert failure on mps3-an547 >> > v2: Add an assert per Peter >> > Tested on x86 with a net card passed to guest(kvm/tcg), ping/ssh pass. >> > Also did simple bootup test with mps3-an547 >> > >> > hw/i386/intel_iommu.c | 2 +- >> > softmmu/memory.c | 5 +++-- >> > 2 files changed, 4 insertions(+), 3 deletions(-) >> > >> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index >> > a62896759c78..faade7def867 100644 >> > --- a/hw/i386/intel_iommu.c >> > +++ b/hw/i386/intel_iommu.c >> > @@ -3850,7 +3850,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 >> > b1a6cae6f583..f7af691991de 100644 >> > --- a/softmmu/memory.c >> > +++ b/softmmu/memory.c >> > @@ -1925,7 +1925,7 @@ 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; >> > + hwaddr addr, end, granularity; >> > IOMMUTLBEntry iotlb; >> > >> > /* If the IOMMU has its own replay callback, override */ @@ >> > -1935,8 +1935,9 @@ void >memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, >IOMMUNotifier *n) >> > } >> > >> > granularity = memory_region_iommu_get_min_page_size(iommu_mr); >> > + end = MIN(n->end, memory_region_size(mr)); >> > >> > - for (addr = 0; addr < memory_region_size(mr); addr += granularity) { >> > + for (addr = n->start; addr < end; addr += granularity) { >> > iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n- >>iommu_idx); >> > if (iotlb.perm != IOMMU_NONE) { >> > n->notify(n, &iotlb); >> >> >> The documentation for the replay method of IOMMUMemoryRegionClass >> says: >> * The default implementation of memory_region_iommu_replay() is to >> * call the IOMMU translate method for every page in the address space >> * with flag == IOMMU_NONE and then call the notifier if translate >> * returns a valid mapping. If this method is implemented then it >> * overrides the default behaviour, and must provide the full semantics >> * of memory_region_iommu_replay(), by calling @notifier for every >> * translation present in the IOMMU. >> >> This commit changes the default implementation so it's no longer doing >> this for every page in the address space. If the change is correct, we >> should update the doc comment too. >> >> Oddly, the doc comment for memory_region_iommu_replay() itself doesn't >> very clearly state what its semantics are; it could probably be >> improved. >> >> Anyway, this change is OK for the TCG use of iommu notifiers, because >> that doesn't care about replay. > >Since the notifier contains the range information I'd say the change shouldn't >affect any caller but only a pure performance difference. Indeed it'll be nicer >the documentation can be updated too. Thanks, > >-- >Peter Xu Thanks Peter Maydell and Peter Xu's comments, will add doc update. May I ask if it's preferred to add doc update to this patch or a separate patch? Regards Zhenzhong ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] memory: Optimize replay of guest mapping 2023-04-19 2:38 ` Duan, Zhenzhong @ 2023-04-19 9:49 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2023-04-19 9:49 UTC (permalink / raw) To: Duan, Zhenzhong Cc: Peter Xu, qemu-devel, mst, jasowang, marcel.apfelbaum, pbonzini, richard.henderson, eduardo, david, philmd On Wed, 19 Apr 2023 at 03:38, Duan, Zhenzhong <zhenzhong.duan@intel.com> wrote: > >> The documentation for the replay method of IOMMUMemoryRegionClass > >> says: > >> * The default implementation of memory_region_iommu_replay() is to > >> * call the IOMMU translate method for every page in the address space > >> * with flag == IOMMU_NONE and then call the notifier if translate > >> * returns a valid mapping. If this method is implemented then it > >> * overrides the default behaviour, and must provide the full semantics > >> * of memory_region_iommu_replay(), by calling @notifier for every > >> * translation present in the IOMMU. > >> > >> This commit changes the default implementation so it's no longer doing > >> this for every page in the address space. If the change is correct, we > >> should update the doc comment too. > >> > >> Oddly, the doc comment for memory_region_iommu_replay() itself doesn't > >> very clearly state what its semantics are; it could probably be > >> improved. > >> > >> Anyway, this change is OK for the TCG use of iommu notifiers, because > >> that doesn't care about replay. > > > >Since the notifier contains the range information I'd say the change shouldn't > >affect any caller but only a pure performance difference. Indeed it'll be nicer > >the documentation can be updated too. Thanks, > Thanks Peter Maydell and Peter Xu's comments, will add doc update. > May I ask if it's preferred to add doc update to this patch or a separate patch? I suggest doing it in this same patch. thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-19 9:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-13 11:00 [PATCH v3] memory: Optimize replay of guest mapping Zhenzhong Duan 2023-04-13 18:53 ` Peter Xu 2023-04-18 10:13 ` Peter Maydell 2023-04-18 13:56 ` Peter Xu 2023-04-19 2:38 ` Duan, Zhenzhong 2023-04-19 9:49 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.