| Message ID | 20170918195100.17593-6-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Andrey, > > On 09/18/2017 04:50 PM, 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. >> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Cc: Jason Wang <jasowang@redhat.com> >> 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 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c >> index 767402909d..989c11be5f 100644 >> --- a/hw/net/imx_fec.c >> +++ b/hw/net/imx_fec.c >> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, >> const uint8_t *buf, >> size += 4; >> /* 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; >> } > > > for this to be ok you need to update imx_enet_write(), such: > > case ENET_FTRL: > - s->regs[index] = value & 0x00003fff; > + value &= 0x00003fff; > + if (value > ENET_MAX_FRAME_SIZE) { > + warn_report("%s: guest requested bigger " > + "frame size than QEMU supports " > + "(%u > %u)", value, > + ENET_MAX_FRAME_SIZE); > + value = ENET_MAX_FRAME_SIZE; > + } > + s->regs[index] = value; > break; Yes, and also an incoming-migration post_load callback that fails migration if the value is too large. It might be simpler to truncate to the smaller of the ENET_FTRL register value and ENET_MAX_FRAME_SIZE. (PS: what is the hardware behaviour if ENET_FTRL is set to a larger value than ENET_MAX_FRAME_SIZE ?) thanks -- PMM
On 18 September 2017 at 20:50, 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. > > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Jason Wang <jasowang@redhat.com> > 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 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c > index 767402909d..989c11be5f 100644 > --- a/hw/net/imx_fec.c > +++ b/hw/net/imx_fec.c > @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf, > size += 4; > > /* 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; > } PS: since you're editing this bit of code anyway, can you fix the typo in the comment in passing: should be "truncated". thanks -- PMM
On Fri, Oct 6, 2017 at 7:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Hi Andrey, >> >> On 09/18/2017 04:50 PM, 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. >>> >>> Cc: Peter Maydell <peter.maydell@linaro.org> >>> Cc: Jason Wang <jasowang@redhat.com> >>> 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 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c >>> index 767402909d..989c11be5f 100644 >>> --- a/hw/net/imx_fec.c >>> +++ b/hw/net/imx_fec.c >>> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, >>> const uint8_t *buf, >>> size += 4; >>> /* 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; >>> } >> >> >> for this to be ok you need to update imx_enet_write(), such: >> >> case ENET_FTRL: >> - s->regs[index] = value & 0x00003fff; >> + value &= 0x00003fff; >> + if (value > ENET_MAX_FRAME_SIZE) { >> + warn_report("%s: guest requested bigger " >> + "frame size than QEMU supports " >> + "(%u > %u)", value, >> + ENET_MAX_FRAME_SIZE); >> + value = ENET_MAX_FRAME_SIZE; >> + } >> + s->regs[index] = value; >> break; > > Yes, and also an incoming-migration post_load callback that > fails migration if the value is too large. > > It might be simpler to truncate to the smaller of the ENET_FTRL > register value and ENET_MAX_FRAME_SIZE. > > (PS: what is the hardware behaviour if ENET_FTRL is set to > a larger value than ENET_MAX_FRAME_SIZE ?) > I haven't tested it in practice, but I assume that hardware should be capable of receiving packets of up to ENET_RCR[MAX_FL] bytes big (both RCR[MAX_FL] and FTRL are 14-bits), since that would allow supporting networks with jumbo frames. What if ENET_MAX_FRAME_SIZE is increased to 16K instead? Would that make all of that additional error checking code unnecessary? Thanks, Andrey Smirnov P.S: And sure, I'll fix the typo in v2.
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index 767402909d..989c11be5f 100644 --- a/hw/net/imx_fec.c +++ b/hw/net/imx_fec.c @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf, size += 4; /* 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; }
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. Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Jason Wang <jasowang@redhat.com> 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 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)