Re: [Qemu-devel] [PATCH] vfio: add check for memory region overflow condition

[Bandan Das <bsd@redhat.com>]

Alex Williamson <alex.williamson@redhat.com> writes:
...
>> >>    mr->size = int128_make64(size);
>> >>    if (size == UINT64_MAX) {
>> >>       mr->size = int128_2_64();
>> >>    }
>> >> So, end - 1 is still valid for end = UINT64_MAX, no ?  
>> >
>> > int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to
>> > @end is clearing altering the value.  If we had a range from zero to  
>> 
>> I thought in128_2_64 is the 128 bit representation of UINT64_MAX. The
>> if condition in memory_region_init doesn't make sense otherwise.
>
> 2^64 cannot be represented with a uint64_t, 2^64 - 1 can:
>
> int128_2_64 = 1_0000_0000_0000_0000h
> UINT64_MAX  =   ffff_ffff_ffff_ffffh

Thanks, understood this part. I still don't understand the if condition
in memory_region_init however. Unless, that function actually takes the
last address for the size parameter and in that case, it should be
UINT64_MAX-1 for a size of UINT64_MAX.

>> > int128_2_64() then the size of that region is int128_2_64().  If we
>> > alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end
>> > - 1 is off by one versus the case where we use the value directly.  
>> 
>> Ok, you mean something like:
>> int128_get64(int128_sub(int128_2_64(), int128_make64(1)));  for (end - 1) ?
>> But we still have to deal with (end - iova) when calling vfio_dmap_map().
>> int128_get64() will definitely assert for iova = 0. 
>
> I don't know that that's the most efficient way to handle it, but @end
> represents a different thing by imposing that -1 and it needs to be
> handled in the reset of the code.
>
>> > You're effectively changing @end to be the last address in the range,  
>> 
>> No, I think I am changing "end" to what we initally started with for size
>> before converting to 128 bit.
>
> Nope, it's the difference between the size of the region and the last
> address of the region.

Ok, but note that it's the "size" that actually asserts here since the
offset is 0. So, we started with a size UINT64_MAX but end with mr->size =
128_2_64().

>> > but only in some cases, and not adjusting the remaining code to match.
>> > Not only that, but the vfio map command is probably going to fail if we
>> > pass in such an unaligned size since the mapping granularity is  
>> 
>> Trying to map such a large region is wrong anyway, I am still trying
>> to workout a solution to avoid calling memory_region_init_iommu()
>> with UINT64_MAX which is what emulated vt-d currently does.
>
> Right, the address width of the IOMMU on x86 is typically nowhere near
> 2^64, so if you take the vfio_dma_map path, you'll surely explode.

And it does. If we fix this assert, then vfio_dma_map() attempts mapping
this direct mapped address range starting from 0 and prints a 
warning message; happens for the whole range and goes on for ever.
The overflow check seemed to me like something we should fix, but now
I am more confused then ever!

> Does this fix actually fix anything or just move us to the next
> assert?  Thanks,
>
> Alex