Re: [RFC PATCH 1/3] net: Pad short frames to minimum size (60 bytes)

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 26/02/2021 10:29, Philippe Mathieu-Daudé wrote:

> On 2/26/21 11:03 AM, Bin Meng wrote:
>> From: Bin Meng <bin.meng@windriver.com>
>>
>> The minimum Ethernet frame length is 60 bytes, and we should pad
>> frames whose length is smaller to the minimum size.
>>
>> This commit fixes the issue as seen with various ethernet models,
>> that ARP requests get dropped, preventing the guest from becoming
>> visible on the network.
> 
> Is it also used in commit 18995b9808d ("Send a RARP packet after
> migration.")?
> 
>> The following 2 commits that attempted to workaround this issue
>> in e1000 and vmxenet3 before, should be reverted.
>>
>>    commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
>>    commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
>>
>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>> ---
>>
>>   net/net.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/net/net.c b/net/net.c
>> index b038370..34004da 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -638,6 +638,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>>                                                    NetPacketSent *sent_cb)
>>   {
>>       NetQueue *queue;
>> +    uint8_t min_buf[60];
> 
> Can you add a definition instead of a magic value?
> Maybe ETH_FRAME_MIN_LEN in "net/eth.h"?
> 
>>       int ret;
>>   
>>   #ifdef DEBUG_NET
>> @@ -649,6 +650,14 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>>           return size;
>>       }
>>   
>> +    /* Pad to minimum Ethernet frame length */
>> +    if (size < sizeof(min_buf)) {
>> +        memcpy(min_buf, buf, size);
>> +        memset(&min_buf[size], 0, sizeof(min_buf) - size);
>> +        buf = min_buf;
>> +        size = sizeof(min_buf);
>> +    }
>> +
>>       /* Let filters handle the packet first */
>>       ret = filter_receive(sender, NET_FILTER_DIRECTION_TX,
>>                            sender, flags, buf, size, sent_cb);
>>

Looks like this pattern is also used in a few other cards. From the SPARC/PPC world:

https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/pcnet.c#L1003
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/sungem.c#L587
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/sunhme.c#L778

I can easily review/test patches that remove the same code from the cards.


ATB,

Mark.