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

[Srinivas Kandagatla]

On 03/04/2025 05:44, Ekansh Gupta wrote:
> 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@xxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Ling Xu<quic_lxu5@xxxxxxxxxxx>
> > > > > > > ---
> > > > > > >      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.
In the past I have suggested you to update the dt-bindings so that any 
new platforms that get added will not use the qcom,non-secure-domain 
flag. The usage of this flag is still confusing for any new users, as 
per the dt bindings its open to be used.

As we can not break the backwards compatibility, why not just restrict 
that to those platforms for now, and enforce new platforms to use not 
use it for for domains like adsp.

--srini