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

[Peter Maydell]

On 22 November 2017 at 20:22, Andrey Smirnov <address@hidden> wrote:
> On Tue, Nov 21, 2017 at 9:31 AM, Peter Maydell <address@hidden> wrote:
>> On 6 November 2017 at 15:47, Andrey Smirnov <address@hidden> 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