| Message ID | 20240722055437.3467900-2-quic_ekangupt@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix user PD inimem requirements | expand |
On Mon, Jul 22, 2024 at 11:24:36AM +0530, 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@kernel.org> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > 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) Meta-comment, for a future change, why not tabs to line things up? > #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), You are aligning it this way, but still checking the file size for INIT_FILELEN_MAX, so that's not really going to change anything here. How was this tested? thanks, greg k-h
On 7/22/2024 11:28 AM, Greg KH wrote: > On Mon, Jul 22, 2024 at 11:24:36AM +0530, 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@kernel.org> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> 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) > Meta-comment, for a future change, why not tabs to line things up? Sure, I'll add a comment. Should I line up all the MACRO definitions? If yes, should I send it as a separate patch? > >> #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), > You are aligning it this way, but still checking the file size for > INIT_FILELEN_MAX, so that's not really going to change anything here. File size is for the fastrpc shell that is to be loaded. This is the size of buffer allocated by user and passed to the driver. The driver will map this buffer to SMMU before passing it to DSP. Therefore the bound check on this size is necessary. As for initmem size for remote process on DSP, this size is also inclusive of initial requirements of DSP size user process(user PD), hence separating it from file size macro. > How was this tested? This is tested with fastrpc use cases available in hexagon SDK: https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps --Ekansh > thanks, > > greg k-h
On Mon, Jul 22, 2024 at 11:42:52AM +0530, Ekansh Gupta wrote: > > > On 7/22/2024 11:28 AM, Greg KH wrote: > > On Mon, Jul 22, 2024 at 11:24:36AM +0530, 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@kernel.org> > >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > >> --- > >> 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) > > Meta-comment, for a future change, why not tabs to line things up? > Sure, I'll add a comment. I didn't say anything about comments :( > Should I line up all the MACRO definitions? If yes, should I send it as a separate patch? As I said, yes, for a future change. > > How was this tested? > This is tested with fastrpc use cases available in hexagon SDK: > https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps Do you have regression tests that attempt to check the boundry conditions and alignment here? thanks, greg k-h
On 7/22/2024 1:09 PM, Greg KH wrote: > On Mon, Jul 22, 2024 at 11:42:52AM +0530, Ekansh Gupta wrote: >> >> On 7/22/2024 11:28 AM, Greg KH wrote: >>> On Mon, Jul 22, 2024 at 11:24:36AM +0530, 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@kernel.org> >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >>>> --- >>>> 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) >>> Meta-comment, for a future change, why not tabs to line things up? >> Sure, I'll add a comment. > I didn't say anything about comments :( Oops, sorry. > >> Should I line up all the MACRO definitions? If yes, should I send it as a separate patch? > As I said, yes, for a future change. Noted, thanks. >>> How was this tested? >> This is tested with fastrpc use cases available in hexagon SDK: >> https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps > Do you have regression tests that attempt to check the boundry > conditions and alignment here? For most of the test cases, I used the fastrpc lib: https://github.com/quic/fastrpc This library is taking care of passing proper shell size which is within the boundary for all the platform that I've tried. I'll try creating and running some regression tests for this change. --Ekansh > > thanks, > > greg k-h
On Mon, Jul 22, 2024 at 01:23:56PM GMT, Ekansh Gupta wrote: > > > On 7/22/2024 1:09 PM, Greg KH wrote: > > On Mon, Jul 22, 2024 at 11:42:52AM +0530, Ekansh Gupta wrote: > >> > >> On 7/22/2024 11:28 AM, Greg KH wrote: > >>> On Mon, Jul 22, 2024 at 11:24:36AM +0530, 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@kernel.org> > >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > >>>> --- > >>>> 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) > >>> Meta-comment, for a future change, why not tabs to line things up? > >> Sure, I'll add a comment. > > I didn't say anything about comments :( > Oops, sorry. > > > >> Should I line up all the MACRO definitions? If yes, should I send it as a separate patch? > > As I said, yes, for a future change. > Noted, thanks. > >>> How was this tested? > >> This is tested with fastrpc use cases available in hexagon SDK: > >> https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps > > Do you have regression tests that attempt to check the boundry > > conditions and alignment here? > For most of the test cases, I used the fastrpc lib: > https://github.com/quic/fastrpc > > This library is taking care of passing proper shell size which is within the boundary for > all the platform that I've tried. > I'll try creating and running some regression tests for this change. Existing userspace implementation provides an example of good behaviour. Please consider implenting and publishing 'bad' behaviour testcases which make sure that the driver doesn't break if it gets passed 'bad' data.
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) #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), 1024 * 1024); err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen, &imem);
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@kernel.org> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> --- drivers/misc/fastrpc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)