Re: [PATCH] softmmu/memory: Skip translation size instead of fixed granularity if translate() successfully

[David Hildenbrand]

On 16.04.22 10:34, chenxiang via wrote:
> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> Currently memory_region_iommu_replay() does full page table walk with
> fixed granularity (page size) no matter translate() succeeds or not.
> Actually if translate() successfully, we can skip translation size
> (iotlb.addr_mask + 1) instead of fixed granularity.
> 
>  Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> ---
>  softmmu/memory.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfa5d5178c..ccfa19cf71 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1924,7 +1924,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, granularity, def_granu;
>      IOMMUTLBEntry iotlb;
>  
>      /* If the IOMMU has its own replay callback, override */
> @@ -1933,12 +1933,15 @@ void memory_region_iommu_replay(IOMMUMemoryRegion 
> *iommu_mr, IOMMUNotifier *n)
>          return;
>      }
>  
> -    granularity = memory_region_iommu_get_min_page_size(iommu_mr);
> +    def_granu = memory_region_iommu_get_min_page_size(iommu_mr);

"granu" sounds weird. I'd suggest calling this "min_granularity".

>  
>      for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
>          iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx);
>          if (iotlb.perm != IOMMU_NONE) {
>              n->notify(n, &iotlb);
> +            granularity = iotlb.addr_mask + 1;
> +        } else {
> +            granularity = def_granu;
>          }

I do wonder if there are cases where we would actually have to round up
the addr instead of simply adding the granularity if we suddenly use
bigger steps then the min granularity.

i.e., there would be a difference between

addr += granularity

and

addr = QEMU_ALIGN_UP(addr + min_granularity, granularity);

I wonder if there are corner cases where translate() could fail on the
first part of a granularity increment but succeed on the second part. My
gut feeling is that it should be fine,

-- 
Thanks,

David / dhildenb