diff mbox series

[v1] misc: fastrpc: Pass proper arguments to scm call

Message ID 20240108100513.19993-1-quic_ekangupt@quicinc.com
State New
Headers
Series [v1] misc: fastrpc: Pass proper arguments to scm call |

Commit Message

Ekansh Gupta Jan. 8, 2024, 10:05 a.m. UTC 
  For CMA memory allocation, ownership is assigned to DSP to make it
accessible by the PD running on the DSP. With current implementation
HLOS VM is stored in the channel structure during rpmsg_probe and
this VM is passed to qcom_scm call as the source VM.

The qcom_scm call will overwrite the passed source VM with the next
VM which would cause a problem in case the scm call is again needed.
Adding a local copy of source VM whereever scm call is made to avoid
this problem.

Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Elliot Berman Jan. 9, 2024, 1:12 a.m. UTC | #1 
On 1/8/2024 2:05 AM, Ekansh Gupta wrote:
> For CMA memory allocation, ownership is assigned to DSP to make it
> accessible by the PD running on the DSP. With current implementation
> HLOS VM is stored in the channel structure during rpmsg_probe and
> this VM is passed to qcom_scm call as the source VM.
> 
> The qcom_scm call will overwrite the passed source VM with the next
> VM which would cause a problem in case the scm call is again needed.
> Adding a local copy of source VM whereever scm call is made to avoid
> this problem.
> 

The perms in fastrpc_channel_ctx should always reflect the current
permission bits, so I'm surprised you see problem.

What is the scenario where that's not the case?

> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 1c6c62a7f7f5..c13efa7727e0 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -263,7 +263,6 @@ struct fastrpc_channel_ctx {
>  	int domain_id;
>  	int sesscount;
>  	int vmcount;
> -	u64 perms;
>  	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>  	struct rpmsg_device *rpdev;
>  	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> @@ -1279,9 +1278,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  
>  		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
>  		if (fl->cctx->vmcount) {
> +			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
>  			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>  							(u64)fl->cctx->remote_heap->size,
> -							&fl->cctx->perms,
> +							&src_perms,
>  							fl->cctx->vmperms, fl->cctx->vmcount);
>  			if (err) {
>  				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
> @@ -1915,8 +1916,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  
>  	/* Add memory to static PD pool, protection thru hypervisor */
>  	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
> +		u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
>  		err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> -			&fl->cctx->perms, fl->cctx->vmperms, fl->cctx->vmcount);
> +			&src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>  		if (err) {
>  			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>  					buf->phys, buf->size, err);
> @@ -2290,7 +2293,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  
>  	if (vmcount) {
>  		data->vmcount = vmcount;
> -		data->perms = BIT(QCOM_SCM_VMID_HLOS);
>  		for (i = 0; i < data->vmcount; i++) {
>  			data->vmperms[i].vmid = vmids[i];
>  			data->vmperms[i].perm = QCOM_SCM_PERM_RWX;
Ekansh Gupta Jan. 9, 2024, 5:38 a.m. UTC | #2 
On 1/9/2024 6:42 AM, Elliot Berman wrote:
>
> On 1/8/2024 2:05 AM, Ekansh Gupta wrote:
>> For CMA memory allocation, ownership is assigned to DSP to make it
>> accessible by the PD running on the DSP. With current implementation
>> HLOS VM is stored in the channel structure during rpmsg_probe and
>> this VM is passed to qcom_scm call as the source VM.
>>
>> The qcom_scm call will overwrite the passed source VM with the next
>> VM which would cause a problem in case the scm call is again needed.
>> Adding a local copy of source VM whereever scm call is made to avoid
>> this problem.
>>
> The perms in fastrpc_channel_ctx should always reflect the current
> permission bits, so I'm surprised you see problem.
>
> What is the scenario where that's not the case?

Thanks for reviewing the changes, Elliot. FastRPC driver is storing
the bitfield of HLOS VMID in fastrpc_channel_ctx in perms(cctx->perms)
and remoteproc specific VMID information from device tree in vmperms(cctx->vmperms).
This information is intended to be passed to qcom_scm call when there is
a requirement to move the ownership of memory to any remoteproc VM. As
the srcvm is overwritten with the new VM, cctx->perms cannot be reused if
the same request comes for any other memory allocation.

The problem is seen with audioPD daemon. When the daemon is stated, it
allocates some memory for audioPD and moves the ownership from HLOS to
ADSP VM using qcom_scm call. After this, audioPD makes a request for some
more memory which is again allocated in kernel and as per current
implementation, qcom_scm call is again made with cctx->perms as srcVm
which is no longer storing HLOS vmid. Hence using a local variable to
make qcom_scm call where there is a need to move ownership from HLOS
to remoteproc VM.

Please let me know if you have any more queries.

--ekansh

>
>> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   drivers/misc/fastrpc.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 1c6c62a7f7f5..c13efa7727e0 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -263,7 +263,6 @@ struct fastrpc_channel_ctx {
>>   	int domain_id;
>>   	int sesscount;
>>   	int vmcount;
>> -	u64 perms;
>>   	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>>   	struct rpmsg_device *rpdev;
>>   	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
>> @@ -1279,9 +1278,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>   
>>   		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
>>   		if (fl->cctx->vmcount) {
>> +			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>> +
>>   			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>>   							(u64)fl->cctx->remote_heap->size,
>> -							&fl->cctx->perms,
>> +							&src_perms,
>>   							fl->cctx->vmperms, fl->cctx->vmcount);
>>   			if (err) {
>>   				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
>> @@ -1915,8 +1916,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>   
>>   	/* Add memory to static PD pool, protection thru hypervisor */
>>   	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
>> +		u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>> +
>>   		err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>> -			&fl->cctx->perms, fl->cctx->vmperms, fl->cctx->vmcount);
>> +			&src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>>   		if (err) {
>>   			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>>   					buf->phys, buf->size, err);
>> @@ -2290,7 +2293,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   
>>   	if (vmcount) {
>>   		data->vmcount = vmcount;
>> -		data->perms = BIT(QCOM_SCM_VMID_HLOS);
>>   		for (i = 0; i < data->vmcount; i++) {
>>   			data->vmperms[i].vmid = vmids[i];
>>   			data->vmperms[i].perm = QCOM_SCM_PERM_RWX;
Elliot Berman Jan. 9, 2024, 5:55 p.m. UTC | #3 
On 1/8/2024 9:38 PM, Ekansh Gupta wrote:
> 
> On 1/9/2024 6:42 AM, Elliot Berman wrote:
>>
>> On 1/8/2024 2:05 AM, Ekansh Gupta wrote:
>>> For CMA memory allocation, ownership is assigned to DSP to make it
>>> accessible by the PD running on the DSP. With current implementation
>>> HLOS VM is stored in the channel structure during rpmsg_probe and
>>> this VM is passed to qcom_scm call as the source VM.
>>>
>>> The qcom_scm call will overwrite the passed source VM with the next
>>> VM which would cause a problem in case the scm call is again needed.
>>> Adding a local copy of source VM whereever scm call is made to avoid
>>> this problem.
>>>
>> The perms in fastrpc_channel_ctx should always reflect the current
>> permission bits, so I'm surprised you see problem.
>>
>> What is the scenario where that's not the case?
> 
> Thanks for reviewing the changes, Elliot. FastRPC driver is storing
> the bitfield of HLOS VMID in fastrpc_channel_ctx in perms(cctx->perms)
> and remoteproc specific VMID information from device tree in vmperms(cctx->vmperms).
> This information is intended to be passed to qcom_scm call when there is
> a requirement to move the ownership of memory to any remoteproc VM. As
> the srcvm is overwritten with the new VM, cctx->perms cannot be reused if
> the same request comes for any other memory allocation.
> 
> The problem is seen with audioPD daemon. When the daemon is stated, it
> allocates some memory for audioPD and moves the ownership from HLOS to
> ADSP VM using qcom_scm call. After this, audioPD makes a request for some
> more memory which is again allocated in kernel and as per current
> implementation, qcom_scm call is again made with cctx->perms as srcVm
> which is no longer storing HLOS vmid. Hence using a local variable to
> make qcom_scm call where there is a need to move ownership from HLOS
> to remoteproc VM.
> 
> Please let me know if you have any more queries.
> 

Ah, got it. There can be multiple allocations/assignments per fastrpc_channel_ctx.

In that case:

Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>

> --ekansh
> 
>>
>>> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")> Cc: stable <stable@kernel.org>
>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>> ---
>>>   drivers/misc/fastrpc.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 1c6c62a7f7f5..c13efa7727e0 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -263,7 +263,6 @@ struct fastrpc_channel_ctx {
>>>       int domain_id;
>>>       int sesscount;
>>>       int vmcount;
>>> -    u64 perms;
>>>       struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>>>       struct rpmsg_device *rpdev;
>>>       struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
>>> @@ -1279,9 +1278,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>>             /* Map if we have any heap VMIDs associated with this ADSP Static Process. */
>>>           if (fl->cctx->vmcount) {
>>> +            u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>> +
>>>               err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>>>                               (u64)fl->cctx->remote_heap->size,
>>> -                            &fl->cctx->perms,
>>> +                            &src_perms,
>>>                               fl->cctx->vmperms, fl->cctx->vmcount);
>>>               if (err) {
>>>                   dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
>>> @@ -1915,8 +1916,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>>         /* Add memory to static PD pool, protection thru hypervisor */
>>>       if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
>>> +        u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>>> +
>>>           err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
>>> -            &fl->cctx->perms, fl->cctx->vmperms, fl->cctx->vmcount);
>>> +            &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>>>           if (err) {
>>>               dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>>>                       buf->phys, buf->size, err);
>>> @@ -2290,7 +2293,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>         if (vmcount) {
>>>           data->vmcount = vmcount;
>>> -        data->perms = BIT(QCOM_SCM_VMID_HLOS);
>>>           for (i = 0; i < data->vmcount; i++) {
>>>               data->vmperms[i].vmid = vmids[i];
>>>               data->vmperms[i].perm = QCOM_SCM_PERM_RWX;
Srinivas Kandagatla Feb. 21, 2024, 7:08 a.m. UTC | #4 
On Mon, 08 Jan 2024 15:35:13 +0530, Ekansh Gupta wrote:
> For CMA memory allocation, ownership is assigned to DSP to make it
> accessible by the PD running on the DSP. With current implementation
> HLOS VM is stored in the channel structure during rpmsg_probe and
> this VM is passed to qcom_scm call as the source VM.
> 
> The qcom_scm call will overwrite the passed source VM with the next
> VM which would cause a problem in case the scm call is again needed.
> Adding a local copy of source VM whereever scm call is made to avoid
> this problem.
> 
> [...]

Applied, thanks!

[1/1] misc: fastrpc: Pass proper arguments to scm call
      commit: 5169a077f22a23d27660d048d5f43a592fd2c62f

Best regards,
diff mbox series

Patch

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 1c6c62a7f7f5..c13efa7727e0 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -263,7 +263,6 @@  struct fastrpc_channel_ctx {
 	int domain_id;
 	int sesscount;
 	int vmcount;
-	u64 perms;
 	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
 	struct rpmsg_device *rpdev;
 	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
@@ -1279,9 +1278,11 @@  static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
 		if (fl->cctx->vmcount) {
+			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
 			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
 							(u64)fl->cctx->remote_heap->size,
-							&fl->cctx->perms,
+							&src_perms,
 							fl->cctx->vmperms, fl->cctx->vmcount);
 			if (err) {
 				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
@@ -1915,8 +1916,10 @@  static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 
 	/* Add memory to static PD pool, protection thru hypervisor */
 	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
+		u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
 		err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
-			&fl->cctx->perms, fl->cctx->vmperms, fl->cctx->vmcount);
+			&src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
 		if (err) {
 			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
 					buf->phys, buf->size, err);
@@ -2290,7 +2293,6 @@  static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 
 	if (vmcount) {
 		data->vmcount = vmcount;
-		data->perms = BIT(QCOM_SCM_VMID_HLOS);
 		for (i = 0; i < data->vmcount; i++) {
 			data->vmperms[i].vmid = vmids[i];
 			data->vmperms[i].perm = QCOM_SCM_PERM_RWX;