Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier

From: Thierry Reding <thierry.reding@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: 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>,
	Dmitry Osipenko <digetx@gmail.com>,
	devicetree@vger.kernel.org, iommu@lists.linux-foundation.org,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
Date: Tue, 8 Jun 2021 18:51:40 +0200	[thread overview]
Message-ID: <YL+gHN4YZ4vt1vPk@orome.fritz.box> (raw)
In-Reply-To: <YLEgXyLBF8PEFNw2@orome.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 5090 bytes --]

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?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]