On Tue, 22 Mar 2016 14:55:14 -0400 Bandan Das <b...@redhat.com> wrote:
> 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. Seems like some sort of compatibility convention since memory_region_init() only takes a uint64_t as the size. memory.c interprets that as "oh, I know you really mean 2^64". > >> > 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(). A size of UINT64_MAX doesn't make much sense to me, that would mean the last address is UINT64_MAX - 1. A size of 2^64 means the last address is UINT64_MAX, which seems reasonable. > >> > 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! Is the MemoryRegion memory_region_is_iommu() such that you're calling vfio_dma_map() from vfio_iommu_map_notify()? If so then we should probably be using 128bit helpers for doing sanity checking and go ahead and let something assert if we get to the vfio_dma_map() in vfio_listener_region_add() with a 2^64 size. Then if you're taking the memory_region_is_iommu() path, vfio_dma_map() is going to be called with translations within that 2^64 bit address space, not mapping the entire space, right? Thanks, Alex