[PATCH v4 0/2] misc: fastrpc: FastRPC reserved memory assignment for SDM845 SLPI

[PATCH v4 0/2] misc: fastrpc: FastRPC reserved memory assignment for SDM845 SLPI

[Dylan Van Assche]

* About *

The Qualcomm SDM845 SoC has a separate SLPI (Sensor Low Power Island)
DSP for sensors connected to the SoC which is responsible for exposing
sensors to userspace, power saving, and other features. 
While sensors are connected to GPIOs of the SoC, they cannot be used
because the hypervisor blocks direct access to the sensors, thus the 
DSP must be used to access any sensor on this SoC. The SLPI DSP uses a
GLink edge (dsps) to communicate with the host and has a FastRPC interface
to load files from the host filesystem such as sensor configuration files.
The FastRPC interface does not use regular FastRPC Compute Banks
but instead uses an allocated CMA region through which communication happens.

* Changes *

This patchseries add support to the FastRPC for assigning a coherent memory
region to a DSP via the hypervisor with the correct permissions.
This is necessary to support the SLPI found in the Qualcomm SDM845 SoC which
does not have dedicated FastRPC Compute Banks, in contrast to newer SoCs,
but uses a memory region instead when allocating buffers.

* Related patches *

1. Remoteproc changes to support the SLPI DSP in SDM845 (v3), needs to be applied:
https://lore.kernel.org/linux-remoteproc/20230330164633.117335-1-me@dylanvanassche.be
2. DTS changes (v5), already applied:
https://lore.kernel.org/linux-devicetree/20230406173148.28309-1-me@dylanvanassche.be

This serie does not depend on any serie, but all of them are necessary
to enable the feature in the end.

* Changelog *

Changes in v4:
- Fixed possible memory leak when driver encounters an error during probing.

Changes in v3:
- Dropped debug prints.
- Added Reviewed-By tags from v2.

Changes in v2:

- Removed double blank lines
- Dropped dt-bindings property as it is not needed for driver behavior
- Add additional patch to allocate buffers via CMA memory for DSPs
  without dedicated FastRPC Compute Banks.

Dylan Van Assche (2):
  misc: fastrpc: support complete DMA pool access to the DSP
  misc: fastrpc: use coherent pool for untranslated Compute Banks

 drivers/misc/fastrpc.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

-- 
2.40.1

[PATCH v4 1/2] misc: fastrpc: support complete DMA pool access to the DSP

[Dylan Van Assche]

To support FastRPC Context Banks which aren't mapped via the SMMU,
make the whole reserved memory region available to the DSP to allow
access to coherent buffers.

This is performed by assigning the memory to the DSP via a hypervisor
call to set the correct permissions for the Virtual Machines on the DSP.
This is only necessary when a memory region is provided for SLPI DSPs
so guard this with a domain ID check.

Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/misc/fastrpc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f48466960f1b..1ced553ae959 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2231,6 +2231,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	int i, err, domain_id = -1, vmcount;
 	const char *domain;
 	bool secure_dsp;
+	struct device_node *rmem_node;
+	struct reserved_mem *rmem;
 	unsigned int vmids[FASTRPC_MAX_VMIDS];
 
 	err = of_property_read_string(rdev->of_node, "label", &domain);
@@ -2274,6 +2276,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		}
 	}
 
+	rmem_node = of_parse_phandle(rdev->of_node, "memory-region", 0);
+	if (domain_id == SDSP_DOMAIN_ID && rmem_node) {
+		rmem = of_reserved_mem_lookup(rmem_node);
+		if (!rmem) {
+			err = -EINVAL;
+			goto fdev_error;
+		}
+
+		qcom_scm_assign_mem(rmem->base, rmem->size, &data->perms,
+				    data->vmperms, data->vmcount);
+
+	}
+
 	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
 	data->secure = secure_dsp;
 
-- 
2.40.1

Re: [PATCH v4 1/2] misc: fastrpc: support complete DMA pool access to the DSP

[Srinivas Kandagatla]

On 11/05/2023 15:11, Dylan Van Assche wrote:
> To support FastRPC Context Banks which aren't mapped via the SMMU,
> make the whole reserved memory region available to the DSP to allow
> access to coherent buffers.

Mapping the whole region sounds very inefficient, and also possibly 
making the cma region not usable by others.

> 

AFAIU SDM845 does not have any context banks for SDSP. All new SoCs 
after 865 have moved to having a context bank.

For such cases (w/o cb) we can make fastrpc_session_alloc use channel 
context device instead of session ctx device. As this is going to be an 
issue when we try to allocate buffers dynamically for that cb.

In the newer platforms (from 865) there is support for iommu and context 
banks on SDSP, so the existing code flow is identical for both ADSP and 
SDSP.


We should be careful not to break newer platfroms while adding support 
to this.

Both myself and Ekansh thought about this and see that the better way to 
add support to this is by

1. extend fastrpc_session_alloc() to support zero context banks.

2. add flags to mark this and allocate meta data using secure allocation 
when its required based on this flag.

3.  buffer allocation can either go with 2 or with a new flag coming 
from userspace.



