[PATCH v2 00/13] net: Pad short frames for network backends

[PATCH v2 00/13] net: Pad short frames for network backends

[Bin Meng]

The minimum Ethernet frame length is 60 bytes. For short frames with
smaller length like ARP packets (only 42 bytes), on a real world NIC
it can choose either padding its length to the minimum required 60
bytes, or sending it out directly to the wire. Such behavior can be
hardcoded or controled by a register bit. Similarly on the receive
path, NICs can choose either dropping such short frames directly or
handing them over to software to handle.

On the other hand, for the network backends like SLiRP/TAP, they
don't expose a way to control the short frame behavior. As of today
they just send/receive data from/to the other end connected to them,
which means any sized packet is acceptable. So they can send and
receive short frames without any problem. It is observed that ARP
packets sent from SLiRP/TAP are 42 bytes, and SLiRP/TAP just send
these ARP packets to the other end which might be a NIC model that
does not allow short frames to pass through.

To provide better compatibility, for packets sent from QEMU network
backends, we change to pad short frames before sending it out to the
other end. This ensures a backend as an Ethernet sender does not
violate the spec. But with this change, the behavior of dropping
short frames in the NIC model cannot be emulated because it always
receives a packet that is spec complaint. The capability of sending
short frames from NIC models cannot be supported as well.

This series should be able to fix the issue as reported with some
NIC models before, that ARP requests get dropped, preventing the
guest from becoming visible on the network. It was workarounded in
these NIC models on the receive path, that when a short frame is
received, it is padded up to 60 bytes.

Changes in v2:
- Do the padding in the slirp/tap codes, instead of net core
- Add a 'do_not_pad' flag to NetClientState to allow net clients to
  tell peer that no padding is needed, e.g.: virtio-net

Bin Meng (13):
  net: Add ETH_ZLEN define in eth.h
  net: Add a 'do_not_pad" to NetClientState
  net: slirp: Pad short frames to minimum size before send
  net: tap: Pad short frames to minimum size before send
  hw/net: virtio-net: Initialize nc->do_not_pad to true
  hw/net: e1000: Remove the logic of padding short frames in the receive
    path
  hw/net: vmxnet3: Remove the logic of padding short frames in the
    receive path
  hw/net: i82596: Remove the logic of padding short frames in the
    receive path
  hw/net: ne2000: Remove the logic of padding short frames in the
    receive path
  hw/net: pcnet: Remove the logic of padding short frames in the receive
    path
  hw/net: rtl8139: Remove the logic of padding short frames in the
    receive path
  hw/net: sungem: Remove the logic of padding short frames in the
    receive path
  hw/net: sunhme: Remove the logic of padding short frames in the
    receive path

 include/net/eth.h   |  1 +
 include/net/net.h   |  1 +
 hw/net/e1000.c      | 11 +----------
 hw/net/i82596.c     | 18 ------------------
 hw/net/ne2000.c     | 12 ------------
 hw/net/pcnet.c      |  9 ---------
 hw/net/rtl8139.c    | 12 ------------
 hw/net/sungem.c     | 14 --------------
 hw/net/sunhme.c     | 11 -----------
 hw/net/virtio-net.c |  4 ++++
 hw/net/vmxnet3.c    | 10 ----------
 net/slirp.c         | 12 ++++++++++++
 net/tap-win32.c     | 12 ++++++++++++
 net/tap.c           | 12 ++++++++++++
 14 files changed, 43 insertions(+), 96 deletions(-)

-- 
2.25.1

[PATCH v2 01/13] net: Add ETH_ZLEN define in eth.h

[Bin Meng]

Add a new macro ETH_ZLEN which represents the minimum size of an
Ethernet frame without FCS.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 include/net/eth.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/net/eth.h b/include/net/eth.h
index 0671be6916..7c825ecb2f 100644
--- a/include/net/eth.h
+++ b/include/net/eth.h
@@ -31,6 +31,7 @@
 
 #define ETH_ALEN 6
 #define ETH_HLEN 14
