Re: [Qemu-arm] [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:00 AM, Peter Maydell <address@hidden> wrote:
> On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>>> Cc: Jason Wang <address@hidden>
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Signed-off-by: Andrey Smirnov <address@hidden>
>>> ---
>>>   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.