From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1841AC433E0 for ; Fri, 5 Jun 2020 17:27:31 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DD21E2074B for ; Fri, 5 Jun 2020 17:27:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DD21E2074B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=iommu-bounces@lists.linux-foundation.org Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 9EC9724B3A; Fri, 5 Jun 2020 17:27:30 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5oZULAM-wdA6; Fri, 5 Jun 2020 17:27:28 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id DDB0E203D5; Fri, 5 Jun 2020 17:27:28 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id BA75DC0888; Fri, 5 Jun 2020 17:27:28 +0000 (UTC) Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 63642C016E for ; Fri, 5 Jun 2020 17:27:27 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 56E3788B2F for ; Fri, 5 Jun 2020 17:27:27 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id LyA34PUdd8gF for ; Fri, 5 Jun 2020 17:27:26 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by hemlock.osuosl.org (Postfix) with ESMTPS id D9BC088B26 for ; Fri, 5 Jun 2020 17:27:25 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id ACC14AFA8; Fri, 5 Jun 2020 17:27:25 +0000 (UTC) Message-ID: Subject: Re: [PATCH v3 09/13] device core: Introduce multiple dma pfn offsets From: Nicolas Saenz Julienne To: Jim Quinlan , Christoph Hellwig Date: Fri, 05 Jun 2020 19:27:16 +0200 In-Reply-To: References: <20200603192058.35296-1-james.quinlan@broadcom.com> <20200603192058.35296-10-james.quinlan@broadcom.com> <09c451e24f62e226e1ceaa0fe5d0a81109cace74.camel@suse.de> User-Agent: Evolution 3.36.2 MIME-Version: 1.0 Cc: Ulf Hansson , Rich Felker , "open list:SUPERH" , David Airlie , "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" , Hanjun Guo , "open list:REMOTE PROCESSOR \(REMOTEPROC\) SUBSYSTEM" , Andy Shevchenko , Bjorn Andersson , Julien Grall , "H. Peter Anvin" , Will Deacon , Christoph Hellwig , "open list:STAGING SUBSYSTEM" , Wolfram Sang , Yoshinori Sato , Frank Rowand , "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , Russell King , "open list:ACPI FOR ARM64 \(ACPI/arm64\)" , Chen-Yu Tsai , Ingo Molnar , "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" , Alan Stern , David Kershner , Len Brown , Ohad Ben-Cohen , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" , Arnd Bergmann , Suzuki K Poulose , Dan Williams , Maxime Ripard , Rob Herring , Borislav Petkov , "open list:DRM DRIVERS FOR ALLWINNER A10" , Yong Deng , Santosh Shilimkar , Bjorn Helgaas , Thomas Gleixner , Mauro Carvalho Chehab , "moderated list:ARM PORT" , Saravana Kannan , Greg Kroah-Hartman , Oliver Neukum , "Rafael J. Wysocki" , open list , Paul Kocialkowski , "open list:IOMMU DRIVERS" , Mark Brown , Stefano Stabellini , Daniel Vetter , Sudeep Holla , "open list:ALLWINNER A10 CSI DRIVER" , Robin Murphy , "open list:USB SUBSYSTEM" X-BeenThere: iommu@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Development issues for Linux IOMMU support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============6690933857139100339==" Errors-To: iommu-bounces@lists.linux-foundation.org Sender: "iommu" --===============6690933857139100339== Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-YwX3NY+MokiYMCDlDcff" --=-YwX3NY+MokiYMCDlDcff Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Christoph, a question arouse, is there a real value to dealing with PFNs (as opposed t= o real addresses) in the core DMA code/structures? I see that in some cases i= t eases interacting with mm, but the overwhelming usage of say, dev->dma_pfn_offset, involves shifting it. Hi Jim, On Thu, 2020-06-04 at 14:01 -0400, Jim Quinlan wrote: > Hi Nicolas, [...] > > I understand the need for dev to be around, devm_*() is key. But also i= t's > > important to keep the functions on purpose. And if of_dma_get_range() s= tarts > > setting ranges it calls, for the very least, for a function rename. Alt= hough > > I'd rather split the parsing and setting of ranges as mentioned earlier= . > > That > > said, I get that's a more drastic move. >=20 > I agree with you. I could do this from device.c: >=20 > of_dma_get_num_ranges(..., &num_ranges); /* new function */ > r =3D devm_kcalloc(dev, num_ranges + 1, sizeof(*r), GFP_KERNEL); > of_dma_get_range(np, &dma_addr, &paddr, &size, r, num_ranges); >=20 > The problem here is that there could be four ranges, all with > offset=3D0. My current code would optimize this case out but the above > would have us holding useless memory and looping through the four > ranges on every dma <=3D> phys conversion only to add 0. Point taken. Ultimately it's setting the device's dma ranges in of_dma_get_range() that was really bothering me, so if we have to pass the device pointer for allocations, be it. > > Talking about drastic moves. How about getting rid of the concept of > > dma_pfn_offset for drivers altogether. Let them provide > > dma_pfn_offset_regions > > (even when there is only one). I feel it's conceptually nicer, as you'd= be > > dealing only in one currency, so to speak, and you'd centralize the bus= DMA > > ranges setter function which is always easier to maintain. > Do you agree that we have to somehow hang this info on the struct > device structure? Because in the dma2phys() and phys2dma() all you > have is the dev parameter. I don't see how this can be done w/o > involving dev. Sorry I didn't make myself clear here. What bothers me is having two functi= ons setting the same device parameter trough different means, I'd be happy to g= et rid of attach_uniform_dma_pfn_offset(), and always use the same function to= set a device's bus dma regions. Something the likes of this comes to mind: dma_attach_pfn_offset_region(struct device *dev, struct dma_pfn_offset_regi= ons *r) We could maybe use some helper macros for the linear case. But that's the g= ist of it. Also, it goes hand in hand with the comment below. Why having a special cas= e for non sparse DMA offsets in struct dma_pfn_offset_regions? The way I see = it, in this case, code simplicity is more interesting than a small optimization= . > > I'd go as far as not creating a special case for uniform offsets. Let j= ust > > set > > cpu_end and dma_end to -1 so we always get a match. It's slightly more > > compute > > heavy, but I don't think it's worth the optimization. > Well, there are two subcases here. One where we do know the bounds > and one where we do not. I suppose for the latter I could have the > drivers calling it with begin=3D0 and end=3D~(dma_addr_t)0. Let me give > this some thought... >=20 > > Just my two cents :) >=20 > Worth much more than $0.02 IMO :-) BTW, would you consider renaming the DMA offset struct to something simpler like, struct bus_dma_region? It complements 'dev->bus_dma_limit' better IMO= . > BTW, I tried putting the "if (dev->dma_pfn_offset_map)" clause inside > the inline functions but the problem is that it slows the fastpath; > consider the following code from dma-direct.h >=20 > if (dev->dma_pfn_offset_map) { > unsigned long dma_pfn_offset =3D dma_pfn_offset_from_phys_addr(dev, paddr); >=20 > dev_addr -=3D ((dma_addr_t)dma_pfn_offset << PAGE_SHIFT); > } > return dev_addr; >=20 > becomes >=20 > unsigned long dma_pfn_offset =3D dma_pfn_offset_from_phys_addr(de= v, paddr); >=20 > dev_addr -=3D ((dma_addr_t)dma_pfn_offset << PAGE_SHIFT); > return dev_addr; >=20 > So those configurations that have no dma_pfn_offsets are doing an > unnecessary shift and add. Fair enough. Still not a huge difference, but I see the value being the mos= t common case. Regards, Nicolas --=-YwX3NY+MokiYMCDlDcff Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEErOkkGDHCg2EbPcGjlfZmHno8x/4FAl7agHQACgkQlfZmHno8 x/6D4wf8C6l0E7AUohntNHuqqtMhRrh3fE2ar9v2l+Hpq582gasP/VKacDmmQlhJ ux+9ZkUFYf9RO/CmM11DNpjVWI1skp2OuI4lSLaC7zneYFOwwqPEcgV0KvcfVFim su5CyLgwdHInfwIHrAsM09BZkFDKxtsBCvALQEX203VZB8HEC7XsZS3++KWL9bgy gbrqF4hs49vdcjVBbkL+wPY7rVZ6Xt8JJfNVx/VJQQtEyjz6Y2tpvRT8KBlZHJKZ +k9kV1dG9+UZ1Tw6kOvVrdKdjLODEu+ehKdSwuyyTNQFy6+nVcJoAKuWbPou69// CZzrN3RMBphApZhnJwGDXfe99/i3GQ== =HHS8 -----END PGP SIGNATURE----- --=-YwX3NY+MokiYMCDlDcff-- --===============6690933857139100339== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu --===============6690933857139100339==--