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=-14.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 613DFC433FE for ; Fri, 3 Sep 2021 13:52:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 46823610CF for ; Fri, 3 Sep 2021 13:52:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243643AbhICNxH (ORCPT ); Fri, 3 Sep 2021 09:53:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58044 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235802AbhICNxG (ORCPT ); Fri, 3 Sep 2021 09:53:06 -0400 Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D2F1C061575; Fri, 3 Sep 2021 06:52:06 -0700 (PDT) Received: by mail-wm1-x333.google.com with SMTP id u26-20020a05600c441a00b002f66b2d8603so3840097wmn.4; Fri, 03 Sep 2021 06:52:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=EnQFsjLHfRSbkFFrk/jmLUJeQadp1wRiJmOtkE4t/kk=; b=jkJAo5lQn9a4GNUXMSWIELWuxBU/ABG/ha385tNsuDrOarAJTeFYheIY0edwCb8ssK mf9bgqVdEfeNG4PlPKuM5FgeiUTtNhKfyWd/Zj7hQP/Vh9sW27zOphWWnWJ1FoilyByW 8yTnxqYTF2bBDCuW3NQ3W99uBvCHfd7gchnLzeUAtKvN44YPPUt9rZGZwTjv+382Wl2K U93ldIidrn3NSnag5e4u0J5sA+q8/ukx9QZE5pTxFcSgEkWLhUwCacsJHbt1utB9r/HS oYWkkKcEyzuwFDxWbv1IdqGQ0ga2VbQgL/jOZ1dntkcfi1zNH8s6fhnWDYWz45HJ5Tb+ 38ng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=EnQFsjLHfRSbkFFrk/jmLUJeQadp1wRiJmOtkE4t/kk=; b=OszA466MmHnsKO6PfifjxZYM34WkoFMPOKED3SihMlXSmDIOkbivtXbg6T6bkdG26R K46tTmpg4BZBtEuXVZ6iZ6Vj4i2EqPYVY2bk06bKt2OjED8QveCNo//R6KqiFEx3wUHB gnANzDeElXmf1I06ldQidl7m2I928Xz99lxarHY9vfQ9PZqjGcMuvOlHhE7MrFSS+HcC gjaglvSiJb8lpuAozvdmRSlT/KXnMjSCzP0s/S2oArMjbhfTl9tbhk3eaeBr9BqwEcuo +PjPBEK1RwiQpgmarUSmxJoXcGUtZcmilHZaon2eJWC0Y5+teRFX2v/x4KzNm4Nw/LTm XiPg== X-Gm-Message-State: AOAM533LWRbrWmOFYsXCeS7vWwTlZPWz9D7C1hvukoXfotmDRKz7Ecmw bS6G+Tnknjn6aAfq1VXYdhE= X-Google-Smtp-Source: ABdhPJxhGux8I7mdFQiNJlgsW+oeFVK4fltNYu6AqoWwxxRLz/5LzJTgeng5pVpgiLZ0p/KuDWA0+A== X-Received: by 2002:a7b:c097:: with SMTP id r23mr512255wmh.114.1630677124641; Fri, 03 Sep 2021 06:52:04 -0700 (PDT) Received: from localhost ([217.111.27.204]) by smtp.gmail.com with ESMTPSA id r129sm4219932wmr.7.2021.09.03.06.52.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Sep 2021 06:52:03 -0700 (PDT) Date: Fri, 3 Sep 2021 15:52:02 +0200 From: Thierry Reding To: Rob Herring Cc: Alyssa Rosenzweig , Sven Peter , Dmitry Osipenko , Joerg Roedel , Will Deacon , Robin Murphy , Nicolin Chen , Krishna Reddy , devicetree@vger.kernel.org, Linux IOMMU , linux-tegra , dri-devel Subject: Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier Message-ID: References: <20210423163234.3651547-1-thierry.reding@gmail.com> <20210423163234.3651547-2-thierry.reding@gmail.com> <20210520220306.GA1976116@robh.at.kernel.org> <7995b0ed-a277-ced1-b3d0-e0e7e02817a6@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9tMQdE4aESM5mGnn" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.1.2 (9a92dba0) (2021-08-24) Precedence: bulk List-ID: X-Mailing-List: linux-tegra@vger.kernel.org --9tMQdE4aESM5mGnn Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote: > On Wed, Sep 1, 2021 at 9:13 AM Thierry Reding = wrote: > > > > On Fri, Jul 02, 2021 at 05:16:25PM +0300, Dmitry Osipenko wrote: > > > 01.07.2021 21:14, Thierry Reding =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > > > On Tue, Jun 08, 2021 at 06:51:40PM +0200, Thierry Reding wrote: > > > >> On Fri, May 28, 2021 at 06:54:55PM +0200, Thierry Reding wrote: > > > >>> On Thu, May 20, 2021 at 05:03:06PM -0500, Rob Herring wrote: > > > >>>> On Fri, Apr 23, 2021 at 06:32:30PM +0200, Thierry Reding wrote: > > > >>>>> From: Thierry Reding > > > >>>>> > > > >>>>> Reserved memory region phandle references can be accompanied by= a > > > >>>>> specifier that provides additional information about how that s= pecific > > > >>>>> reference should be treated. > > > >>>>> > > > >>>>> One use-case is to mark a memory region as needing an identity = mapping > > > >>>>> in the system's IOMMU for the device that references the region= =2E This is > > > >>>>> needed for example when the bootloader has set up hardware (suc= h as a > > > >>>>> display controller) to actively access a memory region (e.g. a = boot > > > >>>>> splash screen framebuffer) during boot. The operating system ca= n use the > > > >>>>> identity mapping flag from the specifier to make sure an IOMMU = identity > > > >>>>> mapping is set up for the framebuffer before IOMMU translations= are > > > >>>>> enabled for the display controller. > > > >>>>> > > > >>>>> Signed-off-by: Thierry Reding > > > >>>>> --- > > > >>>>> .../reserved-memory/reserved-memory.txt | 21 +++++++++++= ++++++++ > > > >>>>> include/dt-bindings/reserved-memory.h | 8 +++++++ > > > >>>>> 2 files changed, 29 insertions(+) > > > >>>>> create mode 100644 include/dt-bindings/reserved-memory.h > > > >>>> > > > >>>> Sorry for being slow on this. I have 2 concerns. > > > >>>> > > > >>>> First, this creates an ABI issue. A DT with cells in 'memory-reg= ion' > > > >>>> will not be understood by an existing OS. I'm less concerned abo= ut this > > > >>>> if we address that with a stable fix. (Though I'm pretty sure we= 've > > > >>>> naively added #?-cells in the past ignoring this issue.) > > > >>> > > > >>> A while ago I had proposed adding memory-region*s* as an alternat= ive > > > >>> name for memory-region to make the naming more consistent with ot= her > > > >>> types of properties (think clocks, resets, gpios, ...). If we add= ed > > > >>> that, we could easily differentiate between the "legacy" cases wh= ere > > > >>> no #memory-region-cells was allowed and the new cases where it wa= s. > > > >>> > > > >>>> Second, it could be the bootloader setting up the reserved regio= n. If a > > > >>>> node already has 'memory-region', then adding more regions is mo= re > > > >>>> complicated compared to adding new properties. And defining what= each > > > >>>> memory-region entry is or how many in schemas is impossible. > > > >>> > > > >>> It's true that updating the property gets a bit complicated, but = it's > > > >>> not exactly rocket science. We really just need to splice the arr= ay. I > > > >>> have a working implemention for this in U-Boot. > > > >>> > > > >>> For what it's worth, we could run into the same issue with any new > > > >>> property that we add. Even if we renamed this to iommu-memory-reg= ion, > > > >>> it's still possible that a bootloader may have to update this pro= perty > > > >>> if it already exists (it could be hard-coded in DT, or it could h= ave > > > >>> been added by some earlier bootloader or firmware). > > > >>> > > > >>>> Both could be addressed with a new property. Perhaps something l= ike > > > >>>> 'iommu-memory-region =3D <&phandle>;'. I think the 'iommu' prefi= x is > > > >>>> appropriate given this is entirely because of the IOMMU being in= the > > > >>>> mix. I might feel differently if we had other uses for cells, bu= t I > > > >>>> don't really see it in this case. > > > >>> > > > >>> I'm afraid that down the road we'll end up with other cases and t= hen we > > > >>> might proliferate a number of *-memory-region properties with var= ying > > > >>> prefixes. > > > >>> > > > >>> I am aware of one other case where we might need something like t= his: on > > > >>> some Tegra SoCs we have audio processors that will access memory = buffers > > > >>> using a DMA engine. These processors are booted from early firmwa= re > > > >>> using firmware from system memory. In order to avoid trashing the > > > >>> firmware, we need to reserve memory. We can do this using reserved > > > >>> memory nodes. However, the audio DMA engine also uses the SMMU, s= o we > > > >>> need to make sure that the firmware memory is marked as reserved = within > > > >>> the SMMU. This is similar to the identity mapping case, but not e= xactly > > > >>> the same. Instead of creating a 1:1 mapping, we just want that IO= VA > > > >>> region to be reserved (i.e. IOMMU_RESV_RESERVED instead of > > > >>> IOMMU_RESV_DIRECT{,_RELAXABLE}). > > > >>> > > > >>> That would also fall into the IOMMU domain, but we can't reuse the > > > >>> iommu-memory-region property for that because then we don't have = enough > > > >>> information to decide which type of reservation we need. > > > >>> > > > >>> We could obviously make iommu-memory-region take a specifier, but= we > > > >>> could just as well use memory-regions in that case since we have > > > >>> something more generic anyway. > > > >>> > > > >>> With the #memory-region-cells proposal, we can easily extend the = cell in > > > >>> the specifier with an additional MEMORY_REGION_IOMMU_RESERVE flag= to > > > >>> take that other use case into account. If we than also change to = the new > > > >>> memory-regions property name, we avoid the ABI issue (and we gain= a bit > > > >>> of consistency while at it). > > > >> > > > >> Ping? Rob, do you want me to add this second use-case to the patch > > > >> series to make it more obvious that this isn't just a one-off thin= g? Or > > > >> how do we proceed? > > > > > > > > Rob, given that additional use-case, do you want me to run with this > > > > proposal and send out an updated series? > > > > > > > > > What about variant with a "descriptor" properties that will describe > > > each region: > > > > > > fb_desc: display-framebuffer-memory-descriptor { > > > needs-identity-mapping; > > > } > > > > > > display@52400000 { > > > memory-region =3D <&fb ...>; > > > memory-region-descriptor =3D <&fb_desc ...>; > > > }; > > > > > > It could be a more flexible/extendible variant. > > > > This problem recently came up on #dri-devel again. Adding Alyssa and > > Sven who are facing a similar challenge on their work on Apple M1 (if I > > understood correctly). Also adding dri-devel for visibility since this > > is a very common problem for display in particular. > > > > On M1 the situation is slightly more complicated: the firmware will > > allocate a couple of buffers (including the framebuffer) in high memory > > (> 4 GiB) and use the IOMMU to map that into an IOVA region below 4 GiB > > so that the display hardware can access it. This makes it impossible to > > bypass the IOMMU like we do on other chips (in particular to work around > > the fault-by-default policy of the ARM SMMU driver). It also means that > > in addition to the simple reserved regions I mentioned we need for audio > > use-cases and identity mapping use-cases we need for display on Tegra, > > we now also need to be able to convey physical to IOVA mappings. > > > > Fitting the latter into the original proposal sounds difficult. A quick > > fix would've been to generate a mapping table in memory and pass that to > > the kernel using a reserved-memory node (similar to what's done for > > example on Tegra for the EMC frequency table on Tegra210) and mark it as > > such using a special flag. But that then involves two layers of parsing, > > which seems a bit suboptimal. Another way to shoehorn that into the > > original proposal would've been to add flags for physical and virtual > > address regions and use pairs to pass them using special flags. Again, > > this is a bit wonky because it needs these to be carefully parsed and > > matched up. > > > > Another downside is that we now have a situation where some of these > > regions are no longer "reserved-memory regions" in the traditional > > sense. This would require an additional flag in the reserved-memory > > region nodes to prevent the IOVA regions from being reserved. By the > > way, this is something that would also be needed for the audio use-case > > I mentioned before, because the physical memory at that address can > > still be used by an operating system. > > > > A more general solution would be to draw a bit from Dmitry's proposal > > and introduce a new top-level "iov-reserved-memory" node. This could be > > modelled on the existing reserved-memory node, except that the physical > > memory pages for regions represented by child nodes would not be marked > > as reserved. Only the IOVA range described by the region would be > > reserved subsequently by the IOMMU framework and/or IOMMU driver. > > > > The simplest case where we just want to reserve some IOVA region could > > then be done like this: > > > > iov-reserved-memory { > > /* > > * Probably safest to default to <2>, <2> here given > > * that most IOMMUs support either > 32 bits of IAS > > * or OAS. > > */ > > #address-cells =3D <2>; > > #size-cells =3D <2>; > > > > firmware: firmware@80000000 { > > reg =3D <0 0x80000000 0 0x01000000>; > > }; > > }; > > > > audio@30000000 { > > ... > > iov-memory-regions =3D <&firmware>; > > ... > > }; > > > > Mappings could be represented by an IOV reserved region taking a > > reference to the reserved-region that they map: > > > > reserved-memory { > > #address-cells =3D <2>; > > #size-cells =3D <2>; > > > > /* 16 MiB of framebuffer at top-of-memory */ > > framebuffer: framebuffer@1,ff000000 { > > reg =3D <0x1 0xff000000 0 0x01000000>; > > no-map; > > }; > > }; > > > > iov-reserved-memory { > > /* IOMMU supports only 32-bit output address space */ > > #address-cells =3D <1>; > > #size-cells =3D <1>; > > > > /* 16 MiB of framebuffer mapped to top of IOVA */ > > fb: fb@ff000000 { > > reg =3D <0 0xff000000 0 0x01000000>; > > memory-region =3D <&framebuffer>; > > }; > > }; > > > > display@40000000 { > > ... > > /* optional? */ > > memory-region =3D <&framebuffer>; > > iov-memory-regions =3D <&fb>; > > ... > > }; > > > > It's interesting how identity mapped regions now become a trivial > > special case of mappings. All that is needed is to make the reg property > > of the IOV reserved region correspond to the reg property of the normal > > reserved region. Alternatively, as a small optimization for lazy people > > like me, we could just allow these cases to omit the reg property and > > instead inherit it from the referenced reserved region. > > > > As the second example shows it might be convenient if memory-region > > could be derived from iov-memory-regions. This could be useful for cases > > where the driver wants to do something with the physical pages of the > > reserved region (such as mapping them and copying out the framebuffer > > data to another buffer so that the reserved memory can be recycled). If > > we have the IOV reserved region, we could provide an API to extract the > > physical reserved region (if it exists). That way we could avoid > > referencing it twice in DT. Then again, there's something elegant about > > the explicit second reference to. It indicates the intent that we may > > want to use the region for something other than just the IOV mapping. > > > > Anyway, this has been long enough. Let me know what you think. Alyssa, > > Sven, it'd be interesting to hear if you think this could work as a > > solution to the problem on M1. > > > > Rob, I think you might like this alternative because it basically gets > > rid of all the points in the original proposal that you were concerned > > about. Let me know what you think. >=20 > Couldn't we keep this all in /reserved-memory? Just add an iova > version of reg. Perhaps abuse 'assigned-address' for this purpose. The > issue I see would be handling reserved iova areas without a physical > area. That can be handled with just a iova and no reg. We already have > a no reg case. I had thought about that initially. One thing I'm worried about is that every child node in /reserved-memory will effectively cause the memory that it described to be reserved. But we don't want that for regions that are "virtual only" (i.e. IOMMU reservations). Obviously we can fix that in Linux, but what about other operating systems? Currently "reg" is a required property for statically allocated regions (which all of these would be). Do you have an idea of how widely that's used? What about other OSes, or bootloaders, what if they encounter these nodes that don't have a "reg" property? If you don't see any concerns with that, I think we could make it work. I don't have any strong opinions about the naming for the IOVA regions. With a tiny stretch of the imagination, "assigned-addresses" even makes sense in this context. Thierry --9tMQdE4aESM5mGnn Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmEyKH8ACgkQ3SOs138+ s6FqTBAAlKhmCG618Z1w4bfeFMC3a9Wyv09PXC/Tgl2xY+tEX4I1xKp8y5qzo7Ub 4AJDq9kDMt5uFBbUhOUA2W+eRVgKdfVRHkw2eBhO2+bvLv2dhNefJOT93usqTc4a 7FforsOLyXWWOvwD8garSjdEOU3np/2v4m4fH/htkLu7N5fkxqsUGfYIUW0rSzCN 6mchtWbK2LoCOr2p3SS2L2YK7QuG/vrzEhfrdwrdyIFhFLPhEmGTw6Ci2m2sRrA6 YHhy2EC7mC3ld1BvX+2wsA1GrGt+3NCslN6VYZO9II8dw30gPPs1zULBn2I9Nmcw PY9RmKj4vs3wcqp5uVK5IrVlyVNXySlDV/D+QbbRz3g4mJN4spvgBonel74dY5Nb +1NuRIIEvLZsrmDPVXoQ5SQ4w0M8+n6nWpLhAUz+di6zeOXtn26hf014tdyUVdH0 LaCuug2xAPZquskpRroCrNQ3TDAWio9EvsQiq2V4H+PAeKZjTR/UR0yioiMX5WjG h4xmPO9ktMWPTCv8Y7+q21xuPiCWW40S4sSrgjVsR+V4A+4puApQpNeGZjpM/poP 0SKqhXhvVh8zW+drydM0Cldw1ggehTeEqcqhFkQcjrhTDz16WaMmvnJTQujvdLbr nAGKeU9Z2K1CexpBddTEsv9E0q7MCzWcPmcv3SFkrbkpJGyEVPA= =NFNS -----END PGP SIGNATURE----- --9tMQdE4aESM5mGnn-- 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=-12.0 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham 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 097A8C433F5 for ; Fri, 3 Sep 2021 13:52:18 +0000 (UTC) Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (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 A0B28610CC for ; Fri, 3 Sep 2021 13:52:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A0B28610CC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.linux-foundation.org Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 694D64268F; Fri, 3 Sep 2021 13:52:17 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zJwsjTEJu_Tx; Fri, 3 Sep 2021 13:52:15 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id BED7242683; Fri, 3 Sep 2021 13:52:14 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8D88FC001A; Fri, 3 Sep 2021 13:52:14 +0000 (UTC) Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0110BC000E for ; Fri, 3 Sep 2021 13:52:12 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id CED5342683 for ; Fri, 3 Sep 2021 13:52:12 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 89Z8bHEsinrJ for ; Fri, 3 Sep 2021 13:52:08 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.8.0 Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by smtp4.osuosl.org (Postfix) with ESMTPS id 91570426B4 for ; Fri, 3 Sep 2021 13:52:06 +0000 (UTC) Received: by mail-wm1-x32a.google.com with SMTP id g138so3552871wmg.4 for ; Fri, 03 Sep 2021 06:52:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=EnQFsjLHfRSbkFFrk/jmLUJeQadp1wRiJmOtkE4t/kk=; b=jkJAo5lQn9a4GNUXMSWIELWuxBU/ABG/ha385tNsuDrOarAJTeFYheIY0edwCb8ssK mf9bgqVdEfeNG4PlPKuM5FgeiUTtNhKfyWd/Zj7hQP/Vh9sW27zOphWWnWJ1FoilyByW 8yTnxqYTF2bBDCuW3NQ3W99uBvCHfd7gchnLzeUAtKvN44YPPUt9rZGZwTjv+382Wl2K U93ldIidrn3NSnag5e4u0J5sA+q8/ukx9QZE5pTxFcSgEkWLhUwCacsJHbt1utB9r/HS oYWkkKcEyzuwFDxWbv1IdqGQ0ga2VbQgL/jOZ1dntkcfi1zNH8s6fhnWDYWz45HJ5Tb+ 38ng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=EnQFsjLHfRSbkFFrk/jmLUJeQadp1wRiJmOtkE4t/kk=; b=YUUF6zj75eXpBpqkm4eFK7PxMUdOY272Tmc1FU4LNX7xZJJUQ1zbxIdat9jXD0rSos ALFtIW9tWRgJkcTBK2E/BILpHr0QbccYqZTXouN41681YMzW7OvqQvsKsw9nEDFxctRv 3iXJcDOz6Z6z1nVtIWqtpJ1DM06EI1CqiF10zFsQLIJQOwwcwD8OClGtNy4yHbp75ynU /22ymzcL3qPV4IUeDDRNiwdy1pMPwZ8wjlYbAi7JS72V/GtcX6kIEK83baibUYA+ZZ6X 1G1FbAYeTT2bLIqCZcKfWL+DOk5MWu6zYiTDTcnSq0aKv83d/uqlNFMTH9wOs6/FclMo zUZw== X-Gm-Message-State: AOAM532KhnUrZ6S5VzJZLJls0NllpAn6vEaHfd3WTQKf9qyxaeDS03k2 x4aI9TbtE22pLR9ZoFlkcJU= X-Google-Smtp-Source: ABdhPJxhGux8I7mdFQiNJlgsW+oeFVK4fltNYu6AqoWwxxRLz/5LzJTgeng5pVpgiLZ0p/KuDWA0+A== X-Received: by 2002:a7b:c097:: with SMTP id r23mr512255wmh.114.1630677124641; Fri, 03 Sep 2021 06:52:04 -0700 (PDT) Received: from localhost ([217.111.27.204]) by smtp.gmail.com with ESMTPSA id r129sm4219932wmr.7.2021.09.03.06.52.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Sep 2021 06:52:03 -0700 (PDT) Date: Fri, 3 Sep 2021 15:52:02 +0200 From: Thierry Reding To: Rob Herring Subject: Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier Message-ID: References: <20210423163234.3651547-1-thierry.reding@gmail.com> <20210423163234.3651547-2-thierry.reding@gmail.com> <20210520220306.GA1976116@robh.at.kernel.org> <7995b0ed-a277-ced1-b3d0-e0e7e02817a6@gmail.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.1.2 (9a92dba0) (2021-08-24) Cc: devicetree@vger.kernel.org, Robin Murphy , Linux IOMMU , dri-devel , Alyssa Rosenzweig , linux-tegra , Dmitry Osipenko , Will Deacon 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="===============1654067279305915727==" Errors-To: iommu-bounces@lists.linux-foundation.org Sender: "iommu" --===============1654067279305915727== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9tMQdE4aESM5mGnn" Content-Disposition: inline --9tMQdE4aESM5mGnn Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote: > On Wed, Sep 1, 2021 at 9:13 AM Thierry Reding = wrote: > > > > On Fri, Jul 02, 2021 at 05:16:25PM +0300, Dmitry Osipenko wrote: > > > 01.07.2021 21:14, Thierry Reding =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > > > On Tue, Jun 08, 2021 at 06:51:40PM +0200, Thierry Reding wrote: > > > >> On Fri, May 28, 2021 at 06:54:55PM +0200, Thierry Reding wrote: > > > >>> On Thu, May 20, 2021 at 05:03:06PM -0500, Rob Herring wrote: > > > >>>> On Fri, Apr 23, 2021 at 06:32:30PM +0200, Thierry Reding wrote: > > > >>>>> From: Thierry Reding > > > >>>>> > > > >>>>> Reserved memory region phandle references can be accompanied by= a > > > >>>>> specifier that provides additional information about how that s= pecific > > > >>>>> reference should be treated. > > > >>>>> > > > >>>>> One use-case is to mark a memory region as needing an identity = mapping > > > >>>>> in the system's IOMMU for the device that references the region= =2E This is > > > >>>>> needed for example when the bootloader has set up hardware (suc= h as a > > > >>>>> display controller) to actively access a memory region (e.g. a = boot > > > >>>>> splash screen framebuffer) during boot. The operating system ca= n use the > > > >>>>> identity mapping flag from the specifier to make sure an IOMMU = identity > > > >>>>> mapping is set up for the framebuffer before IOMMU translations= are > > > >>>>> enabled for the display controller. > > > >>>>> > > > >>>>> Signed-off-by: Thierry Reding > > > >>>>> --- > > > >>>>> .../reserved-memory/reserved-memory.txt | 21 +++++++++++= ++++++++ > > > >>>>> include/dt-bindings/reserved-memory.h | 8 +++++++ > > > >>>>> 2 files changed, 29 insertions(+) > > > >>>>> create mode 100644 include/dt-bindings/reserved-memory.h > > > >>>> > > > >>>> Sorry for being slow on this. I have 2 concerns. > > > >>>> > > > >>>> First, this creates an ABI issue. A DT with cells in 'memory-reg= ion' > > > >>>> will not be understood by an existing OS. I'm less concerned abo= ut this > > > >>>> if we address that with a stable fix. (Though I'm pretty sure we= 've > > > >>>> naively added #?-cells in the past ignoring this issue.) > > > >>> > > > >>> A while ago I had proposed adding memory-region*s* as an alternat= ive > > > >>> name for memory-region to make the naming more consistent with ot= her > > > >>> types of properties (think clocks, resets, gpios, ...). If we add= ed > > > >>> that, we could easily differentiate between the "legacy" cases wh= ere > > > >>> no #memory-region-cells was allowed and the new cases where it wa= s. > > > >>> > > > >>>> Second, it could be the bootloader setting up the reserved regio= n. If a > > > >>>> node already has 'memory-region', then adding more regions is mo= re > > > >>>> complicated compared to adding new properties. And defining what= each > > > >>>> memory-region entry is or how many in schemas is impossible. > > > >>> > > > >>> It's true that updating the property gets a bit complicated, but = it's > > > >>> not exactly rocket science. We really just need to splice the arr= ay. I > > > >>> have a working implemention for this in U-Boot. > > > >>> > > > >>> For what it's worth, we could run into the same issue with any new > > > >>> property that we add. Even if we renamed this to iommu-memory-reg= ion, > > > >>> it's still possible that a bootloader may have to update this pro= perty > > > >>> if it already exists (it could be hard-coded in DT, or it could h= ave > > > >>> been added by some earlier bootloader or firmware). > > > >>> > > > >>>> Both could be addressed with a new property. Perhaps something l= ike > > > >>>> 'iommu-memory-region =3D <&phandle>;'. I think the 'iommu' prefi= x is > > > >>>> appropriate given this is entirely because of the IOMMU being in= the > > > >>>> mix. I might feel differently if we had other uses for cells, bu= t I > > > >>>> don't really see it in this case. > > > >>> > > > >>> I'm afraid that down the road we'll end up with other cases and t= hen we > > > >>> might proliferate a number of *-memory-region properties with var= ying > > > >>> prefixes. > > > >>> > > > >>> I am aware of one other case where we might need something like t= his: on > > > >>> some Tegra SoCs we have audio processors that will access memory = buffers > > > >>> using a DMA engine. These processors are booted from early firmwa= re > > > >>> using firmware from system memory. In order to avoid trashing the > > > >>> firmware, we need to reserve memory. We can do this using reserved > > > >>> memory nodes. However, the audio DMA engine also uses the SMMU, s= o we > > > >>> need to make sure that the firmware memory is marked as reserved = within > > > >>> the SMMU. This is similar to the identity mapping case, but not e= xactly > > > >>> the same. Instead of creating a 1:1 mapping, we just want that IO= VA > > > >>> region to be reserved (i.e. IOMMU_RESV_RESERVED instead of > > > >>> IOMMU_RESV_DIRECT{,_RELAXABLE}). > > > >>> > > > >>> That would also fall into the IOMMU domain, but we can't reuse the > > > >>> iommu-memory-region property for that because then we don't have = enough > > > >>> information to decide which type of reservation we need. > > > >>> > > > >>> We could obviously make iommu-memory-region take a specifier, but= we > > > >>> could just as well use memory-regions in that case since we have > > > >>> something more generic anyway. > > > >>> > > > >>> With the #memory-region-cells proposal, we can easily extend the = cell in > > > >>> the specifier with an additional MEMORY_REGION_IOMMU_RESERVE flag= to > > > >>> take that other use case into account. If we than also change to = the new > > > >>> memory-regions property name, we avoid the ABI issue (and we gain= a bit > > > >>> of consistency while at it). > > > >> > > > >> Ping? Rob, do you want me to add this second use-case to the patch > > > >> series to make it more obvious that this isn't just a one-off thin= g? Or > > > >> how do we proceed? > > > > > > > > Rob, given that additional use-case, do you want me to run with this > > > > proposal and send out an updated series? > > > > > > > > > What about variant with a "descriptor" properties that will describe > > > each region: > > > > > > fb_desc: display-framebuffer-memory-descriptor { > > > needs-identity-mapping; > > > } > > > > > > display@52400000 { > > > memory-region =3D <&fb ...>; > > > memory-region-descriptor =3D <&fb_desc ...>; > > > }; > > > > > > It could be a more flexible/extendible variant. > > > > This problem recently came up on #dri-devel again. Adding Alyssa and > > Sven who are facing a similar challenge on their work on Apple M1 (if I > > understood correctly). Also adding dri-devel for visibility since this > > is a very common problem for display in particular. > > > > On M1 the situation is slightly more complicated: the firmware will > > allocate a couple of buffers (including the framebuffer) in high memory > > (> 4 GiB) and use the IOMMU to map that into an IOVA region below 4 GiB > > so that the display hardware can access it. This makes it impossible to > > bypass the IOMMU like we do on other chips (in particular to work around > > the fault-by-default policy of the ARM SMMU driver). It also means that > > in addition to the simple reserved regions I mentioned we need for audio > > use-cases and identity mapping use-cases we need for display on Tegra, > > we now also need to be able to convey physical to IOVA mappings. > > > > Fitting the latter into the original proposal sounds difficult. A quick > > fix would've been to generate a mapping table in memory and pass that to > > the kernel using a reserved-memory node (similar to what's done for > > example on Tegra for the EMC frequency table on Tegra210) and mark it as > > such using a special flag. But that then involves two layers of parsing, > > which seems a bit suboptimal. Another way to shoehorn that into the > > original proposal would've been to add flags for physical and virtual > > address regions and use pairs to pass them using special flags. Again, > > this is a bit wonky because it needs these to be carefully parsed and > > matched up. > > > > Another downside is that we now have a situation where some of these > > regions are no longer "reserved-memory regions" in the traditional > > sense. This would require an additional flag in the reserved-memory > > region nodes to prevent the IOVA regions from being reserved. By the > > way, this is something that would also be needed for the audio use-case > > I mentioned before, because the physical memory at that address can > > still be used by an operating system. > > > > A more general solution would be to draw a bit from Dmitry's proposal > > and introduce a new top-level "iov-reserved-memory" node. This could be > > modelled on the existing reserved-memory node, except that the physical > > memory pages for regions represented by child nodes would not be marked > > as reserved. Only the IOVA range described by the region would be > > reserved subsequently by the IOMMU framework and/or IOMMU driver. > > > > The simplest case where we just want to reserve some IOVA region could > > then be done like this: > > > > iov-reserved-memory { > > /* > > * Probably safest to default to <2>, <2> here given > > * that most IOMMUs support either > 32 bits of IAS > > * or OAS. > > */ > > #address-cells =3D <2>; > > #size-cells =3D <2>; > > > > firmware: firmware@80000000 { > > reg =3D <0 0x80000000 0 0x01000000>; > > }; > > }; > > > > audio@30000000 { > > ... > > iov-memory-regions =3D <&firmware>; > > ... > > }; > > > > Mappings could be represented by an IOV reserved region taking a > > reference to the reserved-region that they map: > > > > reserved-memory { > > #address-cells =3D <2>; > > #size-cells =3D <2>; > > > > /* 16 MiB of framebuffer at top-of-memory */ > > framebuffer: framebuffer@1,ff000000 { > > reg =3D <0x1 0xff000000 0 0x01000000>; > > no-map; > > }; > > }; > > > > iov-reserved-memory { > > /* IOMMU supports only 32-bit output address space */ > > #address-cells =3D <1>; > > #size-cells =3D <1>; > > > > /* 16 MiB of framebuffer mapped to top of IOVA */ > > fb: fb@ff000000 { > > reg =3D <0 0xff000000 0 0x01000000>; > > memory-region =3D <&framebuffer>; > > }; > > }; > > > > display@40000000 { > > ... > > /* optional? */ > > memory-region =3D <&framebuffer>; > > iov-memory-regions =3D <&fb>; > > ... > > }; > > > > It's interesting how identity mapped regions now become a trivial > > special case of mappings. All that is needed is to make the reg property > > of the IOV reserved region correspond to the reg property of the normal > > reserved region. Alternatively, as a small optimization for lazy people > > like me, we could just allow these cases to omit the reg property and > > instead inherit it from the referenced reserved region. > > > > As the second example shows it might be convenient if memory-region > > could be derived from iov-memory-regions. This could be useful for cases > > where the driver wants to do something with the physical pages of the > > reserved region (such as mapping them and copying out the framebuffer > > data to another buffer so that the reserved memory can be recycled). If > > we have the IOV reserved region, we could provide an API to extract the > > physical reserved region (if it exists). That way we could avoid > > referencing it twice in DT. Then again, there's something elegant about > > the explicit second reference to. It indicates the intent that we may > > want to use the region for something other than just the IOV mapping. > > > > Anyway, this has been long enough. Let me know what you think. Alyssa, > > Sven, it'd be interesting to hear if you think this could work as a > > solution to the problem on M1. > > > > Rob, I think you might like this alternative because it basically gets > > rid of all the points in the original proposal that you were concerned > > about. Let me know what you think. >=20 > Couldn't we keep this all in /reserved-memory? Just add an iova > version of reg. Perhaps abuse 'assigned-address' for this purpose. The > issue I see would be handling reserved iova areas without a physical > area. That can be handled with just a iova and no reg. We already have > a no reg case. I had thought about that initially. One thing I'm worried about is that every child node in /reserved-memory will effectively cause the memory that it described to be reserved. But we don't want that for regions that are "virtual only" (i.e. IOMMU reservations). Obviously we can fix that in Linux, but what about other operating systems? Currently "reg" is a required property for statically allocated regions (which all of these would be). Do you have an idea of how widely that's used? What about other OSes, or bootloaders, what if they encounter these nodes that don't have a "reg" property? If you don't see any concerns with that, I think we could make it work. I don't have any strong opinions about the naming for the IOVA regions. With a tiny stretch of the imagination, "assigned-addresses" even makes sense in this context. Thierry --9tMQdE4aESM5mGnn Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmEyKH8ACgkQ3SOs138+ s6FqTBAAlKhmCG618Z1w4bfeFMC3a9Wyv09PXC/Tgl2xY+tEX4I1xKp8y5qzo7Ub 4AJDq9kDMt5uFBbUhOUA2W+eRVgKdfVRHkw2eBhO2+bvLv2dhNefJOT93usqTc4a 7FforsOLyXWWOvwD8garSjdEOU3np/2v4m4fH/htkLu7N5fkxqsUGfYIUW0rSzCN 6mchtWbK2LoCOr2p3SS2L2YK7QuG/vrzEhfrdwrdyIFhFLPhEmGTw6Ci2m2sRrA6 YHhy2EC7mC3ld1BvX+2wsA1GrGt+3NCslN6VYZO9II8dw30gPPs1zULBn2I9Nmcw PY9RmKj4vs3wcqp5uVK5IrVlyVNXySlDV/D+QbbRz3g4mJN4spvgBonel74dY5Nb +1NuRIIEvLZsrmDPVXoQ5SQ4w0M8+n6nWpLhAUz+di6zeOXtn26hf014tdyUVdH0 LaCuug2xAPZquskpRroCrNQ3TDAWio9EvsQiq2V4H+PAeKZjTR/UR0yioiMX5WjG h4xmPO9ktMWPTCv8Y7+q21xuPiCWW40S4sSrgjVsR+V4A+4puApQpNeGZjpM/poP 0SKqhXhvVh8zW+drydM0Cldw1ggehTeEqcqhFkQcjrhTDz16WaMmvnJTQujvdLbr nAGKeU9Z2K1CexpBddTEsv9E0q7MCzWcPmcv3SFkrbkpJGyEVPA= =NFNS -----END PGP SIGNATURE----- --9tMQdE4aESM5mGnn-- --===============1654067279305915727== 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 --===============1654067279305915727==--