[PATCH v6 0/2] Fix user PD inimem requirements

[PATCH v6 0/2] Fix user PD inimem requirements

[Ekansh Gupta]

This patch series fixes the incorrect initmem size assumptions for
signed and unsigned user PD.
Previous single patch[v4]: https://lore.kernel.org/all/20240719085708.1764952-1-quic_ekangupt@quicinc.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.
Changes in v4:
  - Defined new initmem specific MACROs.
  - Adding extra memory for unsigned PD.
  - Added comment suggesting the reason for this change.
  - Modified commit text.
Changes in v5:
  - Splitted the change into separate patches.
Changes in v6:
  - Changed Unsigned extra length macro name.
  - Add comment in proper format.

Ekansh Gupta (2):
  misc: fastrpc: Define a new initmem size for user PD
  misc: fastrpc: Increase unsigned PD initmem size

 drivers/misc/fastrpc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
2.34.1

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

[Ekansh Gupta]

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)
 #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);
-- 
2.34.1

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

[Dmitry Baryshkov]

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@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)

So, what is the difference between INIT_FILELEN_MAX and
FASTRPC_INITLEN_MIN?

>  #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.

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

-- 
With best wishes
Dmitry

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

[Ekansh Gupta]

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@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)
> 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.

>
>>  #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.

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

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@quicinc.com> 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@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)
> > 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

[PATCH v6 2/2] misc: fastrpc: Increase unsigned PD initmem size

[Ekansh Gupta]

For unsigned PD offloading requirement, additional memory is required
because of additional static heap initialization. Without this
additional memory, PD initialization would fail. Increase the initmem
size by 2MB for unsigned PD initmem buffer allocation. Any additional
memory sent to DSP during PD init is used as the PD heap.

Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index a3a5b745936e..9daeb039c2a2 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -40,6 +40,7 @@
 #define FASTRPC_CTXID_MASK (0xFF0)
 #define INIT_FILELEN_MAX (2 * 1024 * 1024)
 #define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
+#define FASTRPC_UNSIGNED_EXTRA_LEN (2 * 1024 * 1024)
 #define INIT_FILE_NAMELEN_MAX (128)
 #define FASTRPC_DEVICE_NAME	"fastrpc"
 
@@ -1411,8 +1412,15 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 			goto err;
 	}
 
+	/*
+	 * Allocate buffer in kernel for donating to remote process.
+	 * Unsigned PD requires additional memory because of the
+	 * additional static heap initialized within the process.
+	 */
 	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
 		       1024 * 1024);
+	if (unsigned_module)
+		memlen += FASTRPC_UNSIGNED_EXTRA_LEN;
 	err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
 				&imem);
 	if (err)
-- 
2.34.1

Re: [PATCH v6 2/2] misc: fastrpc: Increase unsigned PD initmem size

[Dmitry Baryshkov]

On Mon, Jul 22, 2024 at 01:32:00PM GMT, Ekansh Gupta wrote:
> For unsigned PD offloading requirement, additional memory is required
> because of additional static heap initialization. Without this
> additional memory, PD initialization would fail. Increase the initmem
> size by 2MB for unsigned PD initmem buffer allocation. Any additional
> memory sent to DSP during PD init is used as the PD heap.

From the previous commit message I had the feeling that DSP can request
more memory if required. Currently you are stating that PD init would
fail if it doesn't have enough mem.

> 
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v6 2/2] misc: fastrpc: Increase unsigned PD initmem size

[Ekansh Gupta]

On 7/22/2024 2:04 PM, Dmitry Baryshkov wrote:
> On Mon, Jul 22, 2024 at 01:32:00PM GMT, Ekansh Gupta wrote:
>> For unsigned PD offloading requirement, additional memory is required
>> because of additional static heap initialization. Without this
>> additional memory, PD initialization would fail. Increase the initmem
>> size by 2MB for unsigned PD initmem buffer allocation. Any additional
>> memory sent to DSP during PD init is used as the PD heap.
> From the previous commit message I had the feeling that DSP can request
> more memory if required. Currently you are stating that PD init would
> fail if it doesn't have enough mem.

DSP PD can request for memory only for PD heap and that too once the PD
initialization is complete.

>
>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>

Re: [PATCH v6 2/2] misc: fastrpc: Increase unsigned PD initmem size

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 07:36, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>
>
>
> On 7/22/2024 2:04 PM, Dmitry Baryshkov wrote:
> > On Mon, Jul 22, 2024 at 01:32:00PM GMT, Ekansh Gupta wrote:
> >> For unsigned PD offloading requirement, additional memory is required
> >> because of additional static heap initialization. Without this
> >> additional memory, PD initialization would fail. Increase the initmem
> >> size by 2MB for unsigned PD initmem buffer allocation. Any additional
> >> memory sent to DSP during PD init is used as the PD heap.
> > From the previous commit message I had the feeling that DSP can request
> > more memory if required. Currently you are stating that PD init would
> > fail if it doesn't have enough mem.
>
> DSP PD can request for memory only for PD heap and that too once the PD
> initialization is complete.

This should be a part of the (previous?) commit message.  Also what is
the difference between static heap vs PD heap?

>
> >
> >> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> >> Cc: stable <stable@kernel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
>


-- 
With best wishes
Dmitry

Re: [PATCH v6 0/2] Fix user PD inimem requirements

[Dmitry Baryshkov]

On Mon, Jul 22, 2024 at 01:31:58PM GMT, Ekansh Gupta wrote:
> This patch series fixes the incorrect initmem size assumptions for
> signed and unsigned user PD.
> Previous single patch[v4]: https://lore.kernel.org/all/20240719085708.1764952-1-quic_ekangupt@quicinc.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.
> Changes in v4:
>   - Defined new initmem specific MACROs.
>   - Adding extra memory for unsigned PD.
>   - Added comment suggesting the reason for this change.
>   - Modified commit text.
> Changes in v5:
>   - Splitted the change into separate patches.
> Changes in v6:
>   - Changed Unsigned extra length macro name.
>   - Add comment in proper format.

Please allow reviewers to finish with the previous iteration first. The
usual recommendation is to send new iteration once per 24h timeframe.

> 
> Ekansh Gupta (2):
>   misc: fastrpc: Define a new initmem size for user PD
>   misc: fastrpc: Increase unsigned PD initmem size
> 
>  drivers/misc/fastrpc.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v6 0/2] Fix user PD inimem requirements

[Ekansh Gupta]

On 7/22/2024 1:56 PM, Dmitry Baryshkov wrote:
> On Mon, Jul 22, 2024 at 01:31:58PM GMT, Ekansh Gupta wrote:
>> This patch series fixes the incorrect initmem size assumptions for
>> signed and unsigned user PD.
>> Previous single patch[v4]: https://lore.kernel.org/all/20240719085708.1764952-1-quic_ekangupt@quicinc.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.
>> Changes in v4:
>>   - Defined new initmem specific MACROs.
>>   - Adding extra memory for unsigned PD.
>>   - Added comment suggesting the reason for this change.
>>   - Modified commit text.
>> Changes in v5:
>>   - Splitted the change into separate patches.
>> Changes in v6:
>>   - Changed Unsigned extra length macro name.
>>   - Add comment in proper format.
> Please allow reviewers to finish with the previous iteration first. The
> usual recommendation is to send new iteration once per 24h timeframe.
Noted.

Thanks,
Ekansh
>
>> Ekansh Gupta (2):
>>   misc: fastrpc: Define a new initmem size for user PD
>>   misc: fastrpc: Increase unsigned PD initmem size
>>
>>  drivers/misc/fastrpc.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.34.1
>>