diff mbox series

[2/4] dt-bindings: sram: add documentation for reserved-only flag

Message ID 20200512144803.24344-2-ykaukab@suse.de (mailing list archive)
State New, archived
Headers show
Series [1/4] misc: sram: add support for remapping reserved regions only | expand

Commit Message

Mian Yousaf Kaukab May 12, 2020, 2:48 p.m. UTC
Add documentation for the new optional flag added for SRAM driver.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 Documentation/devicetree/bindings/sram/sram.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Stephen Warren May 12, 2020, 7:45 p.m. UTC | #1
On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> Add documentation for the new optional flag added for SRAM driver.

> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml

> +  reserved-only:
> +    description:
> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> +      remapping type is selected depending upon no-memory-wc as usual.
> +    type: boolean

This feels a bit like a SW flag rather than a HW description, so I'm not
sure it's appropriate to put it into DT.

Are there any cases where the SW should map all of the SRAM, i.e. where
we wouldn't expect to set reserved-only? I'd expect reserved-only to be
the default, and perhaps only, mode of operation for the SRAM driver. If
we can't do that because some SW currently expects to be able to map
arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
SRAM driver which parts it's using, hence still allowing the driver to
only map in-use portions?
Mian Yousaf Kaukab May 13, 2020, 10:41 a.m. UTC | #2
On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > Add documentation for the new optional flag added for SRAM driver.
> 
> > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> 
> > +  reserved-only:
> > +    description:
> > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > +      remapping type is selected depending upon no-memory-wc as usual.
> > +    type: boolean
> 
> This feels a bit like a SW flag rather than a HW description, so I'm not
> sure it's appropriate to put it into DT.

Reserved regions themselves are software descriptions, no? Then we have 'pool'
flag which is again a software flag and so on. This flag falls into same
category and nothing out of ordinary.
> 
> Are there any cases where the SW should map all of the SRAM, i.e. where
> we wouldn't expect to set reserved-only? [...]

Yes, here are a few examples:
arch/arm/boot/dts/aspeed-g*.dtsi
arch/arm/boot/dts/at91*.dtsi
arch/arm/boot/dts/bcm7445.dtsi
Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
except the reserved region.

> [...] I'd expect reserved-only to be
> the default, and perhaps only, mode of operation for the SRAM driver.

It will break compatibility with existing dtbs.

> If we can't do that because some SW currently expects to be able to map
> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> SRAM driver which parts it's using, hence still allowing the driver to
> only map in-use portions?

User doesn’t need sram driver in that case. It can use genalloc api directly.

BR,
Yousaf
Stephen Warren May 19, 2020, 4:16 p.m. UTC | #3
On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
>> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
>>> Add documentation for the new optional flag added for SRAM driver.
>>
>>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
>>
>>> +  reserved-only:
>>> +    description:
>>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
>>> +      remapping type is selected depending upon no-memory-wc as usual.
>>> +    type: boolean
>>
>> This feels a bit like a SW flag rather than a HW description, so I'm not
>> sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.

I suppose that's true to some extent. This is indeed a description of
the system environment presented to the SW that consumes the DT, which
is a bit more than pure HW description but still a description of
something imposed externally rather than describing something that's up
to the discretion of the consuming SW. So, go ahead.

>> Are there any cases where the SW should map all of the SRAM, i.e. where
>> we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi
> arch/arm/boot/dts/at91*.dtsi
> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.
> 
>> [...] I'd expect reserved-only to be
>> the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.
> 
>> If we can't do that because some SW currently expects to be able to map
>> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
>> SRAM driver which parts it's using, hence still allowing the driver to
>> only map in-use portions?
> 
> User doesn’t need sram driver in that case. It can use genalloc api directly.

