Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 May 2023 14:31:37 -0700
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Konrad Dybcio <konrad.dybcio@linaro.org>
CC:     Bjorn Andersson <andersson@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region
Message-ID: <20230530213137.GA3645274@hu-bjorande-lv.qualcomm.com>
References: <20230530193436.3833889-1-quic_bjorande@quicinc.com>
 <20230530193436.3833889-3-quic_bjorande@quicinc.com>
 <0e9903c0-4669-9298-e0ee-72fc775998c3@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <0e9903c0-4669-9298-e0ee-72fc775998c3@linaro.org>
Precedence: bulk

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;