From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34873) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b65z2-0005jZ-3W for qemu-devel@nongnu.org; Thu, 26 May 2016 20:50:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b65yx-0002DS-Tx for qemu-devel@nongnu.org; Thu, 26 May 2016 20:50:35 -0400 Date: Fri, 27 May 2016 10:50:24 +1000 From: David Gibson Message-ID: <20160527005024.GM17226@voom.fritz.box> References: <1462344751-28281-1-git-send-email-aik@ozlabs.ru> <1462344751-28281-19-git-send-email-aik@ozlabs.ru> <20160513162634.278ad267@t450s.home> <95076137-0493-224b-54e8-71023e6da230@ozlabs.ru> <20160516142002.3b47a874@t450s.home> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="yPvoiCUUfIqTdDlV" Content-Disposition: inline In-Reply-To: <20160516142002.3b47a874@t450s.home> Subject: Re: [Qemu-devel] [PATCH qemu v16 18/19] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: Alexey Kardashevskiy , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Alexander Graf , Paolo Bonzini --yPvoiCUUfIqTdDlV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, May 16, 2016 at 02:20:02PM -0600, Alex Williamson wrote: > On Mon, 16 May 2016 14:52:41 +1000 > Alexey Kardashevskiy wrote: >=20 > > On 05/14/2016 08:26 AM, Alex Williamson wrote: > > > On Wed, 4 May 2016 16:52:30 +1000 > > > Alexey Kardashevskiy wrote: > > > =20 > > >> New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window managem= ent. > > >> This adds ability to VFIO common code to dynamically allocate/remove > > >> DMA windows in the host kernel when new VFIO container is added/remo= ved. > > >> > > >> This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_= add > > >> and adds just created IOMMU into the host IOMMU list; the opposite > > >> action is taken in vfio_listener_region_del. > > >> > > >> When creating a new window, this uses heuristic to decide on the TCE= table > > >> levels number. > > >> > > >> This should cause no guest visible change in behavior. > > >> > > >> Signed-off-by: Alexey Kardashevskiy > > >> --- > > >> Changes: > > >> v16: > > >> * used memory_region_iommu_get_page_sizes() in vfio_listener_region_= add() > > >> * enforced no intersections between windows > > >> > > >> v14: > > >> * new to the series > > >> --- > > >> hw/vfio/common.c | 133 ++++++++++++++++++++++++++++++++++++++++++++= ++++++----- > > >> trace-events | 2 + > > >> 2 files changed, 125 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > >> index 03daf88..bd2dee8 100644 > > >> --- a/hw/vfio/common.c > > >> +++ b/hw/vfio/common.c > > >> @@ -240,6 +240,18 @@ static int vfio_dma_map(VFIOContainer *containe= r, hwaddr iova, > > >> return -errno; > > >> } > > >> > > >> +static bool range_contains(hwaddr start, hwaddr end, hwaddr addr) > > >> +{ > > >> + return start <=3D addr && addr <=3D end; > > >> +} =20 > > > > > > a) If you want a "range_foo" function then put it in range.h > > > b) I suspect there are already range.h functions that can do this. > > > =20 > > >> + > > >> +static bool vfio_host_win_intersects(VFIOHostDMAWindow *hostwin, > > >> + hwaddr min_iova, hwaddr max_io= va) > > >> +{ > > >> + return range_contains(hostwin->min_iova, hostwin->min_iova, min= _iova) || > > >> + range_contains(min_iova, max_iova, hostwin->min_iova); > > >> +} =20 > > > > > > How is this different than ranges_overlap()? =20 > > >> + > > >> static VFIOHostDMAWindow *vfio_host_win_lookup(VFIOContainer *conta= iner, > > >> hwaddr min_iova, hwa= ddr max_iova) > > >> { > > >> @@ -279,6 +291,14 @@ static int vfio_host_win_add(VFIOContainer *con= tainer, > > >> return 0; > > >> } > > >> > > >> +static void vfio_host_win_del(VFIOContainer *container, hwaddr min_= iova) > > >> +{ > > >> + VFIOHostDMAWindow *hostwin =3D vfio_host_win_lookup(container, = min_iova, 1); > > >> + > > >> + g_assert(hostwin); =20 > > > > > > Handle the error please. =20 > >=20 > > Will this be enough? > >=20 > > if (!hostwin) { > > error_report("%s: Cannot delete missing window at %"HWADDR_PRI= x, > > __func__, min_iova); > > return; > > } >=20 > Better. I was really thinking to return error to the caller, but if > the caller has no return path, perhaps this is as good as we can do. > Expect that I will push back on any assert() calls added to vfio. In this case I think returning an error makes more sense. The caller understands the context and so can make reasonable error handling decisions. .. which will probably just be a fatal error, but it still makes more logical sense there. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --yPvoiCUUfIqTdDlV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXR5nQAAoJEGw4ysog2bOSsUIQAN7kT3NOQ1S7ValXH2SSwOpx wwYGrZq1pojcrPg87vt5yqw2xxPU4dtA6HdRfoBEcbkMtS8WLbEQo8Fv3hfL5wzC kg6dfqOuFkMRq67rgS8l9h6MYrVJuTCCyC0dADQLzrzPUuxEE8D/eI6HFyhEGWEP PPvWA5v1bdTnKk0WXU2YQdSsQqZYquyJHZEqhmfy98s3jaTs9vtvRCnSJUiAB9Zr 3T4H2yWbMO1SQK6YEjr9m3JT9dncyovhvM3zICxyfxbmGMqO2gAW5jQDH/vmMddA 6M3r2qa2ELmh0Aw2rPHh6JNI96LUdhso+6Nzj7OcCUK2ThWZ1E2Rdew+79W2Xmao p5oouWcHuq7bdC6CON2zneln4Twfzl/HPAIwPex55V8VYZn9JKcv70RBb2qclzIn SabmJqh7pvtFOmlYLLyYWlPwYjj3Q8TJCdkAbPmVwrpB3ANU4rL7Bj/3Vb/600Kl dbppHL/GJAK9JX7hdHVEwR/HJ3PYPEopGCfpmw/qGo8ObsiEX9nZMPXZrdkftR+z 8vnHKXoUsT4AbqhRGqW8uCieOYLA/f9DGYyYc6Cr4U6xoed8xScupBSGm0BBmBWN aIK+scJcW5lwBquSLbA4i3RGK0YPNTEHwkX2wsDpgRcSm7yjuR+Fc/2rW7YvclTQ oGJ/oCT2TC01zUafigG6 =pK5x -----END PGP SIGNATURE----- --yPvoiCUUfIqTdDlV--