This sounds a bit odd. Without a driver for the reserved region, nothing
should be touching it, since otherwise there's no code that owns an
manages the region. If any code needs to consume the region, it should
obtain info about the region from some form of provider code that can
handle both the allocation and mapping. Anything else sounds like some
consumer code directly making use of DT nodes it doesn't own. But since
I'm not familiar enough with the SRAM driver and genalloc code that you
mention to fully understand the allocation paths I guess I won't object
for now, although it does still sound fishy.
Rob Herring (Arm) May 19, 2020, 11:03 p.m. UTC | #4
On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> >>> Add documentation for the new optional flag added for SRAM driver.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> >>
> >>> +  reserved-only:
> >>> +    description:
> >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> >>> +      remapping type is selected depending upon no-memory-wc as usual.
> >>> +    type: boolean
> >>
> >> This feels a bit like a SW flag rather than a HW description, so I'm not
> >> sure it's appropriate to put it into DT.
> > 
> > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > flag which is again a software flag and so on. This flag falls into same
> > category and nothing out of ordinary.
> 
> I suppose that's true to some extent. This is indeed a description of
> the system environment presented to the SW that consumes the DT, which
> is a bit more than pure HW description but still a description of
> something imposed externally rather than describing something that's up
> to the discretion of the consuming SW. So, go ahead.
> 
> >> Are there any cases where the SW should map all of the SRAM, i.e. where
> >> we wouldn't expect to set reserved-only? [...]
> > 
> > Yes, here are a few examples:
> > arch/arm/boot/dts/aspeed-g*.dtsi
> > arch/arm/boot/dts/at91*.dtsi
> > arch/arm/boot/dts/bcm7445.dtsi
> > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > except the reserved region.
> > 
> >> [...] I'd expect reserved-only to be
> >> the default, and perhaps only, mode of operation for the SRAM driver.
> > 
> > It will break compatibility with existing dtbs.
> > 
> >> If we can't do that because some SW currently expects to be able to map
> >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> >> SRAM driver which parts it's using, hence still allowing the driver to
> >> only map in-use portions?
> > 
> > User doesn’t need sram driver in that case. It can use genalloc api directly.
> 
> This sounds a bit odd. Without a driver for the reserved region, nothing
> should be touching it, since otherwise there's no code that owns an
> manages the region. If any code needs to consume the region, it should
> obtain info about the region from some form of provider code that can
> handle both the allocation and mapping. Anything else sounds like some
> consumer code directly making use of DT nodes it doesn't own. But since
> I'm not familiar enough with the SRAM driver and genalloc code that you
> mention to fully understand the allocation paths I guess I won't object
> for now, although it does still sound fishy.

I'm fine with the concept, but I don't think a single flag is adequate. 
If there are reserved regions within the SRAM, then define child nodes 
to mark those regions reserved. I don't think you need a new flag. Just 
a 'reg' property and nothing else.

Rob
Thierry Reding May 20, 2020, 8:40 a.m. UTC | #5
On Wed, May 13, 2020 at 12:41:27PM +0200, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > Add documentation for the new optional flag added for SRAM driver.
> > 
> > > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > 
> > > +  reserved-only:
> > > +    description:
> > > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > +      remapping type is selected depending upon no-memory-wc as usual.
> > > +    type: boolean
> > 
> > This feels a bit like a SW flag rather than a HW description, so I'm not
> > sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.
> > 
> > Are there any cases where the SW should map all of the SRAM, i.e. where
> > we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi

Looking at the implementation of the sole user of this, which is in
drivers/fsi/fsi-master-ast-cf.c, it looks like this really should've
specified a partition because the driver basically goes on to allocate
a fixed 4 KiB region of memory anyway.

> arch/arm/boot/dts/at91*.dtsi

While these define SRAM nodes, I don't see them referenced anywhere.

> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.

The driver currently maps everything, so if this relies on this
particular reserved region not being mapped then that's already broken
anyway.

> > [...] I'd expect reserved-only to be
> > the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.

Yes, that's a bit unfortunate. I think this driver may suffer from a
slightly ambiguous device tree binding and then people just trying to
fit it to their use-cases.

However, I think we could preserve DTB backwards-compatibility while at
the same time correcting course and establish some sort of consistency.

Looking at the examples that you've provided and others, there are two
classes of users: users that don't specify any partitions either use all
of the available SRAM exclusively or manually allocate some part of it,
whereas users that have specified partitions all seem to use only the
defined partitions.

Given that, I think what we could do is check if there are any child
nodes and if not, keep the existing behaviour of mapping the whole SRAM
area. For cases where child nodes exist we could decide to go with the
default that Stephen suggested and only map regions for which a child
node has been defined.

This should allow both categories of users to work the way that they
were probably expected to work.

Any thoughts?

