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@xxxxxxxxxxx>
> --ekansh
>
>>
>>> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")> Cc: stable <stable@xxxxxxxxxx>
>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
>>> ---
>>> 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;
