Alex Williamson <alex.william...@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