Re: [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions

[Alex Williamson]

On Wed, 23 Mar 2016 16:57:35 -0400
Bandan Das <address@hidden> wrote:

> vfio_listener_region_add for a iommu mr results in
> an overflow assert since vt-d memory region is initialized
> with UINT64_MAX. Convert calculations to 128 bit arithmetic
> for such regions and let int128_get64 assert for non iommu
> regions only.
> 
> Suggested-by: Alex Williamson <address@hidden>
> Signed-off-by: Bandan Das <address@hidden>
> ---
> This is v2 of http://patchwork.ozlabs.org/patch/600364/
>  hw/vfio/common.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fb588d8..9489ff3 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -323,7 +323,7 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>  {
>      VFIOContainer *container = container_of(listener, VFIOContainer, 
> listener);
>      hwaddr iova, end;
> -    Int128 llend;
> +    Int128 llend, endaddr;
>      void *vaddr;
>      int ret;
>  
> @@ -349,14 +349,15 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      if (int128_ge(int128_make64(iova), llend)) {
>          return;
>      }
> -    end = int128_get64(llend);
> +    endaddr = int128_sub(llend, int128_one());

I like where this is headed, but there's maybe a little polish left.

endaddr here can never be greater than UINT64_MAX, so why don't we make
it a hwaddr and wrap the whole right side of the equation in an
int128_get64().  Then the (end - 1) changes below don't need the
extra int128 ops.

>  
> -    if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
> -        error_report("vfio: IOMMU container %p can't map guest IOVA region"
> -                     " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> -                     container, iova, end - 1);
> -        ret = -EFAULT;
> -        goto fail;
> +    if ((iova < container->min_iova) || (int128_get64(endaddr) >
> +                                         container->max_iova)) {
> +            error_report("vfio: IOMMU container %p can't map guest IOVA 
> region"
> +                         " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> +                         container, iova, int128_get64(endaddr));
> +            ret = -EFAULT;
> +            goto fail;

Bogus whitespace changes

>      }
>  
>      memory_region_ref(section->mr);
> @@ -364,7 +365,7 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      if (memory_region_is_iommu(section->mr)) {
>          VFIOGuestIOMMU *giommu;
>  
> -        trace_vfio_listener_region_add_iommu(iova, end - 1);
> +        trace_vfio_listener_region_add_iommu(iova, int128_get64(endaddr));
>          /*
>           * FIXME: We should do some checking to see if the
>           * capabilities of the host VFIO IOMMU are adequate to model
> @@ -389,6 +390,8 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>          return;
>      }
>  
> +    end = int128_get64(llend);
> +
>      /* Here we assume that memory_region_is_ram(section->mr)==true */
>  
>      vaddr = memory_region_get_ram_ptr(section->mr) +

For this last chunk I'd do something like this:

+    llsize = int128_sub(llend, int128_make64(iova));
 
-    ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly);
+    ret = vfio_dma_map(container, iova,
+                       int128_get64(llsize), vaddr, section->readonly);
     if (ret) {
         error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
                      "0x%"HWADDR_PRIx", %p) = %d (%m)",
-                     container, iova, end - iova, vaddr, ret);
+                     container, iova, int128_get64(llsize), vaddr, ret);
         goto fail;
     }
 
This maintains the 128bit math until exactly the point where it will
fall apart when vfio_dma_map can't pass an int128 to the vfio API.
Thanks,

Alex