On 11/14/2024 5:30 PM, Dmitry Baryshkov wrote: > On Thu, Nov 14, 2024 at 10:49:52AM +0530, Ekansh Gupta wrote: >> >> On 11/13/2024 5:20 PM, Dmitry Baryshkov wrote: >>> On Wed, 13 Nov 2024 at 08:18, Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx> wrote: >>>> >>>> On 11/13/2024 11:13 AM, Dmitry Baryshkov wrote: >>>>> On Wed, Nov 13, 2024 at 10:30:42AM +0530, Ekansh Gupta wrote: >>>>>> FastRPC framework treats ADSP as a secure domain on sc7280 SoC >>>>>> which means that only secure fastrpc device node should be >>>>>> created for ADSP remoteproc. Remove the non-secure-domain >>>>>> property from ADSP fastrpc node. >>>>> If this prevents the non-secure devices from being created, isn't that a >>>>> regression from the userspace point of view? >>>> The actual intention of having secure and non-secure domains is to utilize signed(high privilege) >>>> and unsigned(low privilege) DSP processes properly. >>>> >>>> Non-secure device node is intended to be used by untrusted/generic applications which needs to >>>> offload tasks to DSP as unsignedPD. Only unsigned PD is expected to be allowed if the process is >>>> using non-secure node. >>>> >>>> Secure device is intended to be used by trusted processes like daemons or any application >>>> which needs to offload as signed PD to DSP. >>>> >>>> The ideal expectation from userspace is to first try to open secure device node and fall back to >>>> non-secure node if the secure node is not accessible or absent. >>>> >>>> I understand your concerns, can you please suggest how this can be improved/corrected? >>> Thank you for the explanation, and thanks for the description of the >>> expected behaviour, but the question is different. >>> Currently (with the property being present in DT) the driver creates a >>> non-secure fastrpc device for the ADSP. >>> Can it actually be used? Note: no mentioning of a particular userspace >>> implementation or the (un)expected usage. >>> If it could not and an attempt to use it resulted in some kind of an >>> error, then the patch is a fix and it should be decribed accordingly. >>> If it could be used and now you are removing this possibility, then it >>> is a regression. Again, this must be clearly documented, but generally >>> this is not allowed. >> Thanks for the clarification, Dmitry. >> >> As of today, if the property is present in DT, non-secure fastrpc device will be created >> for ADSP and as there are no checks to restrict daemons to use only secure node, there >> will not be any failures observed. So there is no error if non-secure property is added >> for ADSP and your 2nd point holds here. >> >> Problems with the current design are(you can look into below points independent of the change): >> >> 1. This creates a security concern as any process that can open non-secure device >> can replicate daemon to attach to DSP root PD and cause troubles there which is not >> a good thing. So basically any trusted process(maybe same group) should only use secure >> device node and any process using non-secure node should only offload to unsigned PD. > Again, you are describing expected behaviour. Other userspace clients > can deviate from this. Okay, understood. > >> 2. Having this property well defined also help in scaling fastrpc driver for new domains(like CDSP1 >> was recently introduced) as driver can only rely on the "label" and "non-secure-domain" property >> for device creation. Say, only secure device is create if property is not defined and both device nodes >> are created if non-secure-domain is define. This way, the dependency on domain_id can be removed >> from fastrpc_rpmsg_probe[1] and create either only fastrpc-xdsp-secure or both(secure and non-secure). > Well, I don't think I follow this point. The property is already > well-defined. By well-defined I meant there isn't a proper documentation of what is meant by non-secure-domain. >> This however is a regression as you have mentioned, but it it helps address multiple problems. >> >> Should I discuss further on documentation or is any more design clarification should be done here? > At least you must explicitly specify that this causes changes to > userspace, and all the reasons to do that. So that everybody else > doesn't have to read between the lines. Ack. > >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n2327 >> >> --ekansh >>>> --ekansh >>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx> >>>>>> --- >>>>>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 1 - >>>>>> 1 file changed, 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi >>>>>> index 3d8410683402..c633926c0f33 100644 >>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >>>>>> @@ -3852,7 +3852,6 @@ fastrpc { >>>>>> compatible = "qcom,fastrpc"; >>>>>> qcom,glink-channels = "fastrpcglink-apps-dsp"; >>>>>> label = "adsp"; >>>>>> - qcom,non-secure-domain; > - Are there other platforms which have this flag set for ADSP? Yes, there are a few platforms where this property is added for ADSP. > - Granted that sc7280 was targeting ChromeOS devices, might it be that > there is a CrOS-specific userspace for that? FastRPC nodes were recently added to this devicetree recently. Looks like this property is just getting copied. It might be that fastrpc was recently tried on ChromeOS device or it might be added to support some other devices that uses fastrpc(qcm6490-idp etc.). --ekansh >>>>>> #address-cells = <1>; >>>>>> #size-cells = <0>; >>>>>> >>>>>> -- >>>>>> 2.34.1 >>>>>>