+#define ETH_ZLEN 60     /* Min. octets in frame sans FCS */
 
 struct eth_header {
     uint8_t  h_dest[ETH_ALEN];   /* destination eth addr */
-- 
2.25.1

[PATCH v2 02/13] net: Add a 'do_not_pad" to NetClientState

[Bin Meng]

This adds a flag in NetClientState, so that a net client can tell
its peer that the packets do not need to be padded to the minimum
size of an Ethernet frame (60 bytes) before sending to it.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 include/net/net.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/net/net.h b/include/net/net.h
index 919facaad2..6fab1f83f5 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -100,6 +100,7 @@ struct NetClientState {
     int vring_enable;
     int vnet_hdr_len;
     bool is_netdev;
+    bool do_not_pad;
     QTAILQ_HEAD(, NetFilterState) filters;
 };
 
-- 
2.25.1

[PATCH v2 03/13] net: slirp: Pad short frames to minimum size before send

[Bin Meng]

The minimum Ethernet frame length is 60 bytes. For short frames with
smaller length like ARP packets (only 42 bytes), on a real world NIC
it can choose either padding its length to the minimum required 60
bytes, or sending it out directly to the wire. Such behavior can be
hardcoded or controled by a register bit. Similarly on the receive
path, NICs can choose either dropping such short frames directly or
handing them over to software to handle.

On the other hand, for the network backends like SLiRP/TAP, they
don't expose a way to control the short frame behavior. As of today
they just send/receive data from/to the other end connected to them,
which means any sized packet is acceptable. So they can send and
receive short frames without any problem. It is observed that ARP
packets sent from SLiRP/TAP are 42 bytes, and SLiRP/TAP just send
these ARP packets to the other end which might be a NIC model that
does not allow short frames to pass through.

To provide better compatibility, for packets sent from QEMU network
backends, we change to pad short frames before sending it out to the
other end. This ensures a backend as an Ethernet sender does not
violate the spec. But with this change, the behavior of dropping
short frames in the NIC model cannot be emulated because it always
receives a packet that is spec complaint. The capability of sending
short frames from NIC models cannot be supported as well.

This commit should be able to fix the issue as reported with some
NIC models before, that ARP requests get dropped, preventing the
guest from becoming visible on the network. It was workarounded in
these NIC models on the receive path, that when a short frame is
received, it is padded up to 60 bytes.

The following 2 commits seem to be the one to workaround this issue
in e1000 and vmxenet3 before, and should probably 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 <bmeng.cn@gmail.com>
---

 net/slirp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/slirp.c b/net/slirp.c
index be914c0be0..ad2db03182 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -31,6 +31,7 @@
 #include <pwd.h>
 #include <sys/wait.h>
 #endif
+#include "net/eth.h"
 #include "net/net.h"
 #include "clients.h"
 #include "hub.h"
@@ -115,6 +116,17 @@ static ssize_t net_slirp_send_packet(const void *pkt, size_t pkt_len,
                                      void *opaque)
 {
     SlirpState *s = opaque;
+    uint8_t min_buf[ETH_ZLEN];
+
+    if (!s->nc.peer->do_not_pad) {
+        /* Pad to minimum Ethernet frame length */
+        if (pkt_len < ETH_ZLEN) {
+            memcpy(min_buf, pkt, pkt_len);
+            memset(&min_buf[pkt_len], 0, ETH_ZLEN - pkt_len);
+            pkt = min_buf;
+            pkt_len = ETH_ZLEN;
+        }
+    }
 
     return qemu_send_packet(&s->nc, pkt, pkt_len);
 }
-- 
2.25.1

[PATCH v2 04/13] net: tap: Pad short frames to minimum size before send

[Bin Meng]

Do the same for tap backend as what we did for slirp.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 net/tap-win32.c | 12 ++++++++++++
 net/tap.c       | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 2b5dcda36e..ec35ab8ce7 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -31,6 +31,7 @@
 
 #include "qemu-common.h"
 #include "clients.h"            /* net_init_tap */
+#include "net/eth.h"
 #include "net/net.h"
 #include "net/tap.h"            /* tap_has_ufo, ... */
 #include "qemu/error-report.h"
@@ -688,9 +689,20 @@ static void tap_win32_send(void *opaque)
     uint8_t *buf;
     int max_size = 4096;
     int size;
+    uint8_t min_buf[ETH_ZLEN];
 
     size = tap_win32_read(s->handle, &buf, max_size);
     if (size > 0) {
+        if (!s->nc.peer->do_not_pad) {
+            /* Pad to minimum Ethernet frame length */
+            if (size < ETH_ZLEN) {
+                memcpy(min_buf, buf, size);
+                memset(&min_buf[size], 0, ETH_ZLEN - size);
+                buf = min_buf;
+                size = ETH_ZLEN;
+            }
+        }
+
         qemu_send_packet(&s->nc, buf, size);
         tap_win32_free_buffer(s->handle, buf);
     }
diff --git a/net/tap.c b/net/tap.c
index b7512853f4..f96b1ccfc0 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -32,6 +32,7 @@
 #include <sys/socket.h>
 #include <net/if.h>
 
+#include "net/eth.h"
 #include "net/net.h"
 #include "clients.h"
 #include "monitor/monitor.h"
@@ -189,6 +190,7 @@ static void tap_send(void *opaque)
 
     while (true) {
         uint8_t *buf = s->buf;
+        uint8_t min_buf[ETH_ZLEN];
 
         size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
         if (size <= 0) {
@@ -200,6 +202,16 @@ static void tap_send(void *opaque)
             size -= s->host_vnet_hdr_len;
         }
 
+        if (!s->nc.peer->do_not_pad) {
+            /* Pad to minimum Ethernet frame length */
+            if (size < ETH_ZLEN) {
+                memcpy(min_buf, buf, size);
+                memset(&min_buf[size], 0, ETH_ZLEN - size);
+                buf = min_buf;
+                size = ETH_ZLEN;
+            }
+        }
+
         size = qemu_send_packet_async(&s->nc, buf, size, tap_send_completed);
         if (size == 0) {
             tap_read_poll(s, false);
-- 
2.25.1

[PATCH v2 05/13] hw/net: virtio-net: Initialize nc->do_not_pad to true

[Bin Meng]

For virtio-net, there is no need to pad the Ethernet frame size to
60 bytes before sending to it.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/virtio-net.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 96a3cc8357..66b9ff4511 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3314,6 +3314,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
                               object_get_typename(OBJECT(dev)), dev->id, n);
     }
 
+    for (i = 0; i < n->max_queues; i++) {
+        n->nic->ncs[i].do_not_pad = true;
+    }
+
     peer_test_vnet_hdr(n);
     if (peer_has_vnet_hdr(n)) {
         for (i = 0; i < n->max_queues; i++) {
-- 
2.25.1

[PATCH v2 06/13] hw/net: e1000: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

This actually reverts commit 78aeb23eded2d0b765bf9145c71f80025b568acd.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

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

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d8da2f6528..a53ba9052b 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -882,7 +882,6 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
     uint16_t vlan_special = 0;
     uint8_t vlan_status = 0;
     uint8_t min_buf[MIN_BUF_SIZE];
-    struct iovec min_iov;
     uint8_t *filter_buf = iov->iov_base;
     size_t size = iov_size(iov, iovcnt);
     size_t iov_ofs = 0;
@@ -898,15 +897,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.25.1

[PATCH v2 07/13] hw/net: vmxnet3: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

This actually reverts commit 40a87c6c9b11ef9c14e0301f76abf0eb2582f08e.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

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

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index eff299f629..d993cce097 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.25.1

[PATCH v2 08/13] hw/net: i82596: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/i82596.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/hw/net/i82596.c b/hw/net/i82596.c
index 055c3a1470..1eca2e2d81 100644
--- a/hw/net/i82596.c
+++ b/hw/net/i82596.c
@@ -73,10 +73,6 @@ enum commands {
 #define I596_EOF        0x8000
 #define SIZE_MASK       0x3fff
 
-#define ETHER_TYPE_LEN 2
-#define VLAN_TCI_LEN 2
-#define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
-
 /* various flags in the chip config registers */
 #define I596_PREFETCH   (s->config[0] & 0x80)
 #define I596_PROMISC    (s->config[8] & 0x01)
@@ -489,8 +485,6 @@ bool i82596_can_receive(NetClientState *nc)
     return true;
 }
 
-#define MIN_BUF_SIZE 60
-
 ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
 {
     I82596State *s = qemu_get_nic_opaque(nc);
@@ -501,7 +495,6 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
     size_t bufsz = sz; /* length of data in buf */
     uint32_t crc;
     uint8_t *crc_ptr;
-    uint8_t buf1[MIN_BUF_SIZE + VLAN_HLEN];
     static const uint8_t broadcast_macaddr[6] = {
                 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 
@@ -584,17 +577,6 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
         }
     }
 
-    /* if too small buffer, then expand it */
-    if (len < MIN_BUF_SIZE + VLAN_HLEN) {
-        memcpy(buf1, buf, len);
-        memset(buf1 + len, 0, MIN_BUF_SIZE + VLAN_HLEN - len);
-        buf = buf1;
-        if (len < MIN_BUF_SIZE) {
-            len = MIN_BUF_SIZE;
-        }
-        bufsz = len;
-    }
-
     /* Calculate the ethernet checksum (4 bytes) */
     len += 4;
     crc = cpu_to_be32(crc32(~0, buf, sz));
-- 
2.25.1

[PATCH v2 09/13] hw/net: ne2000: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/ne2000.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index 6c17ee1ae2..b0a120ece6 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -167,15 +167,12 @@ static int ne2000_buffer_full(NE2000State *s)
     return 0;
 }
 
-#define MIN_BUF_SIZE 60
-
 ssize_t ne2000_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
 {
     NE2000State *s = qemu_get_nic_opaque(nc);
     size_t size = size_;
     uint8_t *p;
     unsigned int total_len, next, avail, len, index, mcast_idx;
-    uint8_t buf1[60];
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 
@@ -213,15 +210,6 @@ ssize_t ne2000_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
         }
     }
 
-
-    /* if too small buffer, then expand it */
-    if (size < MIN_BUF_SIZE) {
-        memcpy(buf1, buf, size);
-        memset(buf1 + size, 0, MIN_BUF_SIZE - size);
-        buf = buf1;
-        size = MIN_BUF_SIZE;
-    }
-
     index = s->curpag << 8;
     if (index >= NE2000_PMEM_END) {
         index = s->start;
-- 
2.25.1

[PATCH v2 10/13] hw/net: pcnet: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/pcnet.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index f3f18d8598..16330335cd 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -987,7 +987,6 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
 {
     PCNetState *s = qemu_get_nic_opaque(nc);
     int is_padr = 0, is_bcast = 0, is_ladr = 0;
-    uint8_t buf1[60];
     int remaining;
     int crc_err = 0;
     size_t size = size_;
@@ -1000,14 +999,6 @@ ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
     printf("pcnet_receive size=%zu\n", size);
 #endif
 
-    /* if too small buffer, then expand it */
-    if (size < MIN_BUF_SIZE) {
-        memcpy(buf1, buf, size);
-        memset(buf1 + size, 0, MIN_BUF_SIZE - size);
-        buf = buf1;
-        size = MIN_BUF_SIZE;
-    }
-
     if (CSR_PROM(s)
         || (is_padr=padr_match(s, buf, size))
         || (is_bcast=padr_bcast(s, buf, size))
-- 
2.25.1

[PATCH v2 11/13] hw/net: rtl8139: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/rtl8139.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 4675ac878e..cbfe29a286 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -827,7 +827,6 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
 
     uint32_t packet_header = 0;
 
-    uint8_t buf1[MIN_BUF_SIZE + VLAN_HLEN];
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 
@@ -939,17 +938,6 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
         }
     }
 
-    /* if too small buffer, then expand it
-     * Include some tailroom in case a vlan tag is later removed. */
-    if (size < MIN_BUF_SIZE + VLAN_HLEN) {
-        memcpy(buf1, buf, size);
-        memset(buf1 + size, 0, MIN_BUF_SIZE + VLAN_HLEN - size);
-        buf = buf1;
-        if (size < MIN_BUF_SIZE) {
-            size = MIN_BUF_SIZE;
-        }
-    }
-
     if (rtl8139_cp_receiver_enabled(s))
     {
         if (!rtl8139_cp_rx_valid(s)) {
-- 
2.25.1

[PATCH v2 12/13] hw/net: sungem: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/sungem.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/hw/net/sungem.c b/hw/net/sungem.c
index 33c3722df6..3fa83168db 100644
--- a/hw/net/sungem.c
+++ b/hw/net/sungem.c
@@ -550,7 +550,6 @@ static ssize_t sungem_receive(NetClientState *nc, const uint8_t *buf,
     PCIDevice *d = PCI_DEVICE(s);
     uint32_t mac_crc, done, kick, max_fsize;
     uint32_t fcs_size, ints, rxdma_cfg, rxmac_cfg, csum, coff;
-    uint8_t smallbuf[60];
     struct gem_rxd desc;
     uint64_t dbase, baddr;
     unsigned int rx_cond;
@@ -584,19 +583,6 @@ static ssize_t sungem_receive(NetClientState *nc, const uint8_t *buf,
         return size;
     }
 
-    /* We don't drop too small frames since we get them in qemu, we pad
-     * them instead. We should probably use the min frame size register
-     * but I don't want to use a variable size staging buffer and I
-     * know both MacOS and Linux use the default 64 anyway. We use 60
-     * here to account for the non-existent FCS.
-     */
-    if (size < 60) {
-        memcpy(smallbuf, buf, size);
-        memset(&smallbuf[size], 0, 60 - size);
-        buf = smallbuf;
-        size = 60;
-    }
-
     /* Get MAC crc */
     mac_crc = net_crc32_le(buf, ETH_ALEN);
 
-- 
2.25.1

[PATCH v2 13/13] hw/net: sunhme: Remove the logic of padding short frames in the receive path

[Bin Meng]

Now that we have implemented unified short frames padding in the
QEMU networking codes, remove the same logic in the NIC codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

 hw/net/sunhme.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/net/sunhme.c b/hw/net/sunhme.c
index fc34905f87..6971796e57 100644
--- a/hw/net/sunhme.c
+++ b/hw/net/sunhme.c
@@ -714,8 +714,6 @@ static inline void sunhme_set_rx_ring_nr(SunHMEState *s, int i)
     s->erxregs[HME_ERXI_RING >> 2] = ring;
 }
 
-#define MIN_BUF_SIZE 60
-
 static ssize_t sunhme_receive(NetClientState *nc, const uint8_t *buf,
                               size_t size)
 {
@@ -724,7 +722,6 @@ static ssize_t sunhme_receive(NetClientState *nc, const uint8_t *buf,
     dma_addr_t rb, addr;
     uint32_t intstatus, status, buffer, buffersize, sum;
     uint16_t csum;
-    uint8_t buf1[60];
     int nr, cr, len, rxoffset, csum_offset;
 
     trace_sunhme_rx_incoming(size);
@@ -775,14 +772,6 @@ static ssize_t sunhme_receive(NetClientState *nc, const uint8_t *buf,
 
     trace_sunhme_rx_filter_accept();
 
-    /* If too small buffer, then expand it */
-    if (size < MIN_BUF_SIZE) {
-        memcpy(buf1, buf, size);
-        memset(buf1 + size, 0, MIN_BUF_SIZE - size);
-        buf = buf1;
-        size = MIN_BUF_SIZE;
-    }
-
     rb = s->erxregs[HME_ERXI_RING >> 2] & HME_ERXI_RING_ADDR;
     nr = sunhme_get_rx_ring_count(s);
     cr = sunhme_get_rx_ring_nr(s);
-- 
2.25.1

Re: [PATCH v2 01/13] net: Add ETH_ZLEN define in eth.h

[Philippe Mathieu-Daudé]

On 3/15/21 8:57 AM, Bin Meng wrote:
> Add a new macro ETH_ZLEN which represents the minimum size of an
> Ethernet frame without FCS.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  include/net/eth.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/net/eth.h b/include/net/eth.h
> index 0671be6916..7c825ecb2f 100644
> --- a/include/net/eth.h
> +++ b/include/net/eth.h
> @@ -31,6 +31,7 @@
>  
>  #define ETH_ALEN 6
>  #define ETH_HLEN 14
> +#define ETH_ZLEN 60     /* Min. octets in frame sans FCS */

What means 'sans'?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
>  struct eth_header {
>      uint8_t  h_dest[ETH_ALEN];   /* destination eth addr */
> 

Re: [PATCH v2 02/13] net: Add a 'do_not_pad" to NetClientState

[Philippe Mathieu-Daudé]

On 3/15/21 8:57 AM, Bin Meng wrote:
> This adds a flag in NetClientState, so that a net client can tell
> its peer that the packets do not need to be padded to the minimum
> size of an Ethernet frame (60 bytes) before sending to it.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  include/net/net.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 919facaad2..6fab1f83f5 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -100,6 +100,7 @@ struct NetClientState {
>      int vring_enable;
>      int vnet_hdr_len;
>      bool is_netdev;
> +    bool do_not_pad;
>      QTAILQ_HEAD(, NetFilterState) filters;
>  };

This is a bit pointless without the next patch, why
not squash it there?

Re: [PATCH v2 02/13] net: Add a 'do_not_pad" to NetClientState

[Philippe Mathieu-Daudé]

On 3/15/21 10:17 AM, Philippe Mathieu-Daudé wrote:
> On 3/15/21 8:57 AM, Bin Meng wrote:
>> This adds a flag in NetClientState, so that a net client can tell
>> its peer that the packets do not need to be padded to the minimum
>> size of an Ethernet frame (60 bytes) before sending to it.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  include/net/net.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/net/net.h b/include/net/net.h
>> index 919facaad2..6fab1f83f5 100644
>> --- a/include/net/net.h
>> +++ b/include/net/net.h
>> @@ -100,6 +100,7 @@ struct NetClientState {
>>      int vring_enable;
>>      int vnet_hdr_len;
>>      bool is_netdev;
>> +    bool do_not_pad;
>>      QTAILQ_HEAD(, NetFilterState) filters;
>>  };
> 
> This is a bit pointless without the next patch, why
> not squash it there?

Ah one is SLiRP and the other is tap. OK then.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 02/13] net: Add a 'do_not_pad" to NetClientState

[Philippe Mathieu-Daudé]

On 3/15/21 10:18 AM, Philippe Mathieu-Daudé wrote:
> On 3/15/21 10:17 AM, Philippe Mathieu-Daudé wrote:
>> On 3/15/21 8:57 AM, Bin Meng wrote:
>>> This adds a flag in NetClientState, so that a net client can tell
>>> its peer that the packets do not need to be padded to the minimum
>>> size of an Ethernet frame (60 bytes) before sending to it.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  include/net/net.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/net/net.h b/include/net/net.h
>>> index 919facaad2..6fab1f83f5 100644
>>> --- a/include/net/net.h
>>> +++ b/include/net/net.h
>>> @@ -100,6 +100,7 @@ struct NetClientState {
>>>      int vring_enable;
>>>      int vnet_hdr_len;
>>>      bool is_netdev;
>>> +    bool do_not_pad;

Maybe 'do_not_pad_to_min_eth_frame_len' to avoid
wondering what padding is it.

>>>      QTAILQ_HEAD(, NetFilterState) filters;
>>>  };
>>
>> This is a bit pointless without the next patch, why
>> not squash it there?
> 
> Ah one is SLiRP and the other is tap. OK then.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Re: [PATCH v2 01/13] net: Add ETH_ZLEN define in eth.h

[Bin Meng]

Hi Philippe,

On Mon, Mar 15, 2021 at 5:13 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 3/15/21 8:57 AM, Bin Meng wrote:
> > Add a new macro ETH_ZLEN which represents the minimum size of an
> > Ethernet frame without FCS.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  include/net/eth.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/net/eth.h b/include/net/eth.h
> > index 0671be6916..7c825ecb2f 100644
> > --- a/include/net/eth.h
> > +++ b/include/net/eth.h
> > @@ -31,6 +31,7 @@
> >
> >  #define ETH_ALEN 6
> >  #define ETH_HLEN 14
> > +#define ETH_ZLEN 60     /* Min. octets in frame sans FCS */
>
> What means 'sans'?

sans-serif font? Does that sound familiar? :)

Please check:
https://www.dictionary.com/browse/sans

This comment was not invented by me, but was just a copy from the one
used in Linux kernel:
https://github.com/torvalds/linux/blob/d635a69dd4981cc51f90293f5f64268620ed1565/include/uapi/linux/if_ether.h#L35

>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Regards,
Bin

Re: [PATCH v2 02/13] net: Add a 'do_not_pad" to NetClientState

[Bin Meng]

Hi Philippe,

On Mon, Mar 15, 2021 at 5:22 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 3/15/21 10:18 AM, Philippe Mathieu-Daudé wrote:
> > On 3/15/21 10:17 AM, Philippe Mathieu-Daudé wrote:
> >> On 3/15/21 8:57 AM, Bin Meng wrote:
> >>> This adds a flag in NetClientState, so that a net client can tell
> >>> its peer that the packets do not need to be padded to the minimum
> >>> size of an Ethernet frame (60 bytes) before sending to it.
> >>>
> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >>> ---
> >>>
> >>>  include/net/net.h | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/include/net/net.h b/include/net/net.h
> >>> index 919facaad2..6fab1f83f5 100644
> >>> --- a/include/net/net.h
> >>> +++ b/include/net/net.h
> >>> @@ -100,6 +100,7 @@ struct NetClientState {
> >>>      int vring_enable;
> >>>      int vnet_hdr_len;
> >>>      bool is_netdev;
> >>> +    bool do_not_pad;
>
> Maybe 'do_not_pad_to_min_eth_frame_len' to avoid
> wondering what padding is it.

I felt the same when I added this :) But I wonder if that name is too long ..

Regards,
Bin

Re: [PATCH v2 02/13] net: Add a 'do_not_pad" to NetClientState

[Peter Maydell]

On Mon, 15 Mar 2021 at 10:17, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Philippe,
>
> On Mon, Mar 15, 2021 at 5:22 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > On 3/15/21 10:18 AM, Philippe Mathieu-Daudé wrote:
> > > On 3/15/21 10:17 AM, Philippe Mathieu-Daudé wrote:
> > >> On 3/15/21 8:57 AM, Bin Meng wrote:
> > >>> This adds a flag in NetClientState, so that a net client can tell
> > >>> its peer that the packets do not need to be padded to the minimum
> > >>> size of an Ethernet frame (60 bytes) before sending to it.
> > >>>
> > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > >>> ---
> > >>>
> > >>>  include/net/net.h | 1 +
> > >>>  1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/include/net/net.h b/include/net/net.h
> > >>> index 919facaad2..6fab1f83f5 100644
> > >>> --- a/include/net/net.h
> > >>> +++ b/include/net/net.h
> > >>> @@ -100,6 +100,7 @@ struct NetClientState {
> > >>>      int vring_enable;
> > >>>      int vnet_hdr_len;
> > >>>      bool is_netdev;
> > >>> +    bool do_not_pad;
> >
> > Maybe 'do_not_pad_to_min_eth_frame_len' to avoid
> > wondering what padding is it.
>
> I felt the same when I added this :) But I wonder if that name is too long ..

If you don't change the name, you could at least add a short
comment in the structure definition describing what the flag does.

thanks
-- PMM

Re: [PATCH v2 01/13] net: Add ETH_ZLEN define in eth.h

[Philippe Mathieu-Daudé]

On 3/15/21 11:15 AM, Bin Meng wrote:
> Hi Philippe,
> 
> On Mon, Mar 15, 2021 at 5:13 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> On 3/15/21 8:57 AM, Bin Meng wrote:
>>> Add a new macro ETH_ZLEN which represents the minimum size of an
>>> Ethernet frame without FCS.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  include/net/eth.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/net/eth.h b/include/net/eth.h
>>> index 0671be6916..7c825ecb2f 100644
>>> --- a/include/net/eth.h
>>> +++ b/include/net/eth.h
>>> @@ -31,6 +31,7 @@
>>>
>>>  #define ETH_ALEN 6
>>>  #define ETH_HLEN 14
>>> +#define ETH_ZLEN 60     /* Min. octets in frame sans FCS */
>>
>> What means 'sans'?
> 
> sans-serif font? Does that sound familiar? :)
> 
> Please check:
> https://www.dictionary.com/browse/sans

Ah this is a British expression from French...

I'd use "without" instead as it is probably clearer for
non-British and non-French developers.

> This comment was not invented by me, but was just a copy from the one
> used in Linux kernel:
> https://github.com/torvalds/linux/blob/d635a69dd4981cc51f90293f5f64268620ed1565/include/uapi/linux/if_ether.h#L35
> 
>>
>> Otherwise:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
> 
> Regards,
> Bin
> 

Re: [PATCH v2 03/13] net: slirp: Pad short frames to minimum size before send

[Jason Wang]

在 2021/3/15 下午3:57, Bin Meng 写道:
> The minimum Ethernet frame length is 60 bytes. For short frames with
> smaller length like ARP packets (only 42 bytes), on a real world NIC
> it can choose either padding its length to the minimum required 60
> bytes, or sending it out directly to the wire. Such behavior can be
> hardcoded or controled by a register bit. Similarly on the receive
> path, NICs can choose either dropping such short frames directly or
> handing them over to software to handle.
>
> On the other hand, for the network backends like SLiRP/TAP, they
> don't expose a way to control the short frame behavior. As of today
> they just send/receive data from/to the other end connected to them,
> which means any sized packet is acceptable. So they can send and
> receive short frames without any problem. It is observed that ARP
> packets sent from SLiRP/TAP are 42 bytes, and SLiRP/TAP just send
> these ARP packets to the other end which might be a NIC model that
> does not allow short frames to pass through.
>
> To provide better compatibility, for packets sent from QEMU network
> backends, we change to pad short frames before sending it out to the
> other end. This ensures a backend as an Ethernet sender does not
> violate the spec. But with this change, the behavior of dropping
> short frames in the NIC model cannot be emulated because it always
> receives a packet that is spec complaint. The capability of sending
> short frames from NIC models cannot be supported as well.
>
> This commit should be able to fix the issue as reported with some
> NIC models before, that ARP requests get dropped, preventing the
> guest from becoming visible on the network. It was workarounded in
> these NIC models on the receive path, that when a short frame is
> received, it is padded up to 60 bytes.
>
> The following 2 commits seem to be the one to workaround this issue
> in e1000 and vmxenet3 before, and should probably 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 <bmeng.cn@gmail.com>
> ---
>
>   net/slirp.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index be914c0be0..ad2db03182 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -31,6 +31,7 @@
>   #include <pwd.h>
>   #include <sys/wait.h>
>   #endif
> +#include "net/eth.h"
>   #include "net/net.h"
>   #include "clients.h"
>   #include "hub.h"
> @@ -115,6 +116,17 @@ static ssize_t net_slirp_send_packet(const void *pkt, size_t pkt_len,
>                                        void *opaque)
>   {
>       SlirpState *s = opaque;
> +    uint8_t min_buf[ETH_ZLEN];
> +
> +    if (!s->nc.peer->do_not_pad) {
> +        /* Pad to minimum Ethernet frame length */
> +        if (pkt_len < ETH_ZLEN) {
> +            memcpy(min_buf, pkt, pkt_len);
> +            memset(&min_buf[pkt_len], 0, ETH_ZLEN - pkt_len);
> +            pkt = min_buf;
> +            pkt_len = ETH_ZLEN;
> +        }
> +    }


Let's introduce a helper for this padding then it could be reused by at 
least TAP?

Thanks


>   
>       return qemu_send_packet(&s->nc, pkt, pkt_len);
>   }