Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 02/04/2025 11:38, Ekansh Gupta wrote:
> 
> 
> On 3/21/2025 5:53 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 20/03/2025 18:43, Dmitry Baryshkov wrote:
>>> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>> GPDSP remoteprocs.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>>> ---
>>>>>     drivers/misc/fastrpc.c | 10 ++++++++--
>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>> --- a/drivers/misc/fastrpc.c
>>>>> +++ b/drivers/misc/fastrpc.c
>>>>> @@ -28,7 +28,9 @@
>>>>>     #define SDSP_DOMAIN_ID (2)
>>>>>     #define CDSP_DOMAIN_ID (3)
>>>>>     #define CDSP1_DOMAIN_ID (4)
>>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>> +#define GDSP0_DOMAIN_ID (5)
>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>
>>>> We have already made the driver look silly here, Lets not add domain ids for
>>>> each instance, which is not a scalable.
>>>>
>>>> Domain ids are strictly for a domain not each instance.
>>>
>>> Then CDSP1 should also be gone, correct?
>> Its already gone as part of the patch that I shared in this discussion.
>>
>> I will send a proper patch to list once Ling/Ekansh has agree with it.
>>
> Thanks, Srini, for sharing this clean-up patch. It looks proper to
> me, but I was thinking if we could remove the domain_id dependency
> from the fastrpc driver. The addition of any new DSP will frequently
> require changes in the driver. Currently, its usage is for creating
> different types of device nodes and transferring memory ownership to
> SLPI when a memory region is added.
> 
> The actual intention behind different types of device nodes can be
> defined as follows:
> 
> fastrpc-xdsp-secure: Used for signed (privileged) PD offload and for daemons.
> fastrpc-xdsp: Should be used only for unsigned (less privileged) PD offload.
> 
> The reason for this constraint is to prevent any untrusted process
> from communicating with any privileged PD on DSP, which poses a security risk.
> The access to different device nodes can be provided/restricted based on UID/GID
> (still need to check more on this; on Android-like systems, this is controlled by
> SELinux).
> 
> There is already a qcom,non-secure-domain device tree property[1] which doesn't
> have a proper definition as of today. The actual way to differentiate between
> secure and non-secure DSP should be based on its ability to support unsigned PD.
> 
> One way to remove the domain_id dependency that I can think of is to use this
> property to create different types of device nodes. Essentially, if unsigned PD
> is supported (e.g., CDSP, GPDSP), we add this property to the DT node and create
> both types of device nodes based on this. Otherwise, only the secure device node
> is created.

This sounds like breaking backwards compatibility on the userspace side. 
You can not do that.

> 
> This raises the question of backward compatibility, but I see that on most older
> platform DTs, this property is already added, so both device nodes will be created
> there, and applications will work as expected. If any old DT DSP node lacks this
> property, we can add it there as well.
> 
> Going forward, the qcom-non-secure-property should be added only if unsigned PD
> is supported. This way, we can clean up the driver completely to remove the
> domain_id dependency.
> 
> If this sounds good, I can work on this design and send out a patch.
> 
> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml#n44
> 
> --Ekansh
> 
>> --srini
>>>
>>
> 


-- 
With best wishes
Dmitry