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

[Peter Maydell]

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