Re: [PATCH v6 1/2] misc: fastrpc: Define a new initmem size for user PD

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 07:35, Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx> wrote:
>
>
>
> On 7/22/2024 2:02 PM, Dmitry Baryshkov wrote:
> > On Mon, Jul 22, 2024 at 01:31:59PM GMT, Ekansh Gupta wrote:
> >> For user PD initialization, initmem is allocated and sent to DSP for
> >> initial memory requirements like shell loading. The size of this memory
> >> is decided based on the shell size that is passed by the user space.
> >> With the current implementation, a minimum of 2MB is always allocated
> >> for initmem even if the size passed by user is less than that. For this
> >> a MACRO is being used which is intended for shell size bound check.
> >> This minimum size of 2MB is not recommended as the PD will have very
> >> less memory for heap and will have to request HLOS again for memory.
> >> Define a new macro for initmem minimum length of 3MB.
> >>
> >> Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
> >> Cc: stable <stable@xxxxxxxxxx>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
> >> ---
> >>  drivers/misc/fastrpc.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index a7a2bcedb37e..a3a5b745936e 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -39,6 +39,7 @@
> >>  #define FASTRPC_DSP_UTILITIES_HANDLE        2
> >>  #define FASTRPC_CTXID_MASK (0xFF0)
> >>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> >> +#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
> > So, what is the difference between INIT_FILELEN_MAX and
> > FASTRPC_INITLEN_MIN?
>
> INIT_FILELEN_MAX is the maximum shell size that can be passed by user.
> FASTRPC_INITLEN_MIN is the minimum initmem length for PD.

At least this should be commented on in the source.

>
> >
> >>  #define INIT_FILE_NAMELEN_MAX (128)
> >>  #define FASTRPC_DEVICE_NAME "fastrpc"
> >>
> >> @@ -1410,7 +1411,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> >>                      goto err;
> >>      }
> >>
> >> -    memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
> >> +    memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
> > BTW: why is the code multiplying filelen by 4? Nothing in the source
> > code suggests that filelen is in u32 words, so I'd assume it's measured
> > in bytes.
>
> The passed filelen is actually the size of fastrpc shell. This size is not sufficient for the user
> PD initialization. The 4x of filelen gives the approx. needed memory for signed PD initialization.
> Yes, filelen is measured in bytes.

Ugh. Shouldn't it be sum(elf.ph[i].memsz) + some margin? I know this
is a separate topic, but since you were touching these lines it has
come to my attention.

>
> >
> >>                     1024 * 1024);
> >>      err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
> >>                              &imem);
> >> --
> >> 2.34.1
> >>
>


-- 
With best wishes
Dmitry