[PATCH 0/2] soc: qcom: rmtfs: Support dynamic allocation

[PATCH 0/2] soc: qcom: rmtfs: Support dynamic allocation

[Bjorn Andersson]

Some platforms have laxed requirements on the placement of the rmtfs
memory region, introduce a mechanism to allow the DeviceTree source
author to give the responsibility of the placement of this region to the
OS.

Bjorn Andersson (2):
  dt-bindings: reserved-memory: rmtfs: Allow dynamic allocation
  soc: qcom: rmtfs: Support dynamic placement of region

 .../reserved-memory/qcom,rmtfs-mem.yaml       | 23 ++++++-
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts       | 10 +++
 drivers/soc/qcom/rmtfs_mem.c                  | 66 ++++++++++++++-----
 3 files changed, 81 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH 1/2] dt-bindings: reserved-memory: rmtfs: Allow dynamic allocation

[Bjorn Andersson]

Allow instances of the qcom,rmtfs-mem either be defined as a
reserved-memory regoin, or just standalone given just a size.

This relieve the DeviceTree source author the need to come up with a
static memory region for the region.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 .../reserved-memory/qcom,rmtfs-mem.yaml       | 23 ++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
index bab982f00485..8b5de033f9ac 100644
--- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
@@ -14,13 +14,16 @@ description: |
 maintainers:
   - Bjorn Andersson <bjorn.andersson@linaro.org>
 
-allOf:
-  - $ref: reserved-memory.yaml
-
 properties:
   compatible:
     const: qcom,rmtfs-mem
 
+  qcom,alloc-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Requested size of the rmtfs memory allocation, when not defined as a
+      reserved-memory region.
+
   qcom,client-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: >
@@ -36,6 +39,11 @@ properties:
 required:
   - qcom,client-id
 
+oneOf:
+  - $ref: reserved-memory.yaml
+  - required:
+      - qcom,alloc-size
+
 unevaluatedProperties: false
 
 examples:
@@ -53,3 +61,12 @@ examples:
             qcom,client-id = <1>;
         };
     };
+  - |
+    rmtfs {
+        compatible = "qcom,rmtfs-mem";
+
+        qcom,alloc-size = <(2*1024*1024)>;
+        qcom,client-id = <1>;
+        qcom,vmid = <15>;
+    };
+...
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: reserved-memory: rmtfs: Allow dynamic allocation

[Konrad Dybcio]

On 30.05.2023 21:34, Bjorn Andersson wrote:
> Allow instances of the qcom,rmtfs-mem either be defined as a
> reserved-memory regoin, or just standalone given just a size.
> 
> This relieve the DeviceTree source author the need to come up with a
> static memory region for the region.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  .../reserved-memory/qcom,rmtfs-mem.yaml       | 23 ++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> index bab982f00485..8b5de033f9ac 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> @@ -14,13 +14,16 @@ description: |
>  maintainers:
>    - Bjorn Andersson <bjorn.andersson@linaro.org>
>  
> -allOf:
> -  - $ref: reserved-memory.yaml
> -
>  properties:
>    compatible:
>      const: qcom,rmtfs-mem
>  
> +  qcom,alloc-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Requested size of the rmtfs memory allocation, when not defined as a
> +      reserved-memory region.
> +
>    qcom,client-id:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: >
> @@ -36,6 +39,11 @@ properties:
>  required:
>    - qcom,client-id
>  
> +oneOf:
> +  - $ref: reserved-memory.yaml
> +  - required:
> +      - qcom,alloc-size
> +
>  unevaluatedProperties: false
>  
>  examples:
> @@ -53,3 +61,12 @@ examples:
>              qcom,client-id = <1>;
>          };
>      };
> +  - |
> +    rmtfs {
> +        compatible = "qcom,rmtfs-mem";
> +
> +        qcom,alloc-size = <(2*1024*1024)>;
2 nitty nits:

- Most uses of DT arithmetic put spaces between the operands
- You could add a comment explaining what this example brings to
  the table

Konrad

> +        qcom,client-id = <1>;
> +        qcom,vmid = <15>;
> +    };
> +...

