| Message ID | 20171106154813.19936-5-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org> > Cc: Jason Wang <jasowang@redhat.com> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org > Cc: yurovsky@gmail.com > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > 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 *) &crc; > > /* 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_SIZE 2032 > > /* 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
On Tue, Nov 21, 2017 at 9:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> 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 <peter.maydell@linaro.org> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> >> Cc: qemu-devel@nongnu.org >> Cc: qemu-arm@nongnu.org >> Cc: yurovsky@gmail.com >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> 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 *) &crc; >> >> /* 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_SIZE 2032 >> >> /* 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
On 22 November 2017 at 20:22, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > On Tue, Nov 21, 2017 at 9:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 6 November 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> 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
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 *) &crc; /* 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_SIZE 2032 /* 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)
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 <peter.maydell@linaro.org> Cc: Jason Wang <jasowang@redhat.com> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: qemu-devel@nongnu.org Cc: qemu-arm@nongnu.org Cc: yurovsky@gmail.com Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- hw/net/imx_fec.c | 4 ++-- include/hw/net/imx_fec.h | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-)