Message-ID: <bfa29a76-f89a-4398-b6b3-1be7ae6cf8b3@oss.qualcomm.com>
Date: Thu, 3 Apr 2025 10:14:11 +0530
From: Ekansh Gupta <ekansh.gupta@....qualcomm.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: Ling Xu <quic_lxu5@...cinc.com>, andersson@...nel.org,
        konradybcio@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
        conor+dt@...nel.org, amahesh@....qualcomm.com, arnd@...db.de,
        gregkh@...uxfoundation.org, quic_kuiw@...cinc.com,
        quic_ekangupt@...cinc.com, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc



On 4/2/2025 2:12 PM, Dmitry Baryshkov wrote:
> 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@....qualcomm.com>
>>>>>> Signed-off-by: Ling Xu <quic_lxu5@...cinc.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.
Okay, I thought if the property is added for all older platforms, that will ensure backward
compatibility is maintained for old built applications.

>From userspace, the expected device open sequence is to try with the secure device node and
fallback to the default/non-secure node if the secure node is not available/accessible.
I understand the ABI cannot be broken, and this expectation should be added for new
applications/platforms.

This is still a security issue that needs to be fixed in some way. I'll try to find out if any other
approach can address this.

That being said, I'm fine with Srini's change for domain name clean-up.

I would request Ling to test the patch.

--Ekansh

>
>>
>> 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
>>>>
>>>
>>
>
>

Powered by blists - more mailing lists