Re: [PATCH 1/2] dt-bindings: reserved-memory: rmtfs: Allow dynamic allocation

[Krzysztof Kozlowski]

On 30/05/2023 21:34, Bjorn Andersson wrote:
> Allow instances of the qcom,rmtfs-mem either be defined as a
> reserved-memory regoin, or just standalone given just a size.

typo: region

I am pretty sure I saw some patches from Qualcomm also making one of
reserved-memory users a bit more dynamic (some boot-thingy?). Would be
nice to unify...

> 
> This relieve the DeviceTree source author the need to come up with a
> static memory region for the region.

If you region does not have to be static, why bothering with the size in
DT? I assume this can be really dynamic and nothing is needed in DT. Not
even size.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: reserved-memory: rmtfs: Allow dynamic allocation

[Bjorn Andersson]

On Wed, May 31, 2023 at 10:05:50AM +0200, Krzysztof Kozlowski wrote:
> On 30/05/2023 21:34, Bjorn Andersson wrote:
> > Allow instances of the qcom,rmtfs-mem either be defined as a
> > reserved-memory regoin, or just standalone given just a size.
> 
> typo: region
> 
> I am pretty sure I saw some patches from Qualcomm also making one of
> reserved-memory users a bit more dynamic (some boot-thingy?). Would be
> nice to unify...
> 
> > 
> > This relieve the DeviceTree source author the need to come up with a
> > static memory region for the region.
> 
> If you region does not have to be static, why bothering with the size in
> DT? I assume this can be really dynamic and nothing is needed in DT. Not
> even size.
> 

The size, client-id and vmid need to match the firmware and access
policy configuration, and are as such device-specific properties for the
region.

The desired size is available during the rmtfs handshake, so it's
plausible that one could come up with a mechanism of dynamic allocation.
But this would be complicated (as I'd prefer not to have the rmtfs
service in the kernel...), and we'd still end up with something in DT to
affect placement etc - often a reserved-memory region to specifically
define the placement.

We'd still need the client-id and vmid properties, and we still need
some mechanism to instantiate the rmtfs service and something that will
configure the access permissions for the region...

Regards,
Bjorn

[PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region

[Bjorn Andersson]

In some configurations, the exact placement of the rmtfs shared memory
region isn't so strict. In the current implementation the author of the
DeviceTree source is forced to make up a memory region.

Extend the rmtfs memory driver to relieve the author of this
responsibility by introducing support for using dynamic allocation in
the driver.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 10 ++++
 drivers/soc/qcom/rmtfs_mem.c            | 66 +++++++++++++++++++------
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index d1440b790fa6..e6191b8ba4c6 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -12,6 +12,8 @@
 #include "pm8998.dtsi"
 #include "pmi8998.dtsi"
 
+/delete-node/ &rmtfs_mem;
+
 / {
 	model = "Qualcomm Technologies, Inc. SDM845 MTP";
 	compatible = "qcom,sdm845-mtp", "qcom,sdm845";
@@ -48,6 +50,14 @@ vreg_s4a_1p8: pm8998-smps4 {
 		vin-supply = <&vph_pwr>;
 	};
 
+	rmtfs {
+		compatible = "qcom,rmtfs-mem";
+
+		qcom,alloc-size = <(2*1024*1024)>;
+		qcom,client-id = <1>;
+		qcom,vmid = <15>;
+	};
+
 	thermal-zones {
 		xo_thermal: xo-thermal {
 			polling-delay-passive = <0>;
diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index f83811f51175..5f56ded9f905 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2017 Linaro Ltd.
  */
 
+#include "linux/gfp_types.h"
+#include "linux/sizes.h"
 #include <linux/kernel.h>
 #include <linux/cdev.h>
 #include <linux/err.h>
@@ -168,23 +170,63 @@ static void qcom_rmtfs_mem_release_device(struct device *dev)
 	kfree(rmtfs_mem);
 }
 
+static int qcom_rmtfs_acquire_mem(struct device *dev, struct qcom_rmtfs_mem *rmtfs_mem)
+{
+	struct device_node *node = dev->of_node;
+	struct reserved_mem *rmem;
+	dma_addr_t dma_addr;
+	void *mem;
+	u32 size;
+	int ret;
+
+	rmem = of_reserved_mem_lookup(node);
+	if (rmem) {
+		rmtfs_mem->addr = rmem->base;
+		rmtfs_mem->size = rmem->size;
+
+		rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
+						rmtfs_mem->size, MEMREMAP_WC);
+		if (IS_ERR(rmtfs_mem->base)) {
+			dev_err(dev, "failed to remap rmtfs_mem region\n");
+			return PTR_ERR(rmtfs_mem->base);
+		}
+
+		return 0;
+	}
+
+	ret = of_property_read_u32(node, "qcom,alloc-size", &size);
+	if (ret < 0) {
+		dev_err(dev, "rmtfs of unknown size\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Ensure that the protected region isn't adjacent to other protected
+	 * regions by allocating an empty page on either side.
+	 */
+	mem = dma_alloc_coherent(dev, size + 2 * SZ_4K, &dma_addr, GFP_KERNEL);
+	if (mem) {
+		rmtfs_mem->base = mem + SZ_4K;
+		rmtfs_mem->addr = dma_addr + SZ_4K;
+		rmtfs_mem->size = size;
+
+		return 0;
+	}
+
+	dev_err(dev, "unable to allocate memory for rmtfs mem\n");
+	return -ENOMEM;
+}
+
 static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
 	struct qcom_scm_vmperm perms[NUM_MAX_VMIDS + 1];
-	struct reserved_mem *rmem;
 	struct qcom_rmtfs_mem *rmtfs_mem;
 	u32 client_id;
 	u32 vmid[NUM_MAX_VMIDS];
 	int num_vmids;
 	int ret, i;
 
-	rmem = of_reserved_mem_lookup(node);
-	if (!rmem) {
-		dev_err(&pdev->dev, "failed to acquire memory region\n");
-		return -EINVAL;
-	}
-
 	ret = of_property_read_u32(node, "qcom,client-id", &client_id);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to parse \"qcom,client-id\"\n");
@@ -196,22 +238,16 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 	if (!rmtfs_mem)
 		return -ENOMEM;
 
-	rmtfs_mem->addr = rmem->base;
 	rmtfs_mem->client_id = client_id;
-	rmtfs_mem->size = rmem->size;
 
 	device_initialize(&rmtfs_mem->dev);
 	rmtfs_mem->dev.parent = &pdev->dev;
 	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
 	rmtfs_mem->dev.release = qcom_rmtfs_mem_release_device;
 
-	rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
-					rmtfs_mem->size, MEMREMAP_WC);
-	if (IS_ERR(rmtfs_mem->base)) {
-		dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n");
-		ret = PTR_ERR(rmtfs_mem->base);
+	ret = qcom_rmtfs_acquire_mem(&pdev->dev, rmtfs_mem);
+	if (ret < 0)
 		goto put_device;
-	}
 
 	cdev_init(&rmtfs_mem->cdev, &qcom_rmtfs_mem_fops);
 	rmtfs_mem->cdev.owner = THIS_MODULE;
-- 
2.25.1

Re: [PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region

[Konrad Dybcio]

On 30.05.2023 21:34, Bjorn Andersson wrote:
> In some configurations, the exact placement of the rmtfs shared memory
> region isn't so strict. In the current implementation the author of the
> DeviceTree source is forced to make up a memory region.
IIUC the test here would be... "works" / "doesn't", just as if one
misplaced the fixed region?

Does the downstream sharedmem-uio driver do any additional cryptic
magic or does it simply rely on the vendor's cma/dma pool settings?
Can we replicate its behavior to stop hardcoding rmtfs, period?

> 
> Extend the rmtfs memory driver to relieve the author of this
> responsibility by introducing support for using dynamic allocation in
> the driver.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 10 ++++
>  drivers/soc/qcom/rmtfs_mem.c            | 66 +++++++++++++++++++------
>  2 files changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index d1440b790fa6..e6191b8ba4c6 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -12,6 +12,8 @@
>  #include "pm8998.dtsi"
>  #include "pmi8998.dtsi"
>  
> +/delete-node/ &rmtfs_mem;
> +
>  / {
>  	model = "Qualcomm Technologies, Inc. SDM845 MTP";
>  	compatible = "qcom,sdm845-mtp", "qcom,sdm845";
> @@ -48,6 +50,14 @@ vreg_s4a_1p8: pm8998-smps4 {
>  		vin-supply = <&vph_pwr>;
>  	};
>  
> +	rmtfs {
> +		compatible = "qcom,rmtfs-mem";
> +
> +		qcom,alloc-size = <(2*1024*1024)>;
> +		qcom,client-id = <1>;
> +		qcom,vmid = <15>;
> +	};
This should have been a separate patch.

> +
>  	thermal-zones {
>  		xo_thermal: xo-thermal {
>  			polling-delay-passive = <0>;
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index f83811f51175..5f56ded9f905 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -3,6 +3,8 @@
>   * Copyright (c) 2017 Linaro Ltd.
>   */
>  
> +#include "linux/gfp_types.h"
> +#include "linux/sizes.h"
<>?

>  #include <linux/kernel.h>
>  #include <linux/cdev.h>
>  #include <linux/err.h>
> @@ -168,23 +170,63 @@ static void qcom_rmtfs_mem_release_device(struct device *dev)
>  	kfree(rmtfs_mem);
>  }
>  
> +static int qcom_rmtfs_acquire_mem(struct device *dev, struct qcom_rmtfs_mem *rmtfs_mem)
> +{
> +	struct device_node *node = dev->of_node;
> +	struct reserved_mem *rmem;
> +	dma_addr_t dma_addr;
> +	void *mem;
> +	u32 size;
> +	int ret;
> +
> +	rmem = of_reserved_mem_lookup(node);
> +	if (rmem) {
> +		rmtfs_mem->addr = rmem->base;
> +		rmtfs_mem->size = rmem->size;
> +
> +		rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
> +						rmtfs_mem->size, MEMREMAP_WC);
> +		if (IS_ERR(rmtfs_mem->base)) {
> +			dev_err(dev, "failed to remap rmtfs_mem region\n");
> +			return PTR_ERR(rmtfs_mem->base);
> +		}
> +
> +		return 0;
> +	}
> +
> +	ret = of_property_read_u32(node, "qcom,alloc-size", &size);
> +	if (ret < 0) {
> +		dev_err(dev, "rmtfs of unknown size\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Ensure that the protected region isn't adjacent to other protected
> +	 * regions by allocating an empty page on either side.
> +	 */
> +	mem = dma_alloc_coherent(dev, size + 2 * SZ_4K, &dma_addr, GFP_KERNEL);
Should this be made pagesize-independent? Can we even run non-4K kernels on msm?

Konrad
> +	if (mem) {
> +		rmtfs_mem->base = mem + SZ_4K;
> +		rmtfs_mem->addr = dma_addr + SZ_4K;
> +		rmtfs_mem->size = size;
> +
> +		return 0;
> +	}
> +
> +	dev_err(dev, "unable to allocate memory for rmtfs mem\n");
> +	return -ENOMEM;
> +}
> +
>  static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node = pdev->dev.of_node;
>  	struct qcom_scm_vmperm perms[NUM_MAX_VMIDS + 1];
> -	struct reserved_mem *rmem;
>  	struct qcom_rmtfs_mem *rmtfs_mem;
>  	u32 client_id;
>  	u32 vmid[NUM_MAX_VMIDS];
>  	int num_vmids;
>  	int ret, i;
>  
> -	rmem = of_reserved_mem_lookup(node);
> -	if (!rmem) {
> -		dev_err(&pdev->dev, "failed to acquire memory region\n");
> -		return -EINVAL;
> -	}
> -
>  	ret = of_property_read_u32(node, "qcom,client-id", &client_id);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to parse \"qcom,client-id\"\n");
> @@ -196,22 +238,16 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
>  	if (!rmtfs_mem)
>  		return -ENOMEM;
>  
> -	rmtfs_mem->addr = rmem->base;
>  	rmtfs_mem->client_id = client_id;
> -	rmtfs_mem->size = rmem->size;
>  
>  	device_initialize(&rmtfs_mem->dev);
>  	rmtfs_mem->dev.parent = &pdev->dev;
>  	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
>  	rmtfs_mem->dev.release = qcom_rmtfs_mem_release_device;
>  
> -	rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
> -					rmtfs_mem->size, MEMREMAP_WC);
> -	if (IS_ERR(rmtfs_mem->base)) {
> -		dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n");
> -		ret = PTR_ERR(rmtfs_mem->base);
> +	ret = qcom_rmtfs_acquire_mem(&pdev->dev, rmtfs_mem);
> +	if (ret < 0)
>  		goto put_device;
> -	}
>  
>  	cdev_init(&rmtfs_mem->cdev, &qcom_rmtfs_mem_fops);
>  	rmtfs_mem->cdev.owner = THIS_MODULE;

Re: [PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region

[Bjorn Andersson]

On Tue, May 30, 2023 at 09:45:10PM +0200, Konrad Dybcio wrote:
> 
> 
> On 30.05.2023 21:34, Bjorn Andersson wrote:
> > In some configurations, the exact placement of the rmtfs shared memory
> > region isn't so strict. In the current implementation the author of the
> > DeviceTree source is forced to make up a memory region.
> IIUC the test here would be... "works" / "doesn't", just as if one
> misplaced the fixed region?
> 

The patch makes no effort to clarify this part.

> Does the downstream sharedmem-uio driver do any additional cryptic
> magic or does it simply rely on the vendor's cma/dma pool settings?
> Can we replicate its behavior to stop hardcoding rmtfs, period?
> 

Alignment on that is the intention with this patchset.

> > 
> > Extend the rmtfs memory driver to relieve the author of this
> > responsibility by introducing support for using dynamic allocation in
> > the driver.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 10 ++++
> >  drivers/soc/qcom/rmtfs_mem.c            | 66 +++++++++++++++++++------
> >  2 files changed, 61 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > index d1440b790fa6..e6191b8ba4c6 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > @@ -12,6 +12,8 @@
> >  #include "pm8998.dtsi"
> >  #include "pmi8998.dtsi"
> >  
> > +/delete-node/ &rmtfs_mem;
> > +
> >  / {
> >  	model = "Qualcomm Technologies, Inc. SDM845 MTP";
> >  	compatible = "qcom,sdm845-mtp", "qcom,sdm845";
> > @@ -48,6 +50,14 @@ vreg_s4a_1p8: pm8998-smps4 {
> >  		vin-supply = <&vph_pwr>;
> >  	};
> >  
> > +	rmtfs {
> > +		compatible = "qcom,rmtfs-mem";
> > +
> > +		qcom,alloc-size = <(2*1024*1024)>;
> > +		qcom,client-id = <1>;
> > +		qcom,vmid = <15>;
> > +	};
> This should have been a separate patch.
> 

Of course, I should have paid more attention when I did the last git
add, to not include test code...

> > +
> >  	thermal-zones {
> >  		xo_thermal: xo-thermal {
> >  			polling-delay-passive = <0>;
> > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > index f83811f51175..5f56ded9f905 100644
> > --- a/drivers/soc/qcom/rmtfs_mem.c
> > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > @@ -3,6 +3,8 @@
> >   * Copyright (c) 2017 Linaro Ltd.
> >   */
> >  
> > +#include "linux/gfp_types.h"
> > +#include "linux/sizes.h"
> <>?
> 
> >  #include <linux/kernel.h>
> >  #include <linux/cdev.h>
> >  #include <linux/err.h>
> > @@ -168,23 +170,63 @@ static void qcom_rmtfs_mem_release_device(struct device *dev)
> >  	kfree(rmtfs_mem);
> >  }
> >  
> > +static int qcom_rmtfs_acquire_mem(struct device *dev, struct qcom_rmtfs_mem *rmtfs_mem)
> > +{
> > +	struct device_node *node = dev->of_node;
> > +	struct reserved_mem *rmem;
> > +	dma_addr_t dma_addr;
> > +	void *mem;
> > +	u32 size;
> > +	int ret;
> > +
> > +	rmem = of_reserved_mem_lookup(node);
> > +	if (rmem) {
> > +		rmtfs_mem->addr = rmem->base;
> > +		rmtfs_mem->size = rmem->size;
> > +
> > +		rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
> > +						rmtfs_mem->size, MEMREMAP_WC);
> > +		if (IS_ERR(rmtfs_mem->base)) {
> > +			dev_err(dev, "failed to remap rmtfs_mem region\n");
> > +			return PTR_ERR(rmtfs_mem->base);
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	ret = of_property_read_u32(node, "qcom,alloc-size", &size);
> > +	if (ret < 0) {
> > +		dev_err(dev, "rmtfs of unknown size\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Ensure that the protected region isn't adjacent to other protected
> > +	 * regions by allocating an empty page on either side.
> > +	 */
> > +	mem = dma_alloc_coherent(dev, size + 2 * SZ_4K, &dma_addr, GFP_KERNEL);
> Should this be made pagesize-independent? Can we even run non-4K kernels on msm?
> 

Yes, I fixed the issue in UFS and I believe Alex corrected the bug in
IPA. With that I've been able to boot the few platforms where I've tried
it with 16KB PAGE_SIZE.

That's however the Linux page size, the numbers here relates to things
on the secure side.

Regards,
Bjorn

> Konrad
> > +	if (mem) {
> > +		rmtfs_mem->base = mem + SZ_4K;
> > +		rmtfs_mem->addr = dma_addr + SZ_4K;
> > +		rmtfs_mem->size = size;
> > +
> > +		return 0;
> > +	}
> > +
> > +	dev_err(dev, "unable to allocate memory for rmtfs mem\n");
> > +	return -ENOMEM;
> > +}
> > +
> >  static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *node = pdev->dev.of_node;
> >  	struct qcom_scm_vmperm perms[NUM_MAX_VMIDS + 1];
> > -	struct reserved_mem *rmem;
> >  	struct qcom_rmtfs_mem *rmtfs_mem;
> >  	u32 client_id;
> >  	u32 vmid[NUM_MAX_VMIDS];
> >  	int num_vmids;
> >  	int ret, i;
> >  
> > -	rmem = of_reserved_mem_lookup(node);
> > -	if (!rmem) {
> > -		dev_err(&pdev->dev, "failed to acquire memory region\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	ret = of_property_read_u32(node, "qcom,client-id", &client_id);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "failed to parse \"qcom,client-id\"\n");
> > @@ -196,22 +238,16 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> >  	if (!rmtfs_mem)
> >  		return -ENOMEM;
> >  
> > -	rmtfs_mem->addr = rmem->base;
> >  	rmtfs_mem->client_id = client_id;
> > -	rmtfs_mem->size = rmem->size;
> >  
> >  	device_initialize(&rmtfs_mem->dev);
> >  	rmtfs_mem->dev.parent = &pdev->dev;
> >  	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
> >  	rmtfs_mem->dev.release = qcom_rmtfs_mem_release_device;
> >  
> > -	rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
> > -					rmtfs_mem->size, MEMREMAP_WC);
> > -	if (IS_ERR(rmtfs_mem->base)) {
> > -		dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n");
> > -		ret = PTR_ERR(rmtfs_mem->base);
> > +	ret = qcom_rmtfs_acquire_mem(&pdev->dev, rmtfs_mem);
> > +	if (ret < 0)
> >  		goto put_device;
> > -	}
> >  
> >  	cdev_init(&rmtfs_mem->cdev, &qcom_rmtfs_mem_fops);
> >  	rmtfs_mem->cdev.owner = THIS_MODULE;

Re: [PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region

[Stephan Gerhold]

On Tue, May 30, 2023 at 12:34:36PM -0700, Bjorn Andersson wrote:
> In some configurations, the exact placement of the rmtfs shared memory
> region isn't so strict. In the current implementation the author of the
> DeviceTree source is forced to make up a memory region.
> 
> Extend the rmtfs memory driver to relieve the author of this
> responsibility by introducing support for using dynamic allocation in
> the driver.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 10 ++++
>  drivers/soc/qcom/rmtfs_mem.c            | 66 +++++++++++++++++++------
>  2 files changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index d1440b790fa6..e6191b8ba4c6 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -12,6 +12,8 @@
>  #include "pm8998.dtsi"
>  #include "pmi8998.dtsi"
>  
> +/delete-node/ &rmtfs_mem;
> +
>  / {
>  	model = "Qualcomm Technologies, Inc. SDM845 MTP";
>  	compatible = "qcom,sdm845-mtp", "qcom,sdm845";
> @@ -48,6 +50,14 @@ vreg_s4a_1p8: pm8998-smps4 {
>  		vin-supply = <&vph_pwr>;
>  	};
>  
> +	rmtfs {
> +		compatible = "qcom,rmtfs-mem";
> +
> +		qcom,alloc-size = <(2*1024*1024)>;
> +		qcom,client-id = <1>;
> +		qcom,vmid = <15>;
> +	};
> +

Couldn't you just use the existing dynamic allocation of
reserved-memory, without any driver changes?

/ {
	reserved-memory {
		rmtfs {
			compatible = "qcom,rmtfs-mem";
			size = <0x0 (2*1024*1024)>;
			alignment = <0x0 ...>; // if you want a special one
			no-map; // don't we want to map this actually?

			qcom,client-id = <1>;
			qcom,vmid = <15>;
		};
	};
};

You won't get the 4K empty pages but I guess you just have them because
you allocate the memory without proper alignment?

Related patch series where I propose using it for most firmware memory
regions:
https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/

Thanks,
Stephan

Re: [PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region

[Bjorn Andersson]

On Tue, May 30, 2023 at 09:48:46PM +0200, Stephan Gerhold wrote:
> On Tue, May 30, 2023 at 12:34:36PM -0700, Bjorn Andersson wrote:
> > In some configurations, the exact placement of the rmtfs shared memory
> > region isn't so strict. In the current implementation the author of the
> > DeviceTree source is forced to make up a memory region.
> > 
> > Extend the rmtfs memory driver to relieve the author of this
> > responsibility by introducing support for using dynamic allocation in
> > the driver.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 10 ++++
> >  drivers/soc/qcom/rmtfs_mem.c            | 66 +++++++++++++++++++------
> >  2 files changed, 61 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > index d1440b790fa6..e6191b8ba4c6 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > @@ -12,6 +12,8 @@
> >  #include "pm8998.dtsi"
> >  #include "pmi8998.dtsi"
> >  
> > +/delete-node/ &rmtfs_mem;
> > +
> >  / {
> >  	model = "Qualcomm Technologies, Inc. SDM845 MTP";
> >  	compatible = "qcom,sdm845-mtp", "qcom,sdm845";
> > @@ -48,6 +50,14 @@ vreg_s4a_1p8: pm8998-smps4 {
> >  		vin-supply = <&vph_pwr>;
> >  	};
> >  
> > +	rmtfs {
> > +		compatible = "qcom,rmtfs-mem";
> > +
> > +		qcom,alloc-size = <(2*1024*1024)>;
> > +		qcom,client-id = <1>;
> > +		qcom,vmid = <15>;
> > +	};
> > +
> 
> Couldn't you just use the existing dynamic allocation of
> reserved-memory, without any driver changes?
> 

That should give us a similar end result, but we have alloc-ranges as
well, if the placement needs to be further refined...

> / {
> 	reserved-memory {
> 		rmtfs {
> 			compatible = "qcom,rmtfs-mem";
> 			size = <0x0 (2*1024*1024)>;
> 			alignment = <0x0 ...>; // if you want a special one
> 			no-map; // don't we want to map this actually?
> 
> 			qcom,client-id = <1>;
> 			qcom,vmid = <15>;
> 		};
> 	};
> };
> 
> You won't get the 4K empty pages but I guess you just have them because
> you allocate the memory without proper alignment?
> 

With dynamic placement there's no guarantee that the region isn't
physically adjacent to another protected region, so this needs to be
handled somehow.

Perhaps the intention to include guard pages can be derived from the
size...

> Related patch series where I propose using it for most firmware memory
> regions:
> https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> 

Thanks for the suggestion,
Bjorn

> Thanks,
> Stephan