From: Rob Herring <robh@kernel.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
Sven Peter <sven@svenpeter.dev>,
Dmitry Osipenko <digetx@gmail.com>,
Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
Robin Murphy <robin.murphy@arm.com>,
Nicolin Chen <nicolinc@nvidia.com>,
Krishna Reddy <vdumpa@nvidia.com>,
devicetree@vger.kernel.org,
Linux IOMMU <iommu@lists.linux-foundation.org>,
linux-tegra <linux-tegra@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
Date: Fri, 3 Sep 2021 08:20:55 -0500 [thread overview]
Message-ID: <CAL_Jsq+TQeb56UbrO1xKFSb1yo0d8U29DPynw3_jQ6gH6Peatw@mail.gmail.com> (raw)
In-Reply-To: <YS+Ke4Ip0InHSnHR@orome.fritz.box>
On Wed, Sep 1, 2021 at 9:13 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Fri, Jul 02, 2021 at 05:16:25PM +0300, Dmitry Osipenko wrote:
> > 01.07.2021 21:14, Thierry Reding пишет:
> > > 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 <treding@nvidia.com>
> > >>>>>
> > >>>>> Reserved memory region phandle references can be accompanied by a
> > >>>>> specifier that provides additional information about how that specific
> > >>>>> 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. This is
> > >>>>> needed for example when the bootloader has set up hardware (such as a
> > >>>>> display controller) to actively access a memory region (e.g. a boot
> > >>>>> splash screen framebuffer) during boot. The operating system can 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 <treding@nvidia.com>
> > >>>>> ---
> > >>>>> .../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-region'
> > >>>> will not be understood by an existing OS. I'm less concerned about 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 alternative
> > >>> name for memory-region to make the naming more consistent with other
> > >>> types of properties (think clocks, resets, gpios, ...). If we added
> > >>> that, we could easily differentiate between the "legacy" cases where
> > >>> no #memory-region-cells was allowed and the new cases where it was.
> > >>>
> > >>>> Second, it could be the bootloader setting up the reserved region. If a
> > >>>> node already has 'memory-region', then adding more regions is more
> > >>>> 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 array. 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-region,
> > >>> it's still possible that a bootloader may have to update this property
> > >>> if it already exists (it could be hard-coded in DT, or it could have
> > >>> been added by some earlier bootloader or firmware).
> > >>>
> > >>>> Both could be addressed with a new property. Perhaps something like
> > >>>> 'iommu-memory-region = <&phandle>;'. I think the 'iommu' prefix 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, but I
> > >>>> don't really see it in this case.
> > >>>
> > >>> I'm afraid that down the road we'll end up with other cases and then we
> > >>> might proliferate a number of *-memory-region properties with varying
> > >>> prefixes.
> > >>>
> > >>> I am aware of one other case where we might need something like this: on
> > >>> some Tegra SoCs we have audio processors that will access memory buffers
> > >>> using a DMA engine. These processors are booted from early firmware
> > >>> 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, so 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 exactly
> > >>> the same. Instead of creating a 1:1 mapping, we just want that IOVA
> > >>> 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 thing? 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 = <&fb ...>;
> > memory-region-descriptor = <&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 = <2>;
> #size-cells = <2>;
>
> firmware: firmware@80000000 {
> reg = <0 0x80000000 0 0x01000000>;
> };
> };
>
> audio@30000000 {
> ...
> iov-memory-regions = <&firmware>;
> ...
> };
>
> Mappings could be represented by an IOV reserved region taking a
> reference to the reserved-region that they map:
>
> reserved-memory {
> #address-cells = <2>;
> #size-cells = <2>;
>
> /* 16 MiB of framebuffer at top-of-memory */
> framebuffer: framebuffer@1,ff000000 {
> reg = <0x1 0xff000000 0 0x01000000>;
> no-map;
> };
> };
>
> iov-reserved-memory {
> /* IOMMU supports only 32-bit output address space */
> #address-cells = <1>;
> #size-cells = <1>;
>
> /* 16 MiB of framebuffer mapped to top of IOVA */
> fb: fb@ff000000 {
> reg = <0 0xff000000 0 0x01000000>;
> memory-region = <&framebuffer>;
> };
> };
>
> display@40000000 {
> ...
> /* optional? */
> memory-region = <&framebuffer>;
> iov-memory-regions = <&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.
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.
Rob
next prev parent reply other threads:[~2021-09-03 13:21 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-23 16:32 [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions Thierry Reding
2021-04-23 16:32 ` [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier Thierry Reding
2021-05-20 22:03 ` Rob Herring
2021-05-28 16:54 ` Thierry Reding
2021-06-08 16:51 ` Thierry Reding
2021-07-01 18:14 ` Thierry Reding
2021-07-02 14:16 ` Dmitry Osipenko
2021-09-01 14:13 ` Thierry Reding
2021-09-03 13:20 ` Rob Herring [this message]
2021-09-03 13:52 ` Thierry Reding
2021-09-03 14:36 ` Rob Herring
2021-09-03 15:35 ` Thierry Reding
2021-09-07 15:33 ` Rob Herring
2021-09-07 17:44 ` Thierry Reding
2021-09-15 15:19 ` Thierry Reding
2022-02-06 22:27 ` Janne Grunau
2022-02-09 16:31 ` Thierry Reding
2022-02-10 23:15 ` Janne Grunau
2022-03-31 16:25 ` Thierry Reding
2022-04-01 17:08 ` Janne Grunau
2021-04-23 16:32 ` [PATCH v2 2/5] iommu: Implement of_iommu_get_resv_regions() Thierry Reding
2021-07-02 14:05 ` Dmitry Osipenko
2021-07-16 14:41 ` Rob Herring
2021-07-17 11:07 ` Dmitry Osipenko
2021-07-30 12:18 ` Will Deacon
2021-04-23 16:32 ` [PATCH v2 3/5] iommu: dma: Use of_iommu_get_resv_regions() Thierry Reding
2021-04-23 16:32 ` [PATCH v2 4/5] iommu/tegra-smmu: Add support for reserved regions Thierry Reding
2021-04-23 16:32 ` [PATCH v2 5/5] iommu/tegra-smmu: Support managed domains Thierry Reding
2021-10-11 23:25 ` Dmitry Osipenko
2021-04-24 7:26 ` [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions Dmitry Osipenko
2021-04-27 18:30 ` Krishna Reddy
2021-04-28 5:44 ` Dmitry Osipenko
2021-04-29 5:51 ` Krishna Reddy
2021-04-29 12:43 ` Dmitry Osipenko
2021-04-28 5:51 ` Dmitry Osipenko
2021-04-28 5:57 ` Mikko Perttunen
2021-04-28 7:55 ` Dmitry Osipenko
2021-04-28 5:59 ` Dmitry Osipenko
2021-10-03 1:09 ` Dmitry Osipenko
2021-10-04 19:23 ` Thierry Reding
2021-10-04 20:32 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAL_Jsq+TQeb56UbrO1xKFSb1yo0d8U29DPynw3_jQ6gH6Peatw@mail.gmail.com \
--to=robh@kernel.org \
--cc=alyssa.rosenzweig@collabora.com \
--cc=devicetree@vger.kernel.org \
--cc=digetx@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=linux-tegra@vger.kernel.org \
--cc=nicolinc@nvidia.com \
--cc=robin.murphy@arm.com \
--cc=sven@svenpeter.dev \
--cc=thierry.reding@gmail.com \
--cc=vdumpa@nvidia.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).