Some platforms have laxed requirements on the placement of the rmtfs
memory region, introduce support for guard pages to allow the DeviceTree
author to rely on the OS/Linux for placement of the region.
Changes since v2:
- Rewrote DeviceTree binding description, to avoid dictating the OS
behavior.
- Adjusted addr and size before memremap(), to avoid mapping the guard
pages, and unnecessarily have to adjust the base pointer.
Changes since v1:
- Drop qcom,alloc-size in favour of using reserved-memory/size
- Introduce explicit property to signal that guard pages should be
carved out from this region (rather than always do it in the dynamic
case).
- Drop the dma_alloc_coherent() based approach and just add support for
the guard pages.
- Added handling of failed reserved-memory allocation (patch 3)
---
Bjorn Andersson (3):
dt-bindings: reserved-memory: rmtfs: Allow guard pages
soc: qcom: rmtfs: Support discarding guard pages
soc: qcom: rtmfs: Handle reserved-memory allocation issues
.../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml | 11 +++++++++++
drivers/soc/qcom/rmtfs_mem.c | 11 ++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
---
base-commit: 926f75c8a5ab70567eb4c2d82fbc96963313e564
change-id: 20230920-rmtfs-mem-guard-pages-d3d0ed0cb036
Best regards,
--
Bjorn Andersson <[email protected]>
In some configurations, the exact placement of the rmtfs shared memory
region isn't so strict. The DeviceTree author can then choose to use the
"size" property and rely on the OS for placement (in combination with
"alloc-ranges", if desired).
But on some platforms the rmtfs memory region may not be allocated
adjacent to regions allocated by other clients. Add support for
discarding the first and last 4k block in the region, if
qcom,use-guard-pages is specified in DeviceTree.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index f83811f51175..83bba9321e72 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
rmtfs_mem->client_id = client_id;
rmtfs_mem->size = rmem->size;
+ /*
+ * If requested, discard the first and last 4k block in order to ensure
+ * that the rmtfs region isn't adjacent to other protected regions.
+ */
+ if (of_property_present(node, "qcom,use-guard-pages")) {
+ rmtfs_mem->addr += SZ_4K;
+ rmtfs_mem->size -= 2 * SZ_4K;
+ }
+
device_initialize(&rmtfs_mem->dev);
rmtfs_mem->dev.parent = &pdev->dev;
rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
--
2.25.1
In the even that Linux failed to allocate the reserved memory range
specified in the DeviceTree, the size of the reserved_mem will be 0,
which results in a oops when memory remapping is attempted.
Detect this and report that the memory region was not found instead.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/soc/qcom/rmtfs_mem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index 83bba9321e72..13823abd85c2 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
int ret, i;
rmem = of_reserved_mem_lookup(node);
- if (!rmem) {
+ if (!rmem || !rmem->size) {
dev_err(&pdev->dev, "failed to acquire memory region\n");
return -EINVAL;
}
--
2.25.1
On 9/21/23 04:37, Bjorn Andersson wrote:
> In the even that Linux failed to allocate the reserved memory range
> specified in the DeviceTree, the size of the reserved_mem will be 0,
> which results in a oops when memory remapping is attempted.
>
> Detect this and report that the memory region was not found instead.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
typo in subject: rtmfs
Acked-by: Konrad Dybcio <[email protected]>
Konrad
On Thu, Sep 21, 2023 at 08:11:23PM +0200, Stephan Gerhold wrote:
> On Wed, Sep 20, 2023 at 07:37:32PM -0700, Bjorn Andersson wrote:
> > In the even that Linux failed to allocate the reserved memory range
> > specified in the DeviceTree, the size of the reserved_mem will be 0,
> > which results in a oops when memory remapping is attempted.
> >
> > Detect this and report that the memory region was not found instead.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
>
> I dropped these checks in my remoteproc patches because Caleb suggested
> maybe putting this check directly in of_reserved_mem_lookup() (or
> similar) given that almost none of the users verify this [1].
>
> Do you have any opinion on that? I asked back then too but you did not
> reply yet [2]. :-)
>
I'm struggling to come up with a use case where one would like to get
hold of the rmem when it wasn't properly initialized. So, let's make an
attempt at returning NULL from of_reserved_mem_lookup() instead.
Thanks,
Bjorn
> [1]: https://lore.kernel.org/linux-arm-msm/[email protected]/
> [2]: https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> > ---
> > drivers/soc/qcom/rmtfs_mem.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > index 83bba9321e72..13823abd85c2 100644
> > --- a/drivers/soc/qcom/rmtfs_mem.c
> > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > @@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> > int ret, i;
> >
> > rmem = of_reserved_mem_lookup(node);
> > - if (!rmem) {
> > + if (!rmem || !rmem->size) {
> > dev_err(&pdev->dev, "failed to acquire memory region\n");
> > return -EINVAL;
> > }
> >
> > --
> > 2.25.1
> >
On Wed, Sep 20, 2023 at 07:37:32PM -0700, Bjorn Andersson wrote:
> In the even that Linux failed to allocate the reserved memory range
> specified in the DeviceTree, the size of the reserved_mem will be 0,
> which results in a oops when memory remapping is attempted.
>
> Detect this and report that the memory region was not found instead.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
I dropped these checks in my remoteproc patches because Caleb suggested
maybe putting this check directly in of_reserved_mem_lookup() (or
similar) given that almost none of the users verify this [1].
Do you have any opinion on that? I asked back then too but you did not
reply yet [2]. :-)
[1]: https://lore.kernel.org/linux-arm-msm/[email protected]/
[2]: https://lore.kernel.org/linux-arm-msm/[email protected]/
> ---
> drivers/soc/qcom/rmtfs_mem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 83bba9321e72..13823abd85c2 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -180,7 +180,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> int ret, i;
>
> rmem = of_reserved_mem_lookup(node);
> - if (!rmem) {
> + if (!rmem || !rmem->size) {
> dev_err(&pdev->dev, "failed to acquire memory region\n");
> return -EINVAL;
> }
>
> --
> 2.25.1
>
On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote:
> On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> > In some configurations, the exact placement of the rmtfs shared memory
> > region isn't so strict. The DeviceTree author can then choose to use the
> > "size" property and rely on the OS for placement (in combination with
> > "alloc-ranges", if desired).
> >
> > But on some platforms the rmtfs memory region may not be allocated
> > adjacent to regions allocated by other clients. Add support for
> > discarding the first and last 4k block in the region, if
> > qcom,use-guard-pages is specified in DeviceTree.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > index f83811f51175..83bba9321e72 100644
> > --- a/drivers/soc/qcom/rmtfs_mem.c
> > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> > rmtfs_mem->client_id = client_id;
> > rmtfs_mem->size = rmem->size;
> >
> > + /*
> > + * If requested, discard the first and last 4k block in order to ensure
> > + * that the rmtfs region isn't adjacent to other protected regions.
> > + */
> > + if (of_property_present(node, "qcom,use-guard-pages")) {
>
> I think of_property_read_bool() would be more fitting here. Right now
> of_property_present() is just a wrapper around of_property_read_bool().
> Semantically reading a bool fits better here though. :-)
>
Are you saying that you would prefer this to be a bool, so hat you can
give it a "false" value? Or you are simply saying "it walks like a
boolean, quacks like a boolean, let's use the boolean accessor"?
> Feel free to fix that up while applying.
>
> FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good
> way to describe this in the DT. For the implementation side feel free to
> add my
>
Right, I don't think I commented on your suggestion to make the size of
the guard page configurable. I am not aware of any current or upcoming
reasons for adding such complexity, so I'd simply prefer to stick with a
boolean. Should that need arise, I think this model would allow
extension to express that.
Regards,
Bjorn
> Reviewed-by: Stephan Gerhold <[email protected]>
>
> Thanks,
> Stephan
>
> > + rmtfs_mem->addr += SZ_4K;
> > + rmtfs_mem->size -= 2 * SZ_4K;
> > + }
> > +
> > device_initialize(&rmtfs_mem->dev);
> > rmtfs_mem->dev.parent = &pdev->dev;
> > rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
> >
> > --
> > 2.25.1
> >
On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> In some configurations, the exact placement of the rmtfs shared memory
> region isn't so strict. The DeviceTree author can then choose to use the
> "size" property and rely on the OS for placement (in combination with
> "alloc-ranges", if desired).
>
> But on some platforms the rmtfs memory region may not be allocated
> adjacent to regions allocated by other clients. Add support for
> discarding the first and last 4k block in the region, if
> qcom,use-guard-pages is specified in DeviceTree.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index f83811f51175..83bba9321e72 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> rmtfs_mem->client_id = client_id;
> rmtfs_mem->size = rmem->size;
>
> + /*
> + * If requested, discard the first and last 4k block in order to ensure
> + * that the rmtfs region isn't adjacent to other protected regions.
> + */
> + if (of_property_present(node, "qcom,use-guard-pages")) {
I think of_property_read_bool() would be more fitting here. Right now
of_property_present() is just a wrapper around of_property_read_bool().
Semantically reading a bool fits better here though. :-)
Feel free to fix that up while applying.
FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good
way to describe this in the DT. For the implementation side feel free to
add my
Reviewed-by: Stephan Gerhold <[email protected]>
Thanks,
Stephan
> + rmtfs_mem->addr += SZ_4K;
> + rmtfs_mem->size -= 2 * SZ_4K;
> + }
> +
> device_initialize(&rmtfs_mem->dev);
> rmtfs_mem->dev.parent = &pdev->dev;
> rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
>
> --
> 2.25.1
>
eOn Thu, Sep 21, 2023 at 07:51:42PM -0700, Bjorn Andersson wrote:
> On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote:
> > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> > > In some configurations, the exact placement of the rmtfs shared memory
> > > region isn't so strict. The DeviceTree author can then choose to use the
> > > "size" property and rely on the OS for placement (in combination with
> > > "alloc-ranges", if desired).
> > >
> > > But on some platforms the rmtfs memory region may not be allocated
> > > adjacent to regions allocated by other clients. Add support for
> > > discarding the first and last 4k block in the region, if
> > > qcom,use-guard-pages is specified in DeviceTree.
> > >
> > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > ---
> > > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > > index f83811f51175..83bba9321e72 100644
> > > --- a/drivers/soc/qcom/rmtfs_mem.c
> > > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> > > rmtfs_mem->client_id = client_id;
> > > rmtfs_mem->size = rmem->size;
> > >
> > > + /*
> > > + * If requested, discard the first and last 4k block in order to ensure
> > > + * that the rmtfs region isn't adjacent to other protected regions.
> > > + */
> > > + if (of_property_present(node, "qcom,use-guard-pages")) {
> >
> > I think of_property_read_bool() would be more fitting here. Right now
> > of_property_present() is just a wrapper around of_property_read_bool().
> > Semantically reading a bool fits better here though. :-)
> >
>
> Are you saying that you would prefer this to be a bool, so hat you can
> give it a "false" value? Or you are simply saying "it walks like a
> boolean, quacks like a boolean, let's use the boolean accessor"?
>
The latter. I would expect that of_property_present() is used for
properties which usually have a value, while of_property_read_bool()
is used for pure bool values which can be present or not but must not
have a value. I think a "bool" in terms of DT is simply a present or
not-present property without any value?
For example consider
regulator-min-microvolts = <4200000000>;
regulator-always-on;
Then I would expect
- of_property_present(..., "regulator-min-microvolts"), but
- of_property_read_bool(..., "regulator-always-on")
Does that make sense? :D
> > Feel free to fix that up while applying.
> >
> > FWIW I don't really have an opinion if "qcom,use-guard-pages" is a good
> > way to describe this in the DT. For the implementation side feel free to
> > add my
> >
>
> Right, I don't think I commented on your suggestion to make the size of
> the guard page configurable. I am not aware of any current or upcoming
> reasons for adding such complexity, so I'd simply prefer to stick with a
> boolean. Should that need arise, I think this model would allow
> extension to express that.
>
I must admit I forgot that I suggested this until now. :')
I don't see a use case for a different "guard size" either so I think
it's fine to have it as a bool.
Thanks,
Stephan
On Fri, Sep 22, 2023 at 09:35:00AM +0200, Stephan Gerhold wrote:
> eOn Thu, Sep 21, 2023 at 07:51:42PM -0700, Bjorn Andersson wrote:
> > On Thu, Sep 21, 2023 at 08:04:06PM +0200, Stephan Gerhold wrote:
> > > On Wed, Sep 20, 2023 at 07:37:31PM -0700, Bjorn Andersson wrote:
> > > > In some configurations, the exact placement of the rmtfs shared memory
> > > > region isn't so strict. The DeviceTree author can then choose to use the
> > > > "size" property and rely on the OS for placement (in combination with
> > > > "alloc-ranges", if desired).
> > > >
> > > > But on some platforms the rmtfs memory region may not be allocated
> > > > adjacent to regions allocated by other clients. Add support for
> > > > discarding the first and last 4k block in the region, if
> > > > qcom,use-guard-pages is specified in DeviceTree.
> > > >
> > > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > > ---
> > > > drivers/soc/qcom/rmtfs_mem.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > > > index f83811f51175..83bba9321e72 100644
> > > > --- a/drivers/soc/qcom/rmtfs_mem.c
> > > > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > > > @@ -200,6 +200,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> > > > rmtfs_mem->client_id = client_id;
> > > > rmtfs_mem->size = rmem->size;
> > > >
> > > > + /*
> > > > + * If requested, discard the first and last 4k block in order to ensure
> > > > + * that the rmtfs region isn't adjacent to other protected regions.
> > > > + */
> > > > + if (of_property_present(node, "qcom,use-guard-pages")) {
> > >
> > > I think of_property_read_bool() would be more fitting here. Right now
> > > of_property_present() is just a wrapper around of_property_read_bool().
> > > Semantically reading a bool fits better here though. :-)
> > >
> >
> > Are you saying that you would prefer this to be a bool, so hat you can
> > give it a "false" value? Or you are simply saying "it walks like a
> > boolean, quacks like a boolean, let's use the boolean accessor"?
> >
>
> The latter. I would expect that of_property_present() is used for
> properties which usually have a value, while of_property_read_bool()
> is used for pure bool values which can be present or not but must not
> have a value. I think a "bool" in terms of DT is simply a present or
> not-present property without any value?
>
> For example consider
>
> regulator-min-microvolts = <4200000000>;
> regulator-always-on;
>
> Then I would expect
>
> - of_property_present(..., "regulator-min-microvolts"), but
> - of_property_read_bool(..., "regulator-always-on")
>
> Does that make sense? :D
>
Certainly, of_property_read_duck() it is.
Thanks,
Bjorn
On 9/21/23 04:37, Bjorn Andersson wrote:
> In some configurations, the exact placement of the rmtfs shared memory
> region isn't so strict. The DeviceTree author can then choose to use the
> "size" property and rely on the OS for placement (in combination with
> "alloc-ranges", if desired).
>
> But on some platforms the rmtfs memory region may not be allocated
> adjacent to regions allocated by other clients. Add support for
> discarding the first and last 4k block in the region, if
> qcom,use-guard-pages is specified in DeviceTree.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
I don't want to block this anymore, but I guess I should ask
the question whether it would be valuable to add a common
reserved-memory property for e.g. low-padding and high-padding
Have we seen cases of that outside rmtfs?
Konrad
On Wed, 20 Sep 2023 19:37:29 -0700, Bjorn Andersson wrote:
> Some platforms have laxed requirements on the placement of the rmtfs
> memory region, introduce support for guard pages to allow the DeviceTree
> author to rely on the OS/Linux for placement of the region.
>
> Changes since v2:
> - Rewrote DeviceTree binding description, to avoid dictating the OS
> behavior.
> - Adjusted addr and size before memremap(), to avoid mapping the guard
> pages, and unnecessarily have to adjust the base pointer.
>
> [...]
Applied, thanks!
[1/3] dt-bindings: reserved-memory: rmtfs: Allow guard pages
commit: 3ad96787949a96256931ca59aff73ea604bc0e6c
[2/3] soc: qcom: rmtfs: Support discarding guard pages
commit: 9265bc6bce6919c771970e5a425a66551a1c78a0
Best regards,
--
Bjorn Andersson <[email protected]>