Thierry
Thierry Reding May 20, 2020, 8:55 a.m. UTC | #6
On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > >>> Add documentation for the new optional flag added for SRAM driver.
> > >>
> > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > >>
> > >>> +  reserved-only:
> > >>> +    description:
> > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > >>> +    type: boolean
> > >>
> > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > >> sure it's appropriate to put it into DT.
> > > 
> > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > flag which is again a software flag and so on. This flag falls into same
> > > category and nothing out of ordinary.
> > 
> > I suppose that's true to some extent. This is indeed a description of
> > the system environment presented to the SW that consumes the DT, which
> > is a bit more than pure HW description but still a description of
> > something imposed externally rather than describing something that's up
> > to the discretion of the consuming SW. So, go ahead.
> > 
> > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > >> we wouldn't expect to set reserved-only? [...]
> > > 
> > > Yes, here are a few examples:
> > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > arch/arm/boot/dts/at91*.dtsi
> > > arch/arm/boot/dts/bcm7445.dtsi
> > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > except the reserved region.
> > > 
> > >> [...] I'd expect reserved-only to be
> > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > 
> > > It will break compatibility with existing dtbs.
> > > 
> > >> If we can't do that because some SW currently expects to be able to map
> > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > >> SRAM driver which parts it's using, hence still allowing the driver to
> > >> only map in-use portions?
> > > 
> > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > 
> > This sounds a bit odd. Without a driver for the reserved region, nothing
> > should be touching it, since otherwise there's no code that owns an
> > manages the region. If any code needs to consume the region, it should
> > obtain info about the region from some form of provider code that can
> > handle both the allocation and mapping. Anything else sounds like some
> > consumer code directly making use of DT nodes it doesn't own. But since
> > I'm not familiar enough with the SRAM driver and genalloc code that you
> > mention to fully understand the allocation paths I guess I won't object
> > for now, although it does still sound fishy.
> 
> I'm fine with the concept, but I don't think a single flag is adequate. 
> If there are reserved regions within the SRAM, then define child nodes 
> to mark those regions reserved. I don't think you need a new flag. Just 
> a 'reg' property and nothing else.

It sounds to me like there are two different interpretations of SRAM and
reserved regions. On one hand, as you suggest, we have one SRAM that's
made available as genalloc pool and then individual regions can be
marked as reserved so that they aren't added to that pool.

