Re: [Qemu-devel] [PATCH v3 04/30] imx_fec: Use ENET_FTRL to determine truncation length

[Peter Maydell]

On 22 November 2017 at 20:22, Andrey Smirnov  wrote:
> On Tue, Nov 21, 2017 at 9:31 AM, Peter Maydell  
> wrote:
>> On 6 November 2017 at 15:47, Andrey Smirnov  wrote:
>>> Frame truncation length, TRUNC_FL, is determined by the contents of
>>> ENET_FTRL register, so convert the code to use it instead of a
>>> hardcoded constant.
>>>
>>> To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
>>> increase the value of the latter to its theoretical maximum of 16K.

>>>
>>> +#define ENET_MAX_FRAME_SIZE(1 << ENET_RCR_MAX_FL_LENGTH)
>>
>> This means we now have functions with 16K local array
>> variables on the stack, which seems like a bad idea.
>>
>
> Can't say I see a big difference between having a 2K vs 16K buffer on
> the stack, but regardless, I am not quite clear if you are not too hot
> about this patch and want me to drop it (I am fine with it) or do you
> want me to modify it to have the emulation layer allocate said 16K
> buffer on the heap instead of a stack?

To be clearer: 16K is too large a buffer to have on the stack;
2K is about as big as you want to go.
If you want to allow 16K packets then you need to avoid having
on-stack arrays of that length. (But you don't want to do a
heap allocation for every function call either.)

"look for and fix functions with arrays of 16K or more" is one
of our bite-sized-tasks for people who want to fix bugs:
https://wiki.qemu.org/Contribute/BiteSizedTasks#Large_frames

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 04/30] imx_fec: Use ENET_FTRL to determine truncation length

[Andrey Smirnov]

On Tue, Nov 21, 2017 at 9:31 AM, Peter Maydell  wrote:
> On 6 November 2017 at 15:47, Andrey Smirnov  wrote:
>> Frame truncation length, TRUNC_FL, is determined by the contents of
>> ENET_FTRL register, so convert the code to use it instead of a
>> hardcoded constant.
>>
>> To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
>> increase the value of the latter to its theoretical maximum of 16K.
>>
>> Cc: Peter Maydell 
>> Cc: Jason Wang 
>> Cc: Philippe Mathieu-Daudé 
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-...@nongnu.org
>> Cc: yurov...@gmail.com
>> Signed-off-by: Andrey Smirnov 
>> ---
>>  hw/net/imx_fec.c | 4 ++--
>>  include/hw/net/imx_fec.h | 3 ++-
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index eb034ffd0c..dda0816fb3 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, 
>> const uint8_t *buf,
>>  crc_ptr = (uint8_t *) 
>>
>>  /* Huge frames are truncted.  */
>> -if (size > ENET_MAX_FRAME_SIZE) {
>> -size = ENET_MAX_FRAME_SIZE;
>> +if (size > s->regs[ENET_FTRL]) {
>> +size = s->regs[ENET_FTRL];
>>  flags |= ENET_BD_TR | ENET_BD_LG;
>>  }
>>
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 4bc8f03ec2..0fcc4f0c71 100644
>> --- a/include/hw/net/imx_fec.h
>> +++ b/include/hw/net/imx_fec.h
>> @@ -86,7 +86,6 @@
>>  #define ENET_TCCR3 393
>>  #define ENET_MAX   400
>>
>> -#define ENET_MAX_FRAME_SIZE2032
>>
>>  /* EIR and EIMR */
>>  #define ENET_INT_HB(1 << 31)
>> @@ -155,6 +154,8 @@
>>  #define ENET_RCR_NLC   (1 << 30)
>>  #define ENET_RCR_GRS   (1 << 31)
>>
>> +#define ENET_MAX_FRAME_SIZE(1 << ENET_RCR_MAX_FL_LENGTH)
>
> This means we now have functions with 16K local array
> variables on the stack, which seems like a bad idea.
>

Can't say I see a big difference between having a 2K vs 16K buffer on
the stack, but regardless, I am not quite clear if you are not too hot
about this patch and want me to drop it (I am fine with it) or do you
want me to modify it to have the emulation layer allocate said 16K
buffer on the heap instead of a stack?

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH v3 04/30] imx_fec: Use ENET_FTRL to determine truncation length

