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

[Thierry Reding <thierry.reding@gmail.com>]

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

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?

Thierry

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