From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56048) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiRSc-0006Bf-5e for qemu-devel@nongnu.org; Tue, 22 Mar 2016 14:55:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiRSX-0000nE-6A for qemu-devel@nongnu.org; Tue, 22 Mar 2016 14:55:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46326) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiRSW-0000mr-UN for qemu-devel@nongnu.org; Tue, 22 Mar 2016 14:55:17 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 7E2B0AA0A9 for ; Tue, 22 Mar 2016 18:55:16 +0000 (UTC) From: Bandan Das References: <20160321163441.29684cbf@t450s.home> <20160321183052.59e87b29@ul30vt.home> <20160321201600.7073b441@ul30vt.home> Date: Tue, 22 Mar 2016 14:55:14 -0400 In-Reply-To: <20160321201600.7073b441@ul30vt.home> (Alex Williamson's message of "Mon, 21 Mar 2016 20:16:00 -0600") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] vfio: add check for memory region overflow condition List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel Alex Williamson 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