[Peter Maydell]

On 6 November 2017 at 15:47, Andrey Smirnov  wrote:
> Frame truncation length, TRUNC_FL, is determined by the contents of
> ENET_FTRL register, so convert the code to use it instead of a
> hardcoded constant.
>
> To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
> increase the value of the latter to its theoretical maximum of 16K.
>
> Cc: Peter Maydell 
> Cc: Jason Wang 
> Cc: Philippe Mathieu-Daudé 
> Cc: qemu-devel@nongnu.org
> Cc: qemu-...@nongnu.org
> Cc: yurov...@gmail.com
> Signed-off-by: Andrey Smirnov 
> ---
>  hw/net/imx_fec.c | 4 ++--
>  include/hw/net/imx_fec.h | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index eb034ffd0c..dda0816fb3 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, 
> const uint8_t *buf,
>  crc_ptr = (uint8_t *) 
>
>  /* Huge frames are truncted.  */
> -if (size > ENET_MAX_FRAME_SIZE) {
> -size = ENET_MAX_FRAME_SIZE;
> +if (size > s->regs[ENET_FTRL]) {
> +size = s->regs[ENET_FTRL];
>  flags |= ENET_BD_TR | ENET_BD_LG;
>  }
>
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 4bc8f03ec2..0fcc4f0c71 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -86,7 +86,6 @@
>  #define ENET_TCCR3 393
>  #define ENET_MAX   400
>
> -#define ENET_MAX_FRAME_SIZE2032
>
>  /* EIR and EIMR */
>  #define ENET_INT_HB(1 << 31)
> @@ -155,6 +154,8 @@
>  #define ENET_RCR_NLC   (1 << 30)
>  #define ENET_RCR_GRS   (1 << 31)
>
> +#define ENET_MAX_FRAME_SIZE(1 << ENET_RCR_MAX_FL_LENGTH)

This means we now have functions with 16K local array
variables on the stack, which seems like a bad idea.

thanks
-- PMM

[Qemu-devel] [PATCH v3 04/30] imx_fec: Use ENET_FTRL to determine truncation length

[Andrey Smirnov]

Frame truncation length, TRUNC_FL, is determined by the contents of
ENET_FTRL register, so convert the code to use it instead of a
hardcoded constant.

To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
increase the value of the latter to its theoretical maximum of 16K.

Cc: Peter Maydell 
Cc: Jason Wang 
Cc: Philippe Mathieu-Daudé 
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
Cc: yurov...@gmail.com
Signed-off-by: Andrey Smirnov 
---
 hw/net/imx_fec.c | 4 ++--
 include/hw/net/imx_fec.h | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index eb034ffd0c..dda0816fb3 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const 
uint8_t *buf,
 crc_ptr = (uint8_t *) 
 
 /* Huge frames are truncted.  */
-if (size > ENET_MAX_FRAME_SIZE) {
-size = ENET_MAX_FRAME_SIZE;
+if (size > s->regs[ENET_FTRL]) {
+size = s->regs[ENET_FTRL];
 flags |= ENET_BD_TR | ENET_BD_LG;
 }
 
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 4bc8f03ec2..0fcc4f0c71 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -86,7 +86,6 @@
 #define ENET_TCCR3 393
 #define ENET_MAX   400
 
-#define ENET_MAX_FRAME_SIZE2032
 
 /* EIR and EIMR */
 #define ENET_INT_HB(1 << 31)
@@ -155,6 +154,8 @@
 #define ENET_RCR_NLC   (1 << 30)
 #define ENET_RCR_GRS   (1 << 31)
 
+#define ENET_MAX_FRAME_SIZE(1 << ENET_RCR_MAX_FL_LENGTH)
+
 /* TCR */
 #define ENET_TCR_GTS   (1 << 0)
 #define ENET_TCR_FDEN  (1 << 2)
-- 
2.13.6