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

[Bandan Das]

Alex Williamson <address@hidden> writes:
...
>> 
>> 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.

Sounds good.

>>  
>> -    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

Oops, sorry!

>>      }
>>  
>>      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.

Yep, sounds good. v2 coming up shortly.

> Thanks,
>
> Alex