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

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

[Bin Meng]

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.

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)")


Bin Meng (3):
  net: Pad short frames to minimum size (60 bytes)
  Revert "e1000: Pad short frames to minimum size (60 bytes)"
  Revert "vmxnet3: Pad short frames to minimum size (60 bytes)"

 hw/net/e1000.c   | 10 +---------
 hw/net/vmxnet3.c | 10 ----------
 net/net.c        |  9 +++++++++
 3 files changed, 10 insertions(+), 19 deletions(-)

-- 
2.7.4

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

[Bin Meng]

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.

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];
     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);
-- 
2.7.4

[RFC PATCH 2/3] Revert "e1000: Pad short frames to minimum size (60 bytes)"

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

This reverts commit 78aeb23eded2d0b765bf9145c71f80025b568acd.

The short frames padding is now handled in the QEMU networking codes.
Revert this workaround in e1000.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 hw/net/e1000.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d8da2f6..8edc2e7 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -898,15 +898,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
         return 0;
     }
 
-    /* Pad to minimum Ethernet frame length */
-    if (size < sizeof(min_buf)) {
-        iov_to_buf(iov, iovcnt, 0, min_buf, size);
-        memset(&min_buf[size], 0, sizeof(min_buf) - size);
-        min_iov.iov_base = filter_buf = min_buf;
-        min_iov.iov_len = size = sizeof(min_buf);
-        iovcnt = 1;
-        iov = &min_iov;
-    } else if (iov->iov_len < MAXIMUM_ETHERNET_HDR_LEN) {
+    if (iov->iov_len < MAXIMUM_ETHERNET_HDR_LEN) {
         /* This is very unlikely, but may happen. */
         iov_to_buf(iov, iovcnt, 0, min_buf, MAXIMUM_ETHERNET_HDR_LEN);
         filter_buf = min_buf;
-- 
2.7.4

[RFC PATCH 3/3] Revert "vmxnet3: Pad short frames to minimum size (60 bytes)"

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

This reverts commit 40a87c6c9b11ef9c14e0301f76abf0eb2582f08e.

The short frames padding is now handled in the QEMU networking codes.
Revert this workaround in vmxnet3.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

 hw/net/vmxnet3.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index eff299f..d993cce 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -39,7 +39,6 @@
 
 #define PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION 0x1
 #define VMXNET3_MSIX_BAR_SIZE 0x2000
-#define MIN_BUF_SIZE 60
 
 /* Compatibility flags for migration */
 #define VMXNET3_COMPAT_FLAG_OLD_MSI_OFFSETS_BIT 0
@@ -1951,7 +1950,6 @@ vmxnet3_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     VMXNET3State *s = qemu_get_nic_opaque(nc);
     size_t bytes_indicated;
-    uint8_t min_buf[MIN_BUF_SIZE];
 
     if (!vmxnet3_can_receive(nc)) {
         VMW_PKPRN("Cannot receive now");
@@ -1964,14 +1962,6 @@ vmxnet3_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         size -= sizeof(struct virtio_net_hdr);
     }
 
-    /* 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);
-    }
-
     net_rx_pkt_set_packet_type(s->rx_pkt,
         get_eth_packet_type(PKT_GET_ETH_HDR(buf)));
 
-- 
2.7.4

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

[Philippe Mathieu-Daudé]

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);
> 

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

[Mark Cave-Ayland]

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.

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

[Peter Maydell]

On Fri, 26 Feb 2021 at 10:03, Bin Meng <bmeng.cn@gmail.com> 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.
>
> 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>
> ---

Is it better to do this here, or in the places which create
network packets? Doing it centrally has the advantage of
being just one place to change which then means senders
and receivers don't need to think about it. On the other
hand it means we don't have any equivalent of really actually
sending a short frame and having the modelled ethernet device
implement the handling of the short frame.

thanks
-- PMM

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

[Bin Meng]

On Sat, Feb 27, 2021 at 2:57 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Feb 2021 at 10:03, Bin Meng <bmeng.cn@gmail.com> 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.
> >
> > 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>
> > ---
>
> Is it better to do this here, or in the places which create
> network packets?

After looking at the eTSEC manual further, I think we should put these
codes in the send path.

> Doing it centrally has the advantage of
> being just one place to change which then means senders
> and receivers don't need to think about it. On the other
> hand it means we don't have any equivalent of really actually
> sending a short frame and having the modelled ethernet device
> implement the handling of the short frame.

Maybe the best choice would be each ethernet model duplicates the
codes in their send path?

I have not checked e1000 and vmxnet3 manual, but it seems workaround
this issue in the receive path is wrong.

Regards,
Bin