Alex Williamson <[email protected]> writes: > On Mon, 21 Mar 2016 20:06:32 -0400 > Bandan Das <[email protected]> wrote: > >> Alex Williamson <[email protected]> writes: >> >> > On Mon, 21 Mar 2016 18:00:50 -0400 >> > Bandan Das <[email protected]> wrote: >> > >> >> vfio_listener_region_add for a iommu mr results in >> >> an overflow assert since emulated iommu memory region is initialized >> >> with UINT64_MAX. Add a check just like memory_region_size() >> >> does. >> >> >> >> Signed-off-by: Bandan Das <[email protected]> >> >> --- >> >> hw/vfio/common.c | 7 ++++++- >> >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> >> index fb588d8..269244b 100644 >> >> --- a/hw/vfio/common.c >> >> +++ b/hw/vfio/common.c >> >> @@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener >> >> *listener, >> >> if (int128_ge(int128_make64(iova), llend)) { >> >> return; >> >> } >> >> - end = int128_get64(llend); >> >> + >> >> + if (int128_eq(llend, int128_2_64())) { >> >> + end = UINT64_MAX; >> >> + } else { >> >> + end = int128_get64(llend); >> >> + } >> >> >> >> if ((iova < container->min_iova) || ((end - 1) > >> >> container->max_iova)) { >> >> error_report("vfio: IOMMU container %p can't map guest IOVA >> >> region" >> > >> > But now all the calculations where we use end-1 are wrong. See the >> > discussion with Pierre Morel in the January qemu-devel archives. >> > There's a solution in there, but I never saw a follow-up from Pierre >> > with a revised patch. Thanks, >> >> I am missing something. When end < UIN64_MAX, end - 1 calculations are valid >> because >> the patch doesn't change that behavior. When end is UINT64_MAX, >> int128_get64() doesn't know how >> to calculate this value and we are just feeding it manually. The patch is >> just the opposite >> of what memory_region_init() did to init the mem region in the first place: >> 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. > 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. > 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. > 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. > likely the system page size. Thanks, > > Alex
