From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40374) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ait0i-00028a-KA for qemu-devel@nongnu.org; Wed, 23 Mar 2016 20:20:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ait0f-0008Rk-EA for qemu-devel@nongnu.org; Wed, 23 Mar 2016 20:20:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ait0f-0008Rf-6y for qemu-devel@nongnu.org; Wed, 23 Mar 2016 20:20:21 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id E54EAC049D7F for ; Thu, 24 Mar 2016 00:20:20 +0000 (UTC) From: Bandan Das References: <20160323170016.4561f075@t450s.home> Date: Wed, 23 Mar 2016 20:20:19 -0400 In-Reply-To: <20160323170016.4561f075@t450s.home> (Alex Williamson's message of "Wed, 23 Mar 2016 17:00:16 -0600") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org Alex Williamson writes: ... >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index fb588d8..9489ff3 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -323,7 +323,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >> { >> VFIOContainer *container = container_of(listener, VFIOContainer, listener); >> hwaddr iova, end; >> - Int128 llend; >> + Int128 llend, endaddr; >> void *vaddr; >> int ret; >> >> @@ -349,14 +349,15 @@ static void vfio_listener_region_add(MemoryListener *listener, >> if (int128_ge(int128_make64(iova), llend)) { >> return; >> } >> - end = int128_get64(llend); >> + endaddr = int128_sub(llend, int128_one()); > > I like where this is headed, but there's maybe a little polish left. > > endaddr here can never be greater than UINT64_MAX, so why don't we make > it a hwaddr and wrap the whole right side of the equation in an > int128_get64(). Then the (end - 1) changes below don't need the > extra int128 ops. Sounds good. >> >> - if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) { >> - error_report("vfio: IOMMU container %p can't map guest IOVA region" >> - " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, >> - container, iova, end - 1); >> - ret = -EFAULT; >> - goto fail; >> + if ((iova < container->min_iova) || (int128_get64(endaddr) > >> + container->max_iova)) { >> + error_report("vfio: IOMMU container %p can't map guest IOVA region" >> + " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, >> + container, iova, int128_get64(endaddr)); >> + ret = -EFAULT; >> + goto fail; > > Bogus whitespace changes Oops, sorry! >> } >> >> memory_region_ref(section->mr); >> @@ -364,7 +365,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >> if (memory_region_is_iommu(section->mr)) { >> VFIOGuestIOMMU *giommu; >> >> - trace_vfio_listener_region_add_iommu(iova, end - 1); >> + trace_vfio_listener_region_add_iommu(iova, int128_get64(endaddr)); >> /* >> * FIXME: We should do some checking to see if the >> * capabilities of the host VFIO IOMMU are adequate to model >> @@ -389,6 +390,8 @@ static void vfio_listener_region_add(MemoryListener *listener, >> return; >> } >> >> + end = int128_get64(llend); >> + >> /* Here we assume that memory_region_is_ram(section->mr)==true */ >> >> vaddr = memory_region_get_ram_ptr(section->mr) + > > For this last chunk I'd do something like this: > > + llsize = int128_sub(llend, int128_make64(iova)); > > - ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly); > + ret = vfio_dma_map(container, iova, > + int128_get64(llsize), vaddr, section->readonly); > if (ret) { > error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", " > "0x%"HWADDR_PRIx", %p) = %d (%m)", > - container, iova, end - iova, vaddr, ret); > + container, iova, int128_get64(llsize), vaddr, ret); > goto fail; > } > > This maintains the 128bit math until exactly the point where it will > fall apart when vfio_dma_map can't pass an int128 to the vfio API. Yep, sounds good. v2 coming up shortly. > Thanks, > > Alex