| Message ID | 20230214034211.683203-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: Optimize replay of guest mapping | expand |
On Tue, Feb 14, 2023 at 11:43 AM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote: > > 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. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > Tested only on x86 with a net card passed to guest, ping/ssh pass. > > hw/i386/intel_iommu.c | 2 +- > softmmu/memory.c | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 98a5c304a7d7..6b1de80e8573 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 9d64efca269b..f096716e6e78 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1923,7 +1923,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 +1935,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) { Is [n->start, n->end] guaranteed to be the subset of memory_region_size(mr)? Thanks > iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx); > if (iotlb.perm != IOMMU_NONE) { > n->notify(n, &iotlb); > -- > 2.25.1 > >
>-----Original Message----- >From: Jason Wang <jasowang@redhat.com> >Sent: Tuesday, February 14, 2023 2:25 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: qemu-devel@nongnu.org; mst@redhat.com; peterx@redhat.com; >pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net; >marcel.apfelbaum@gmail.com; david@redhat.com; philmd@linaro.org >Subject: Re: [PATCH] memory: Optimize replay of guest mapping > >On Tue, Feb 14, 2023 at 11:43 AM Zhenzhong Duan ><zhenzhong.duan@intel.com> wrote: >> >> 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. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> Tested only on x86 with a net card passed to guest, ping/ssh pass. >> >> hw/i386/intel_iommu.c | 2 +- >> softmmu/memory.c | 3 +-- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index >> 98a5c304a7d7..6b1de80e8573 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 >> 9d64efca269b..f096716e6e78 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1923,7 +1923,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 +1935,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) { > >Is [n->start, n->end] guaranteed to be the subset of memory_region_size(mr)? In current implementation it is. [n->start, n->end] of notifier is derived from iommu memory region's section which is a subset of iommu memory region itself. Thanks Zhenzhong
On Tue, Feb 14, 2023 at 07:04:56AM +0000, Duan, Zhenzhong wrote: > >> @@ -1936,7 +1935,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) { > > > >Is [n->start, n->end] guaranteed to be the subset of memory_region_size(mr)? > > In current implementation it is. > [n->start, n->end] of notifier is derived from iommu memory region's section > which is a subset of iommu memory region itself. Yes, currently it seems to be guaranteed by the callers assuming they're always doing the right thing. Maybe it'll worth it to have memory_region_register_iommu_notifier() assert properly to make sure it always hold true? The patch itself looks good here, thanks.
>-----Original Message----- >From: Peter Xu <peterx@redhat.com> >Sent: Wednesday, February 15, 2023 1:29 AM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: Jason Wang <jasowang@redhat.com>; qemu-devel@nongnu.org; >mst@redhat.com; pbonzini@redhat.com; richard.henderson@linaro.org; >eduardo@habkost.net; marcel.apfelbaum@gmail.com; david@redhat.com; >philmd@linaro.org >Subject: Re: [PATCH] memory: Optimize replay of guest mapping > >On Tue, Feb 14, 2023 at 07:04:56AM +0000, Duan, Zhenzhong wrote: >> >> @@ -1936,7 +1935,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) { >> > >> >Is [n->start, n->end] guaranteed to be the subset of >memory_region_size(mr)? >> >> In current implementation it is. >> [n->start, n->end] of notifier is derived from iommu memory region's >> section which is a subset of iommu memory region itself. > >Yes, currently it seems to be guaranteed by the callers assuming they're >always doing the right thing. > >Maybe it'll worth it to have memory_region_register_iommu_notifier() assert >properly to make sure it always hold true? Yea, good suggestion, will do in v2. Thanks Zhenzhong
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 98a5c304a7d7..6b1de80e8573 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 9d64efca269b..f096716e6e78 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1923,7 +1923,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 +1935,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);
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. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- Tested only on x86 with a net card passed to guest, ping/ssh pass. hw/i386/intel_iommu.c | 2 +- softmmu/memory.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)