Message-ID: <46aa3914-7ac9-4605-911f-5120e8a33720@quicinc.com>
Date: Tue, 2 Jul 2024 14:10:26 +0530
From: Ekansh Gupta <quic_ekangupt@...cinc.com>
To: Bjorn Andersson <quic_bjorande@...cinc.com>
CC: <srinivas.kandagatla@...aro.org>, <linux-arm-msm@...r.kernel.org>,
        <gregkh@...uxfoundation.org>, <quic_bkumar@...cinc.com>,
        <linux-kernel@...r.kernel.org>, <quic_chennak@...cinc.com>,
        <dri-devel@...ts.freedesktop.org>, <arnd@...db.de>,
        stable
	<stable@...nel.org>
Subject: Re: [PATCH v3] misc: fastrpc: Increase max user PD initmem size



On 7/2/2024 2:44 AM, Bjorn Andersson wrote:
> On Mon, Jul 01, 2024 at 05:22:37PM +0530, Ekansh Gupta wrote:
>> For user PD initialization, initmem is allocated and sent to DSP for
>> initial memory requirements like shell loading. This size is passed
>> by user space and is checked against a max size.
> Why does fastrpc_init_create_process() allocate 4x the passed value and
> why is the value rounded up to INIT_FILELEN_MAX?
The passed value is actually the size of fastrpc shell. This size is not sufficient for the user
PD initialization and the PD also needs some additional memory for it's other requirements
like heap. The value is aligned to 1 MB and there is a possibility that the user passed value
is zero(user can only pass the size if it can open the shell). For this, there is at least some
memory allocated and sent to DSP for PD initialization(2MB as of today).
>> For unsigned PD
>> offloading requirement, more than 2MB size could be passed by user
>> which would result in PD initialization failure. Increase the maximum
>> size that can be passed py user for user PD initmem allocation.
> Sounds good, but why not 2.1MB or a rounder arbitrary value of 8 or 16?
>
> What is actually expected to fit in this initial memory? Is it the shell
> that has grown beyond 2MB?
I checked this again with the size of shell and I see that the shell size is not going beyond
2 MB, it's the unsigned PD requirement which is asking for more memory. This is because
there are some static heap initialization specifically for unsigned PD. Do you suggest having
a different definition for minimum initmem? Or have it as a local variable which changes
based on PD type(2MB for signed PD and 5MB for unsigned PD)?
>
> Also, s/py/by
>
>> Any
>> additional memory sent to DSP during PD init is used as the PD heap.
>>
> Does this mean that the initmem is used for shell loading and initial
> heap, and if more heap is needed after that the DSP can request more
> memory? Related to the question in v2, how is this memory accounted for?
Yes this is for the initial heap requirement of PD(described above also). In case any more
memory is required by DSP PD, it will make a reverse call to borrow more memory from
HLOS using ADD_PAGES request which is supported by fastrpc_req_mmap. However,
for unsigned PD the heaps are statically initialized which brings the requirement of some
additional memory.
>
> What would it mean that init.filefd != 0 in
> fastrpc_init_create_process(), will that pre-allocated memory (which was
> allocated without any size checks afaict) then be used for the same
> purpose? Why is a buffer of 4x the size of initfd then also allocated
> and mapped to the DSP?
The init.filefd is the FD of fastrpc shell that is opened and read by the process user space.
I believe the pre-allocated memory you mentioned is the memory pointed by init.file. If
the shell file is opened by user process DSP will load the shell and add the initmem to the
DSP PD pool. If the user space has not opened and read the shell, DSP root PD daemon
will open and read the shell for loading for PD spawning. Please let me know if there are
any more questions here. Basically the usage of initmem is for shell loading and remaining
memory is added to PD pool of heap and other usage.
>
>
> Could you please send a patch adding some comments documenting these
> things, the steps taken to create a new process, and what the 6
> arguments built in fastrpc_init_create_process() actually means?
Sure, I'll add this information in the next spin.
>
> Perhaps I'm just failing to read the answers already in the
> implementation, if so I'm sorry.
Thanks for reviewing the patch. I'll add most of the information in commit text and as
comments in the next patch.

--Ekansh
>
> Regards,
> Bjorn
>
>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>> Cc: stable <stable@...nel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
>> ---
>> Changes in v2:
>>   - Modified commit text.
>>   - Removed size check instead of updating max file size.
>> Changes in v3:
>>   - Added bound check again with a higher max size definition.
>>   - Modified commit text accordingly.
>>
>>  drivers/misc/fastrpc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 5204fda51da3..11a230af0b10 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -38,7 +38,7 @@
>>  #define FASTRPC_INIT_HANDLE	1
>>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
>>  #define FASTRPC_CTXID_MASK (0xFF0)
>> -#define INIT_FILELEN_MAX (2 * 1024 * 1024)
>> +#define INIT_FILELEN_MAX (5 * 1024 * 1024)
>>  #define INIT_FILE_NAMELEN_MAX (128)
>>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>>  
>> -- 
>> 2.34.1
>>
>>
>>

Powered by blists - more mailing lists