At the same time, each reserved region is also exposed as a separate
pool and that's in fact used by many consumers as a way of getting a
specific chunk of the SRAM for their own use (via phandle to the region
from the consumer's device tree node).

In addition to that, the reserved region code doesn't actually fully do
its job because while the reserved region isn't actually added to the
"top-level" SRAM pool, the memory is still mapped. At the same time this
is something that we actually want because, like I mentioned, some of
the consumers do want to get at their SRAM chunks via references to the
partitions.

The problem that this patch series is really trying to solve is another
still: the complete SRAM is always mapped to kernel memory, irrespective
of whether any regions are marked reserved or not and that can cause
speculative accesses to memory outside of the defined regions.

Stephen's suggestion is to default to only mapping memory for which a
partition has been defined in the SRAM and assuming that all SRAM
outside of those partitions is off limits. I think that's a sensible
default and it's unambiguous.

But as Yousaf points out that would break compatibility with existing
device trees. Depending on how you interpret the bindings one could
argue that those device trees are buggy and should have partitions
defined (in the cases I've looked at they end up using a fixed region
anyway, so that could've just been made explicit in the device tree).

However, it also looks like all of the users that rely on the original
behaviour where they can just access the full pool are those that don't
define any reserved regions, whereas all users that do reserve regions
will actually use those reserved regions.

So I think we can make use of this by differentiating in the driver
between SRAM nodes with or without children and change the behaviour
accordingly. I think that has the big advantage that it makes things
work as (I think) most people would expect and doesn't further
complicate the binding with extra flags.

Thierry
Mian Yousaf Kaukab May 26, 2020, 3:28 p.m. UTC | #7
On Wed, May 20, 2020 at 10:55:58AM +0200, Thierry Reding wrote:
> On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> > On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > >>> Add documentation for the new optional flag added for SRAM driver.
> > > >>
> > > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > > >>
> > > >>> +  reserved-only:
> > > >>> +    description:
> > > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > > >>> +    type: boolean
> > > >>
> > > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > > >> sure it's appropriate to put it into DT.
> > > > 
> > > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > > flag which is again a software flag and so on. This flag falls into same
> > > > category and nothing out of ordinary.
> > > 
> > > I suppose that's true to some extent. This is indeed a description of
> > > the system environment presented to the SW that consumes the DT, which
> > > is a bit more than pure HW description but still a description of
> > > something imposed externally rather than describing something that's up
> > > to the discretion of the consuming SW. So, go ahead.
> > > 
> > > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > > >> we wouldn't expect to set reserved-only? [...]
> > > > 
> > > > Yes, here are a few examples:
> > > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > > arch/arm/boot/dts/at91*.dtsi
> > > > arch/arm/boot/dts/bcm7445.dtsi
> > > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > > except the reserved region.
> > > > 
> > > >> [...] I'd expect reserved-only to be
> > > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > > 
> > > > It will break compatibility with existing dtbs.
> > > > 
> > > >> If we can't do that because some SW currently expects to be able to map
> > > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > > >> SRAM driver which parts it's using, hence still allowing the driver to
> > > >> only map in-use portions?
> > > > 
> > > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > > 
> > > This sounds a bit odd. Without a driver for the reserved region, nothing
> > > should be touching it, since otherwise there's no code that owns an
> > > manages the region. If any code needs to consume the region, it should
> > > obtain info about the region from some form of provider code that can
> > > handle both the allocation and mapping. Anything else sounds like some
> > > consumer code directly making use of DT nodes it doesn't own. But since
> > > I'm not familiar enough with the SRAM driver and genalloc code that you
> > > mention to fully understand the allocation paths I guess I won't object
> > > for now, although it does still sound fishy.
> > 
> > I'm fine with the concept, but I don't think a single flag is adequate. 
> > If there are reserved regions within the SRAM, then define child nodes 
> > to mark those regions reserved. I don't think you need a new flag. Just 
> > a 'reg' property and nothing else.
> 
> It sounds to me like there are two different interpretations of SRAM and
> reserved regions. On one hand, as you suggest, we have one SRAM that's
> made available as genalloc pool and then individual regions can be
> marked as reserved so that they aren't added to that pool.
> 
> At the same time, each reserved region is also exposed as a separate
> pool and that's in fact used by many consumers as a way of getting a
> specific chunk of the SRAM for their own use (via phandle to the region
> from the consumer's device tree node).
> 
> In addition to that, the reserved region code doesn't actually fully do
> its job because while the reserved region isn't actually added to the
> "top-level" SRAM pool, the memory is still mapped. At the same time this
> is something that we actually want because, like I mentioned, some of
> the consumers do want to get at their SRAM chunks via references to the
> partitions.
> 
> The problem that this patch series is really trying to solve is another
> still: the complete SRAM is always mapped to kernel memory, irrespective
> of whether any regions are marked reserved or not and that can cause
> speculative accesses to memory outside of the defined regions.
> 
> Stephen's suggestion is to default to only mapping memory for which a
> partition has been defined in the SRAM and assuming that all SRAM
> outside of those partitions is off limits. I think that's a sensible
> default and it's unambiguous.
> 
> But as Yousaf points out that would break compatibility with existing
> device trees. Depending on how you interpret the bindings one could
> argue that those device trees are buggy and should have partitions
> defined (in the cases I've looked at they end up using a fixed region
> anyway, so that could've just been made explicit in the device tree).
> 
> However, it also looks like all of the users that rely on the original
> behaviour where they can just access the full pool are those that don't
> define any reserved regions, whereas all users that do reserve regions
> will actually use those reserved regions.
> 
> So I think we can make use of this by differentiating in the driver
> between SRAM nodes with or without children and change the behaviour
> accordingly. I think that has the big advantage that it makes things
> work as (I think) most people would expect and doesn't further
> complicate the binding with extra flags.

I tend to agree on mapping partitions only if they exist. So far I could
only find one exception. It is arch/arm/boot/dts/armada-370.dtsi which is
using the top level pool as well as a partition to reserve 32 bytes at the
bottom of sram. This can be fixed along with the sram driver change, by
adding another partition for the rest of the sram and using its handle in
the crypto@90000 instead of top-level sram node handle. Do you see anymore
exceptions where both top level pool and the partitions both are being used?

Then on the backward compatibility topic, another issue is that boot code
could add sram nodes dynamically. For example arch/arm/mach-k3/common.c in
u-boot does it. This particular case will not break after the suggested change
because it is not adding any partitions. However, there could be other
boot-loaders which are not this lucky.

> 
> Thierry

/Yousaf
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 19d116ff9ddc..4bcc309fa841 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -55,6 +55,12 @@  properties:
       as write combining. WC is used by default.
     type: boolean
 
+  reserved-only:
+    description:
+      The flag indicating, that only SRAM reserved regions have to be remapped.
+      remapping type is selected depending upon no-memory-wc as usual.
+    type: boolean
+
 patternProperties:
   "^([a-z]*-)?sram(-section)?@[a-f0-9]+$":
     type: object