> This is performed by assigning the memory to the DSP via a hypervisor
> call to set the correct permissions for the Virtual Machines on the DSP.
> This is only necessary when a memory region is provided for SLPI DSPs
> so guard this with a domain ID check.
> 
> Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   drivers/misc/fastrpc.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f48466960f1b..1ced553ae959 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2231,6 +2231,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   	int i, err, domain_id = -1, vmcount;
>   	const char *domain;
>   	bool secure_dsp;
> +	struct device_node *rmem_node;
> +	struct reserved_mem *rmem;
>   	unsigned int vmids[FASTRPC_MAX_VMIDS];
>   
>   	err = of_property_read_string(rdev->of_node, "label", &domain);
> @@ -2274,6 +2276,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   		}
>   	}
>   
> +	rmem_node = of_parse_phandle(rdev->of_node, "memory-region", 0);
> +	if (domain_id == SDSP_DOMAIN_ID && rmem_node) {
> +		rmem = of_reserved_mem_lookup(rmem_node);
> +		if (!rmem) {
> +			err = -EINVAL;
> +			goto fdev_error;
> +		}
> +
> +		qcom_scm_assign_mem(rmem->base, rmem->size, &data->perms,
> +				    data->vmperms, data->vmcount);

vmperms need to be a bit field.

> +
> +	}
> +
>   	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
>   	data->secure = secure_dsp;
>   

--srini

Re: [PATCH v4 1/2] misc: fastrpc: support complete DMA pool access to the DSP

[Caleb Connolly]

On 30/05/2023 11:52, Srinivas Kandagatla wrote:
> On 11/05/2023 15:11, Dylan Van Assche wrote:
>> To support FastRPC Context Banks which aren't mapped via the SMMU,
>> make the whole reserved memory region available to the DSP to allow
>> access to coherent buffers.
> 
> Mapping the whole region sounds very inefficient, and also possibly
> making the cma region not usable by others.

Probably I'm missing something obvious here... The downstream driver
maps the whole region, what are the advantages to doing it on a
per-allocation basis?

What are the other users?
> 
>>
> 
> AFAIU SDM845 does not have any context banks for SDSP. All new SoCs
> after 865 have moved to having a context bank.
> 
> For such cases (w/o cb) we can make fastrpc_session_alloc use channel
> context device instead of session ctx device. As this is going to be an
> issue when we try to allocate buffers dynamically for that cb.

Right.. This is what patch 2 does, but presumably the ALLOC_DMA_BUF,
MMAP, etc ioctls won't work with the current iteration?

> 
> In the newer platforms (from 865) there is support for iommu and context
> banks on SDSP, so the existing code flow is identical for both ADSP and
> SDSP.
> 
> 
> We should be careful not to break newer platfroms while adding support
> to this.
> 
> Both myself and Ekansh thought about this and see that the better way to
> add support to this is by
> 
> 1. extend fastrpc_session_alloc() to support zero context banks.
> 
> 2. add flags to mark this and allocate meta data using secure allocation
> when its required based on this flag.
> 
> 3.  buffer allocation can either go with 2 or with a new flag coming
> from userspace.

This sounds pretty good to me!
> 
> 
> 
>> This is performed by assigning the memory to the DSP via a hypervisor
>> call to set the correct permissions for the Virtual Machines on the DSP.
>> This is only necessary when a memory region is provided for SLPI DSPs
>> so guard this with a domain ID check.
>>
>> Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
>> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   drivers/misc/fastrpc.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index f48466960f1b..1ced553ae959 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -2231,6 +2231,8 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>>       int i, err, domain_id = -1, vmcount;
>>       const char *domain;
>>       bool secure_dsp;
>> +    struct device_node *rmem_node;
>> +    struct reserved_mem *rmem;
>>       unsigned int vmids[FASTRPC_MAX_VMIDS];
>>         err = of_property_read_string(rdev->of_node, "label", &domain);
>> @@ -2274,6 +2276,19 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>>           }
>>       }
>>   +    rmem_node = of_parse_phandle(rdev->of_node, "memory-region", 0);
>> +    if (domain_id == SDSP_DOMAIN_ID && rmem_node) {
>> +        rmem = of_reserved_mem_lookup(rmem_node);
>> +        if (!rmem) {
>> +            err = -EINVAL;
>> +            goto fdev_error;
>> +        }
>> +
>> +        qcom_scm_assign_mem(rmem->base, rmem->size, &data->perms,
>> +                    data->vmperms, data->vmcount);
> 
> vmperms need to be a bit field.
> 
>> +
>> +    }
>> +
>>       secure_dsp = !(of_property_read_bool(rdev->of_node,
>> "qcom,non-secure-domain"));
>>       data->secure = secure_dsp;
>>   
> 
> --srini

-- 
// Caleb (they/them)

[PATCH v4 2/2] misc: fastrpc: use coherent pool for untranslated Compute Banks

[Dylan Van Assche]

Use fastrpc_remote_heap_alloc to allocate from the FastRPC device
instead of the Compute Bank when the session ID is 0. This ensures
that the allocation is inside the coherent DMA pool which is already
accessible to the DSP. This is necessary to support FastRPC devices
which do not have dedicated Compute Banks such as the SLPI on the SDM845.
The latter uses an allocated CMA region instead of FastRPC Compute Banks.

Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/misc/fastrpc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 1ced553ae959..3d47d3d13b5d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -939,7 +939,10 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 
 	ctx->msg_sz = pkt_size;
 
-	err = fastrpc_buf_alloc(ctx->fl, dev, pkt_size, &ctx->buf);
+	if (ctx->fl->sctx->sid)
+		err = fastrpc_buf_alloc(ctx->fl, dev, pkt_size, &ctx->buf);
+	else
+		err = fastrpc_remote_heap_alloc(ctx->fl, dev, pkt_size, &ctx->buf);
 	if (err)
 		return err;
 
-- 
2.40.1