Re: [PATCH v3] memory: Optimize replay of guest mapping

[Peter Xu]

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