Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On 2022-03-31 18:25:05 +0200, Thierry Reding wrote: > On Fri, Feb 11, 2022 at 12:15:44AM +0100, Janne Grunau wrote: > > On 2022-02-09 17:31:16 +0100, Thierry Reding wrote: > > > On Sun, Feb 06, 2022 at 11:27:00PM +0100, Janne Grunau wrote: > > > > On 2021-09-15 17:19:39 +0200, Thierry Reding wrote: > > > > > On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote: > > > > > > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote: > > > > > > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding > > > > > > > wrote: > > > > > > > > > > > > > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > > > > > > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote: > > > > > > > > > > > > > > > > > > > > > > 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). > > > > > > > > > > > > > > > > > > By virtual only, you mean no physical mapping, just a region > > > > > > > > > of > > > > > > > > > virtual space, right? For that we'd have no 'reg' and > > > > > > > > > therefore no > > > > > > > > > (physical) reservation by the OS. It's similar to non-static > > > > > > > > > regions. > > > > > > > > > You need a specific handler for them. We'd probably want a > > > > > > > > > compatible > > > > > > > > > as well for these virtual reservations. > > > > > > > > > > > > > > > > Yeah, these would be purely used for reserving regions in the > > > > > > > > IOVA so > > > > > > > > that they won't be used by the IOVA allocator. Typically these > > > > > > > > would be > > > > > > > > used for cases where those addresses have some special meaning. > > > > > > > > > > > > > > > > Do we want something like: > > > > > > > > > > > > > > > > compatible = "iommu-reserved"; > > > > > > > > > > > > > > > > for these? Or would that need to be: > > > > > > > > > > > > > > > > compatible = "linux,iommu-reserved"; > > > > > > > > > > > > > > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix > > > > > > > > compatible strings in the reserved-memory DT bindings directory. > > > > > > > > > > > > > > I would not use 'linux,' here. > > > > > > > > > > > > > > > > > > > > > > > On the other hand, do we actually need the compatible string? > > > > > > > > Because we > > > > > > > > don't really want to associate much extra information with this > > > > > > > > like we > > > > > > > > do for example with "shared-dma-pool". The logic to handle this > > > > > > > > would > > > > > > > > all be within the IOMMU framework. All we really need is for the > > > > > > > > standard reservation code to skip nodes that don't have a reg > > > > > > > > property > > > > > > > > so we don't reserve memory for "virtual-only" allocations. > > > > > > > > > > > > > > It doesn't hurt to have one and I can imagine we might want to > > > > > > > iterate > > > > > > > over all the nodes. It's slightly easier and more common to > > > > > > > iterate > > > > > > > over compatible nodes rather than nodes with some property. > > > > > > > > > > > > > > > > Are these being global in DT going to be a problem? > > > > > > > > > Presumably we have > > > > > > > > > a virtual space per IOMMU. We'd know which IOMMU based on a > > > > > > > > > device's > > > > > > > > > 'iommus' and 'memory-region' properties, but within > > > > > > > > > /reserved-memory > > > > > > > > > we wouldn't be able to distinguish overlapping addresses from > > > > > > > > > separate > > > > > > > > > address spaces. Or we could have 2 different IOVAs for 1 > > > > > > > > > physical > > > > > > > > > space. That could be solved with something like this: > > > > > > > > > > > > > > > > > > iommu-addresses = <&iommu1 >; > > > > > > > > > > > > > > > > The only case that would be problematic would be if we have > > > > > > > > overlapping > > > > > > > > physical regions, because that will probably trip up the > > > > > > > > standard code. > > > > > > > > > > > > > > > > But this could also be worked around by looking at > > > > > > > > iommu-addresses. For > > > >
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Fri, Feb 11, 2022 at 12:15:44AM +0100, Janne Grunau wrote:
> On 2022-02-09 17:31:16 +0100, Thierry Reding wrote:
> > On Sun, Feb 06, 2022 at 11:27:00PM +0100, Janne Grunau wrote:
> > > On 2021-09-15 17:19:39 +0200, Thierry Reding wrote:
> > > > On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote:
> > > > > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote:
> > > > > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding
> > > > > > wrote:
> > > > > > >
> > > > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote:
> > > > > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote:
> > > > > > > > > >
> > > > > > > > > > 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).
> > > > > > > >
> > > > > > > > By virtual only, you mean no physical mapping, just a region of
> > > > > > > > virtual space, right? For that we'd have no 'reg' and therefore
> > > > > > > > no
> > > > > > > > (physical) reservation by the OS. It's similar to non-static
> > > > > > > > regions.
> > > > > > > > You need a specific handler for them. We'd probably want a
> > > > > > > > compatible
> > > > > > > > as well for these virtual reservations.
> > > > > > >
> > > > > > > Yeah, these would be purely used for reserving regions in the
> > > > > > > IOVA so
> > > > > > > that they won't be used by the IOVA allocator. Typically these
> > > > > > > would be
> > > > > > > used for cases where those addresses have some special meaning.
> > > > > > >
> > > > > > > Do we want something like:
> > > > > > >
> > > > > > > compatible = "iommu-reserved";
> > > > > > >
> > > > > > > for these? Or would that need to be:
> > > > > > >
> > > > > > > compatible = "linux,iommu-reserved";
> > > > > > >
> > > > > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix
> > > > > > > compatible strings in the reserved-memory DT bindings directory.
> > > > > >
> > > > > > I would not use 'linux,' here.
> > > > > >
> > > > > > >
> > > > > > > On the other hand, do we actually need the compatible string?
> > > > > > > Because we
> > > > > > > don't really want to associate much extra information with this
> > > > > > > like we
> > > > > > > do for example with "shared-dma-pool". The logic to handle this
> > > > > > > would
> > > > > > > all be within the IOMMU framework. All we really need is for the
> > > > > > > standard reservation code to skip nodes that don't have a reg
> > > > > > > property
> > > > > > > so we don't reserve memory for "virtual-only" allocations.
> > > > > >
> > > > > > It doesn't hurt to have one and I can imagine we might want to
> > > > > > iterate
> > > > > > over all the nodes. It's slightly easier and more common to iterate
> > > > > > over compatible nodes rather than nodes with some property.
> > > > > >
> > > > > > > > Are these being global in DT going to be a problem? Presumably
> > > > > > > > we have
> > > > > > > > a virtual space per IOMMU. We'd know which IOMMU based on a
> > > > > > > > device's
> > > > > > > > 'iommus' and 'memory-region' properties, but within
> > > > > > > > /reserved-memory
> > > > > > > > we wouldn't be able to distinguish overlapping addresses from
> > > > > > > > separate
> > > > > > > > address spaces. Or we could have 2 different IOVAs for 1
> > > > > > > > physical
> > > > > > > > space. That could be solved with something like this:
> > > > > > > >
> > > > > > > > iommu-addresses = <&iommu1 >;
> > > > > > >
> > > > > > > The only case that would be problematic would be if we have
> > > > > > > overlapping
> > > > > > > physical regions, because that will probably trip up the standard
> > > > > > > code.
> > > > > > >
> > > > > > > But this could also be worked around by looking at
> > > > > > > iommu-addresses. For
> > > > > > > example, if we had something like this:
> > > > > > >
> > > > > > > reserved-memory {
> > > > > > > fb_dc0: fb@8000 {
> > > > > > > reg = <0x8000 0x0100>;
> > > > > > > iommu-addresses = <0xa00
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On 2022-02-09 17:31:16 +0100, Thierry Reding wrote:
> On Sun, Feb 06, 2022 at 11:27:00PM +0100, Janne Grunau wrote:
> > On 2021-09-15 17:19:39 +0200, Thierry Reding wrote:
> > > On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote:
> > > > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote:
> > > > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding
> > > > > wrote:
> > > > > >
> > > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote:
> > > > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote:
> > > > > > > > >
> > > > > > > > > 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).
> > > > > > >
> > > > > > > By virtual only, you mean no physical mapping, just a region of
> > > > > > > virtual space, right? For that we'd have no 'reg' and therefore no
> > > > > > > (physical) reservation by the OS. It's similar to non-static
> > > > > > > regions.
> > > > > > > You need a specific handler for them. We'd probably want a
> > > > > > > compatible
> > > > > > > as well for these virtual reservations.
> > > > > >
> > > > > > Yeah, these would be purely used for reserving regions in the IOVA
> > > > > > so
> > > > > > that they won't be used by the IOVA allocator. Typically these
> > > > > > would be
> > > > > > used for cases where those addresses have some special meaning.
> > > > > >
> > > > > > Do we want something like:
> > > > > >
> > > > > > compatible = "iommu-reserved";
> > > > > >
> > > > > > for these? Or would that need to be:
> > > > > >
> > > > > > compatible = "linux,iommu-reserved";
> > > > > >
> > > > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix
> > > > > > compatible strings in the reserved-memory DT bindings directory.
> > > > >
> > > > > I would not use 'linux,' here.
> > > > >
> > > > > >
> > > > > > On the other hand, do we actually need the compatible string?
> > > > > > Because we
> > > > > > don't really want to associate much extra information with this
> > > > > > like we
> > > > > > do for example with "shared-dma-pool". The logic to handle this
> > > > > > would
> > > > > > all be within the IOMMU framework. All we really need is for the
> > > > > > standard reservation code to skip nodes that don't have a reg
> > > > > > property
> > > > > > so we don't reserve memory for "virtual-only" allocations.
> > > > >
> > > > > It doesn't hurt to have one and I can imagine we might want to iterate
> > > > > over all the nodes. It's slightly easier and more common to iterate
> > > > > over compatible nodes rather than nodes with some property.
> > > > >
> > > > > > > Are these being global in DT going to be a problem? Presumably we
> > > > > > > have
> > > > > > > a virtual space per IOMMU. We'd know which IOMMU based on a
> > > > > > > device's
> > > > > > > 'iommus' and 'memory-region' properties, but within
> > > > > > > /reserved-memory
> > > > > > > we wouldn't be able to distinguish overlapping addresses from
> > > > > > > separate
> > > > > > > address spaces. Or we could have 2 different IOVAs for 1 physical
> > > > > > > space. That could be solved with something like this:
> > > > > > >
> > > > > > > iommu-addresses = <&iommu1 >;
> > > > > >
> > > > > > The only case that would be problematic would be if we have
> > > > > > overlapping
> > > > > > physical regions, because that will probably trip up the standard
> > > > > > code.
> > > > > >
> > > > > > But this could also be worked around by looking at iommu-addresses.
> > > > > > For
> > > > > > example, if we had something like this:
> > > > > >
> > > > > > reserved-memory {
> > > > > > fb_dc0: fb@8000 {
> > > > > > reg = <0x8000 0x0100>;
> > > > > > iommu-addresses = <0xa000 0x0100>;
> > > > > > };
> > > > > >
> > > > > > fb_dc1: fb@8000 {
> > > > >
> > > > > You can't have 2 nodes with the same name (actually, you can, they
> > > > > just get merged together). Different names with the same unit-address
> > > > > is a dtc warning. I'd
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Sun, Feb 06, 2022 at 11:27:00PM +0100, Janne Grunau wrote:
> On 2021-09-15 17:19:39 +0200, Thierry Reding wrote:
> > On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote:
> > > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote:
> > > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding
> > > > wrote:
> > > > >
> > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote:
> > > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding
> > > > > > wrote:
> > > > > > >
> > > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote:
> > > > > > > >
> > > > > > > > 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).
> > > > > >
> > > > > > By virtual only, you mean no physical mapping, just a region of
> > > > > > virtual space, right? For that we'd have no 'reg' and therefore no
> > > > > > (physical) reservation by the OS. It's similar to non-static
> > > > > > regions.
> > > > > > You need a specific handler for them. We'd probably want a
> > > > > > compatible
> > > > > > as well for these virtual reservations.
> > > > >
> > > > > Yeah, these would be purely used for reserving regions in the IOVA so
> > > > > that they won't be used by the IOVA allocator. Typically these would
> > > > > be
> > > > > used for cases where those addresses have some special meaning.
> > > > >
> > > > > Do we want something like:
> > > > >
> > > > > compatible = "iommu-reserved";
> > > > >
> > > > > for these? Or would that need to be:
> > > > >
> > > > > compatible = "linux,iommu-reserved";
> > > > >
> > > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix
> > > > > compatible strings in the reserved-memory DT bindings directory.
> > > >
> > > > I would not use 'linux,' here.
> > > >
> > > > >
> > > > > On the other hand, do we actually need the compatible string? Because
> > > > > we
> > > > > don't really want to associate much extra information with this like
> > > > > we
> > > > > do for example with "shared-dma-pool". The logic to handle this would
> > > > > all be within the IOMMU framework. All we really need is for the
> > > > > standard reservation code to skip nodes that don't have a reg property
> > > > > so we don't reserve memory for "virtual-only" allocations.
> > > >
> > > > It doesn't hurt to have one and I can imagine we might want to iterate
> > > > over all the nodes. It's slightly easier and more common to iterate
> > > > over compatible nodes rather than nodes with some property.
> > > >
> > > > > > Are these being global in DT going to be a problem? Presumably we
> > > > > > have
> > > > > > a virtual space per IOMMU. We'd know which IOMMU based on a device's
> > > > > > 'iommus' and 'memory-region' properties, but within /reserved-memory
> > > > > > we wouldn't be able to distinguish overlapping addresses from
> > > > > > separate
> > > > > > address spaces. Or we could have 2 different IOVAs for 1 physical
> > > > > > space. That could be solved with something like this:
> > > > > >
> > > > > > iommu-addresses = <&iommu1 >;
> > > > >
> > > > > The only case that would be problematic would be if we have
> > > > > overlapping
> > > > > physical regions, because that will probably trip up the standard
> > > > > code.
> > > > >
> > > > > But this could also be worked around by looking at iommu-addresses.
> > > > > For
> > > > > example, if we had something like this:
> > > > >
> > > > > reserved-memory {
> > > > > fb_dc0: fb@8000 {
> > > > > reg = <0x8000 0x0100>;
> > > > > iommu-addresses = <0xa000 0x0100>;
> > > > > };
> > > > >
> > > > > fb_dc1: fb@8000 {
> > > >
> > > > You can't have 2 nodes with the same name (actually, you can, they
> > > > just get merged together). Different names with the same unit-address
> > > > is a dtc warning. I'd really like to make that a full blown
> > > > overlapping region check.
> > >
> > > Right... so this would be a lot easier to deal with using that earlier
> > > proposal where the IOMMU regions were a separate thing and referencing
> > > the reserved-memory nodes. In those cases we could just have the
> > > physical reservation for the fr
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On 2021-09-15 17:19:39 +0200, Thierry Reding wrote:
> On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote:
> > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote:
> > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding
> > > wrote:
> > > >
> > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote:
> > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding
> > > > > wrote:
> > > > > >
> > > > > > On Fri, Sep 03, 2021 at 08:20:55AM -0500, Rob Herring wrote:
> > > > > > >
> > > > > > > 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).
> > > > >
> > > > > By virtual only, you mean no physical mapping, just a region of
> > > > > virtual space, right? For that we'd have no 'reg' and therefore no
> > > > > (physical) reservation by the OS. It's similar to non-static regions.
> > > > > You need a specific handler for them. We'd probably want a compatible
> > > > > as well for these virtual reservations.
> > > >
> > > > Yeah, these would be purely used for reserving regions in the IOVA so
> > > > that they won't be used by the IOVA allocator. Typically these would be
> > > > used for cases where those addresses have some special meaning.
> > > >
> > > > Do we want something like:
> > > >
> > > > compatible = "iommu-reserved";
> > > >
> > > > for these? Or would that need to be:
> > > >
> > > > compatible = "linux,iommu-reserved";
> > > >
> > > > ? There seems to be a mix of vendor-prefix vs. non-vendor-prefix
> > > > compatible strings in the reserved-memory DT bindings directory.
> > >
> > > I would not use 'linux,' here.
> > >
> > > >
> > > > On the other hand, do we actually need the compatible string? Because we
> > > > don't really want to associate much extra information with this like we
> > > > do for example with "shared-dma-pool". The logic to handle this would
> > > > all be within the IOMMU framework. All we really need is for the
> > > > standard reservation code to skip nodes that don't have a reg property
> > > > so we don't reserve memory for "virtual-only" allocations.
> > >
> > > It doesn't hurt to have one and I can imagine we might want to iterate
> > > over all the nodes. It's slightly easier and more common to iterate
> > > over compatible nodes rather than nodes with some property.
> > >
> > > > > Are these being global in DT going to be a problem? Presumably we have
> > > > > a virtual space per IOMMU. We'd know which IOMMU based on a device's
> > > > > 'iommus' and 'memory-region' properties, but within /reserved-memory
> > > > > we wouldn't be able to distinguish overlapping addresses from separate
> > > > > address spaces. Or we could have 2 different IOVAs for 1 physical
> > > > > space. That could be solved with something like this:
> > > > >
> > > > > iommu-addresses = <&iommu1 >;
> > > >
> > > > The only case that would be problematic would be if we have overlapping
> > > > physical regions, because that will probably trip up the standard code.
> > > >
> > > > But this could also be worked around by looking at iommu-addresses. For
> > > > example, if we had something like this:
> > > >
> > > > reserved-memory {
> > > > fb_dc0: fb@8000 {
> > > > reg = <0x8000 0x0100>;
> > > > iommu-addresses = <0xa000 0x0100>;
> > > > };
> > > >
> > > > fb_dc1: fb@8000 {
> > >
> > > You can't have 2 nodes with the same name (actually, you can, they
> > > just get merged together). Different names with the same unit-address
> > > is a dtc warning. I'd really like to make that a full blown
> > > overlapping region check.
> >
> > Right... so this would be a lot easier to deal with using that earlier
> > proposal where the IOMMU regions were a separate thing and referencing
> > the reserved-memory nodes. In those cases we could just have the
> > physical reservation for the framebuffer once (so we don't get any
> > duplicates or overlaps) and then have each IOVA reservation reference
> > that to create the mapping.
> >
> > >
> > > > reg = <0x8000 0x0100>;
> > > > iommu-addresses = <0xb000 0x0100>;
> > > > };
> > > > };
> > > >
> > > > We could make the code identify
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Tue, Sep 07, 2021 at 07:44:44PM +0200, Thierry Reding wrote: > On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote: > > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding > > wrote: > > > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > > > > wrote: > > > > > > > > > > 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 пишет: > > > > > > > > > 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 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 > > > > > > > > > --- > > > > > > > > > .../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 coul
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Tue, Sep 07, 2021 at 10:33:24AM -0500, Rob Herring wrote: > On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding > wrote: > > > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > > > wrote: > > > > > > > > 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 пишет: > > > > > > > > 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 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 > > > > > > > > --- > > > > > > > > .../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' > >
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Fri, Sep 3, 2021 at 10:36 AM Thierry Reding wrote: > > On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > > wrote: > > > > > > 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 пишет: > > > > > > > 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 > > > > > > > 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 > > > > > > > --- > > > > > > > .../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
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Fri, Sep 03, 2021 at 09:36:33AM -0500, Rob Herring wrote: > On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding > wrote: > > > > 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 пишет: > > > > > > 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 > > > > > > 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 > > > > > > --- > > > > > > .../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. > > > > > >>> > > > > > >>>
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
On Fri, Sep 3, 2021 at 8:52 AM Thierry Reding wrote: > > 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 пишет: > > > > > 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 > > > > > 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 > > > > > --- > > > > > .../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 > > > > >>> memo
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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 пишет: > > > > 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 > > > > 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 > > > > --- > > > > .../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. I
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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 пишет:
> > > 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 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
> > > ---
> > > .../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-m
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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
> >
> > 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
> > ---
> > .../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
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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
>
> 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
> ---
> .../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).
>>
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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 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
> > > > ---
> > > > .../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
> >
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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 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
> > > ---
> > > .../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
signature.asc
Description: PGP signature
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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 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
> > ---
> > .../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).
Thierry
signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
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 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 > --- > .../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.) 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. 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. Rob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
From: Thierry Reding 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 --- .../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 diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt index e8d3096d922c..e9c2f80b441f 100644 --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt @@ -52,6 +52,11 @@ compatible (optional) - standard definition be used by an operating system to instantiate the necessary pool management subsystem if necessary. - vendor specific string in the form ,[-] +#memory-region-cells (optional) - +- Defines how many cells are used to form the memory region specifier. + The memory region specifier contains additional information on how a + reserved memory region referenced by the corresponding phandle will + be used in a specific context. no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping of the region as part of its standard mapping of system memory, @@ -83,6 +88,22 @@ memory-region (optional) - phandle, specifier pairs to children of /reserved-mem memory-region-names (optional) - a list of names, one for each corresponding entry in the memory-region property +Reserved memory region references can be accompanied by a memory region +specifier, which provides additional information about how the memory region +will be used in that specific context. If a reserved memory region does not +have the #memory-region-cells property, 0 is implied and no information +besides the phandle is conveyed. For reserved memory regions that contain +#memory-region-cells = <1>, the following encoding applies if not otherwise +overridden by the bindings selected by the region's compatible string: + + - bit 0: If set, requests that the region be identity mapped if the system +uses an IOMMU for I/O virtual address translations. This is used, for +example, when a bootloader has configured a display controller to display +a boot splash. Once the OS takes over and enables the IOMMU for the given +display controller, the IOMMU may fault if the framebuffer hasn't been +mapped to the IOMMU at the address that the display controller tries to +access. + Example --- This example defines 3 contiguous regions are defined for Linux kernel: diff --git a/include/dt-bindings/reserved-memory.h b/include/dt-bindings/reserved-memory.h new file mode 100644 index ..174ca3448342 --- /dev/null +++ b/include/dt-bindings/reserved-memory.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */ + +#ifndef _DT_BINDINGS_RESERVED_MEMORY_H +#define _DT_BINDINGS_RESERVED_MEMORY_H + +#define MEMORY_REGION_IDENTITY_MAPPING 0x1 + +#endif -- 2.30.2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
