[RFC PATCH v3 00/10] net: Handle short frames for SLiRP/TAP interfaces

[RFC PATCH v3 00/10] net: Handle short frames for SLiRP/TAP interfaces

[Philippe Mathieu-Daudé]

This is Bin's series but using iovec structure in 1st patch
for zero copy.

Bin's cover:

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 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 SLiRP/TAP, we
change to pad short frames before sending it out to the other end.
This ensures SLiRP/TAP as an Ethernet sender do 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 are still supported and short frames can still pass through
SLiRP/TAP interfaces.

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.

Bin Meng (9):
  net: Pad short frames to minimum size before send from SLiRP/TAP
  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

Philippe Mathieu-Daudé (1):
  net: Use 'struct iovec' in qemu_send_packet_async_with_flags()

 include/net/eth.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/vmxnet3.c  | 10 ----------
 net/net.c         | 47 ++++++++++++++++++++++++++---------------------
 10 files changed, 28 insertions(+), 117 deletions(-)

-- 
2.26.2

[RFC PATCH v3 01/10] net: Use 'struct iovec' in qemu_send_packet_async_with_flags()

[Philippe Mathieu-Daudé]

Directly use iovec structure in qemu_send_packet_async_with_flags()
by inlining filter_receive() and using qemu_net_queue_send_iov()
instead of qemu_net_queue_send().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/net.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/net/net.c b/net/net.c
index fb7b7dcc252..159e4d0ec25 100644
--- a/net/net.c
+++ b/net/net.c
@@ -581,22 +581,6 @@ static ssize_t filter_receive_iov(NetClientState *nc,
     return ret;
 }
 
-static ssize_t filter_receive(NetClientState *nc,
-                              NetFilterDirection direction,
-                              NetClientState *sender,
-                              unsigned flags,
-                              const uint8_t *data,
-                              size_t size,
-                              NetPacketSent *sent_cb)
-{
-    struct iovec iov = {
-        .iov_base = (void *)data,
-        .iov_len = size
-    };
-
-    return filter_receive_iov(nc, direction, sender, flags, &iov, 1, sent_cb);
-}
-
 void qemu_purge_queued_packets(NetClientState *nc)
 {
     if (!nc->peer) {
@@ -638,6 +622,13 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
 {
     NetQueue *queue;
     int ret;
+    int iovcnt = 1;
+    struct iovec iov[] = {
+        [0] = {
+            .iov_base = (void *)buf,
+            .iov_len = size,
+        },
+    };
 
 #ifdef DEBUG_NET
     printf("qemu_send_packet_async:\n");
@@ -649,21 +640,21 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
     }
 
     /* Let filters handle the packet first */
-    ret = filter_receive(sender, NET_FILTER_DIRECTION_TX,
-                         sender, flags, buf, size, sent_cb);
+    ret = filter_receive_iov(sender, NET_FILTER_DIRECTION_TX,
+                             sender, flags, iov, iovcnt, sent_cb);
     if (ret) {
         return ret;
     }
 
-    ret = filter_receive(sender->peer, NET_FILTER_DIRECTION_RX,
-                         sender, flags, buf, size, sent_cb);
+    ret = filter_receive_iov(sender->peer, NET_FILTER_DIRECTION_RX,
+                             sender, flags, iov, iovcnt, sent_cb);
     if (ret) {
         return ret;
     }
 
     queue = sender->peer->incoming_queue;
 
-    return qemu_net_queue_send(queue, sender, flags, buf, size, sent_cb);
+    return qemu_net_queue_send_iov(queue, sender, flags, iov, iovcnt, sent_cb);
 }
 
 ssize_t qemu_send_packet_async(NetClientState *sender,
-- 
2.26.2

[RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Philippe Mathieu-Daudé]

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

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 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 SLiRP/TAP, we
change to pad short frames before sending it out to the other end.
This ensures SLiRP/TAP as an Ethernet sender do 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 are still supported and short frames can still pass through
SLiRP/TAP interfaces.

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-2-git-send-email-bmeng.cn@gmail.com>
[PMD: Use struct iovec for zero-copy]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/net/eth.h |  1 +
 net/net.c         | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/net/eth.h b/include/net/eth.h
index 0671be69165..7c825ecb2f7 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 */
diff --git a/net/net.c b/net/net.c
index 159e4d0ec25..d42ac9365eb 100644
--- a/net/net.c
+++ b/net/net.c
@@ -620,6 +620,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
                                                  const uint8_t *buf, int size,
                                                  NetPacketSent *sent_cb)
 {
+    static const uint8_t null_buf[ETH_ZLEN] = { };
     NetQueue *queue;
     int ret;
     int iovcnt = 1;
@@ -628,6 +629,10 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
             .iov_base = (void *)buf,
             .iov_len = size,
         },
+        [1] = {
+            .iov_base = (void *)null_buf,
+            .iov_len = ETH_ZLEN,
+        },
     };
 
 #ifdef DEBUG_NET
@@ -639,6 +644,15 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
         return size;
     }
 
+    /* Pad to minimum Ethernet frame length for SLiRP and TAP */
+    if (sender->info->type == NET_CLIENT_DRIVER_USER ||
+        sender->info->type == NET_CLIENT_DRIVER_TAP) {
+        if (size < ETH_ZLEN) {
+            iov[1].iov_len = ETH_ZLEN - size;
+            iovcnt = 2;
+        }
+    }
+
     /* Let filters handle the packet first */
     ret = filter_receive_iov(sender, NET_FILTER_DIRECTION_TX,
                              sender, flags, iov, iovcnt, sent_cb);
-- 
2.26.2

[RFC PATCH v3 03/10] hw/net: e1000: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-3-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.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 d8da2f6528b..a53ba9052b4 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.26.2

[RFC PATCH v3 04/10] hw/net: vmxnet3: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-4-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/vmxnet3.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index eff299f6290..d993cce097a 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.26.2

[RFC PATCH v3 05/10] hw/net: i82596: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-5-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/i82596.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/hw/net/i82596.c b/hw/net/i82596.c
index 055c3a1470c..1eca2e2d816 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.26.2

[RFC PATCH v3 06/10] hw/net: ne2000: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-6-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/ne2000.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index 6c17ee1ae21..b0a120ece63 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.26.2

[RFC PATCH v3 07/10] hw/net: pcnet: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-7-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/pcnet.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index f3f18d8598c..16330335cd2 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.26.2

[RFC PATCH v3 08/10] hw/net: rtl8139: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-8-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/rtl8139.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 4675ac878e9..cbfe29a2867 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.26.2

[RFC PATCH v3 09/10] hw/net: sungem: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-9-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/sungem.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/hw/net/sungem.c b/hw/net/sungem.c
index 33c3722df6f..3fa83168db0 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.26.2

[RFC PATCH v3 10/10] hw/net: sunhme: Remove the logic of padding short frames in the receive path

[Philippe Mathieu-Daudé]

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

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 <bin.meng@windriver.com>
Message-Id: <1614763306-18026-10-git-send-email-bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/sunhme.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/net/sunhme.c b/hw/net/sunhme.c
index fc34905f875..6971796e575 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.26.2

Re: [RFC PATCH v3 00/10] net: Handle short frames for SLiRP/TAP interfaces

[Bin Meng]

On Thu, Mar 4, 2021 at 3:12 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> This is Bin's series but using iovec structure in 1st patch
> for zero copy.
>
> Bin's cover:
>
> 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 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 SLiRP/TAP, we
> change to pad short frames before sending it out to the other end.
> This ensures SLiRP/TAP as an Ethernet sender do 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 are still supported and short frames can still pass through
> SLiRP/TAP interfaces.
>
> 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.

Ping?

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/4 3:11 上午, Philippe Mathieu-Daudé wrote:
> From: Bin Meng <bin.meng@windriver.com>
>
> 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 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.


Do we need to care about other type of networking backends? E.g socket.

Or at least we should keep the padding logic if we can't audit all of 
the backends.

Thanks


>
> To provide better compatibility, for packets sent from SLiRP/TAP, we
> change to pad short frames before sending it out to the other end.
> This ensures SLiRP/TAP as an Ethernet sender do 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 are still supported and short frames can still pass through
> SLiRP/TAP interfaces.
>
> 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 <bin.meng@windriver.com>
> Message-Id: <1614763306-18026-2-git-send-email-bmeng.cn@gmail.com>
> [PMD: Use struct iovec for zero-copy]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/net/eth.h |  1 +
>   net/net.c         | 14 ++++++++++++++
>   2 files changed, 15 insertions(+)
>
> diff --git a/include/net/eth.h b/include/net/eth.h
> index 0671be69165..7c825ecb2f7 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 */
> diff --git a/net/net.c b/net/net.c
> index 159e4d0ec25..d42ac9365eb 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -620,6 +620,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>                                                    const uint8_t *buf, int size,
>                                                    NetPacketSent *sent_cb)
>   {
> +    static const uint8_t null_buf[ETH_ZLEN] = { };
>       NetQueue *queue;
>       int ret;
>       int iovcnt = 1;
> @@ -628,6 +629,10 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>               .iov_base = (void *)buf,
>               .iov_len = size,
>           },
> +        [1] = {
> +            .iov_base = (void *)null_buf,
> +            .iov_len = ETH_ZLEN,
> +        },
>       };
>   
>   #ifdef DEBUG_NET
> @@ -639,6 +644,15 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>           return size;
>       }
>   
> +    /* Pad to minimum Ethernet frame length for SLiRP and TAP */
> +    if (sender->info->type == NET_CLIENT_DRIVER_USER ||
> +        sender->info->type == NET_CLIENT_DRIVER_TAP) {
> +        if (size < ETH_ZLEN) {
> +            iov[1].iov_len = ETH_ZLEN - size;
> +            iovcnt = 2;
> +        }
> +    }
> +
>       /* Let filters handle the packet first */
>       ret = filter_receive_iov(sender, NET_FILTER_DIRECTION_TX,
>                                sender, flags, iov, iovcnt, sent_cb);

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Mon, Mar 8, 2021 at 11:48 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/4 3:11 上午, Philippe Mathieu-Daudé wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > 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 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.
>
>
> Do we need to care about other type of networking backends? E.g socket.
>

I am not sure as I never used other backends. If someone who is more
familiar with the network codes better than me can confirm other
backends are also needed, we might do:

if (sender->info->type != NET_CLIENT_DRIVER_NIC)

> Or at least we should keep the padding logic if we can't audit all of
> the backends.

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Peter Maydell]

On Mon, 8 Mar 2021 at 03:48, Jason Wang <jasowang@redhat.com> wrote:
> Do we need to care about other type of networking backends? E.g socket.
>
> Or at least we should keep the padding logic if we can't audit all of
> the backends.

I think the key thing we need to do here is make a decision
and be clear about what we're doing. There are three options
I can see:

(1) we say that the net API demands that backends pad
packets they emit to the minimum ethernet frame length
unless they specifically are intending to emit a short frame,
and we fix any backends that don't comply (or equivalently,
add support in the core code for a backend to mark itself
as "I don't pad; please do it for me").

(2) we say that the networking subsystem doesn't support
short packets, and just have the common code always enforce
padding short frames to the minimum length somewhere between
when it receives a packet from a backend and passes it to
a NIC model.

(3) we say that it's the job of the NIC models to pad
short frames as they see them coming in.

I think (3) is pretty clearly the worst of these, since it
requires every NIC model to handle it; it has no advantages
over (2) that I can see. I don't have a strong take on whether
we'd rather have (1) or (2): it's a tradeoff between whether
we support modelling of short frames vs simplicity of code.
I'd just like us to be clear about what point or points in
the code have the responsibility for padding short frames.

On
+    if (sender->info->type == NET_CLIENT_DRIVER_USER ||
+        sender->info->type == NET_CLIENT_DRIVER_TAP) {

vs
> if (sender->info->type != NET_CLIENT_DRIVER_NIC)

another option would be to add a 'bool pad_short_tx_frames' to
the NetClientInfo struct, and have those things which don't
pad set it to true.

thanks
-- PMM

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/8 6:22 下午, Peter Maydell wrote:
> On Mon, 8 Mar 2021 at 03:48, Jason Wang <jasowang@redhat.com> wrote:
>> Do we need to care about other type of networking backends? E.g socket.
>>
>> Or at least we should keep the padding logic if we can't audit all of
>> the backends.
> I think the key thing we need to do here is make a decision
> and be clear about what we're doing. There are three options
> I can see:
>
> (1) we say that the net API demands that backends pad
> packets they emit to the minimum ethernet frame length
> unless they specifically are intending to emit a short frame,
> and we fix any backends that don't comply (or equivalently,
> add support in the core code for a backend to mark itself
> as "I don't pad; please do it for me").
>
> (2) we say that the networking subsystem doesn't support
> short packets, and just have the common code always enforce
> padding short frames to the minimum length somewhere between
> when it receives a packet from a backend and passes it to
> a NIC model.
>
> (3) we say that it's the job of the NIC models to pad
> short frames as they see them coming in.
>
> I think (3) is pretty clearly the worst of these, since it
> requires every NIC model to handle it; it has no advantages
> over (2) that I can see. I don't have a strong take on whether
> we'd rather have (1) or (2): it's a tradeoff between whether
> we support modelling of short frames vs simplicity of code.
> I'd just like us to be clear about what point or points in
> the code have the responsibility for padding short frames.


I'm not sure how much value we can gain from (1). So (2) looks better to me.

Bin or Philippe, want to send a new version?

Thanks


>
> On
> +    if (sender->info->type == NET_CLIENT_DRIVER_USER ||
> +        sender->info->type == NET_CLIENT_DRIVER_TAP) {
>
> vs
>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> another option would be to add a 'bool pad_short_tx_frames' to
> the NetClientInfo struct, and have those things which don't
> pad set it to true.
>
> thanks
> -- PMM
>

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

Hi Jason,

On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> > On Mon, 8 Mar 2021 at 03:48, Jason Wang <jasowang@redhat.com> wrote:
> >> Do we need to care about other type of networking backends? E.g socket.
> >>
> >> Or at least we should keep the padding logic if we can't audit all of
> >> the backends.
> > I think the key thing we need to do here is make a decision
> > and be clear about what we're doing. There are three options
> > I can see:
> >
> > (1) we say that the net API demands that backends pad
> > packets they emit to the minimum ethernet frame length
> > unless they specifically are intending to emit a short frame,
> > and we fix any backends that don't comply (or equivalently,
> > add support in the core code for a backend to mark itself
> > as "I don't pad; please do it for me").
> >
> > (2) we say that the networking subsystem doesn't support
> > short packets, and just have the common code always enforce
> > padding short frames to the minimum length somewhere between
> > when it receives a packet from a backend and passes it to
> > a NIC model.
> >
> > (3) we say that it's the job of the NIC models to pad
> > short frames as they see them coming in.
> >
> > I think (3) is pretty clearly the worst of these, since it
> > requires every NIC model to handle it; it has no advantages
> > over (2) that I can see. I don't have a strong take on whether
> > we'd rather have (1) or (2): it's a tradeoff between whether
> > we support modelling of short frames vs simplicity of code.
> > I'd just like us to be clear about what point or points in
> > the code have the responsibility for padding short frames.
>
>
> I'm not sure how much value we can gain from (1). So (2) looks better to me.
>
> Bin or Philippe, want to send a new version?
>

I think this series does what (2) asks for. Or am I missing anything?

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/9 4:35 下午, Bin Meng wrote:
> Hi Jason,
>
> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
>>> On Mon, 8 Mar 2021 at 03:48, Jason Wang <jasowang@redhat.com> wrote:
>>>> Do we need to care about other type of networking backends? E.g socket.
>>>>
>>>> Or at least we should keep the padding logic if we can't audit all of
>>>> the backends.
>>> I think the key thing we need to do here is make a decision
>>> and be clear about what we're doing. There are three options
>>> I can see:
>>>
>>> (1) we say that the net API demands that backends pad
>>> packets they emit to the minimum ethernet frame length
>>> unless they specifically are intending to emit a short frame,
>>> and we fix any backends that don't comply (or equivalently,
>>> add support in the core code for a backend to mark itself
>>> as "I don't pad; please do it for me").
>>>
>>> (2) we say that the networking subsystem doesn't support
>>> short packets, and just have the common code always enforce
>>> padding short frames to the minimum length somewhere between
>>> when it receives a packet from a backend and passes it to
>>> a NIC model.
>>>
>>> (3) we say that it's the job of the NIC models to pad
>>> short frames as they see them coming in.
>>>
>>> I think (3) is pretty clearly the worst of these, since it
>>> requires every NIC model to handle it; it has no advantages
>>> over (2) that I can see. I don't have a strong take on whether
>>> we'd rather have (1) or (2): it's a tradeoff between whether
>>> we support modelling of short frames vs simplicity of code.
>>> I'd just like us to be clear about what point or points in
>>> the code have the responsibility for padding short frames.
>>
>> I'm not sure how much value we can gain from (1). So (2) looks better to me.
>>
>> Bin or Philippe, want to send a new version?
>>
> I think this series does what (2) asks for. Or am I missing anything?


It only did the padding for user/TAP.

Thanks


>
> Regards,
> Bin
>

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

Hi Jason,

On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/9 4:35 下午, Bin Meng wrote:
> > Hi Jason,
> >
> > On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> >>> On Mon, 8 Mar 2021 at 03:48, Jason Wang <jasowang@redhat.com> wrote:
> >>>> Do we need to care about other type of networking backends? E.g socket.
> >>>>
> >>>> Or at least we should keep the padding logic if we can't audit all of
> >>>> the backends.
> >>> I think the key thing we need to do here is make a decision
> >>> and be clear about what we're doing. There are three options
> >>> I can see:
> >>>
> >>> (1) we say that the net API demands that backends pad
> >>> packets they emit to the minimum ethernet frame length
> >>> unless they specifically are intending to emit a short frame,
> >>> and we fix any backends that don't comply (or equivalently,
> >>> add support in the core code for a backend to mark itself
> >>> as "I don't pad; please do it for me").
> >>>
> >>> (2) we say that the networking subsystem doesn't support
> >>> short packets, and just have the common code always enforce
> >>> padding short frames to the minimum length somewhere between
> >>> when it receives a packet from a backend and passes it to
> >>> a NIC model.
> >>>
> >>> (3) we say that it's the job of the NIC models to pad
> >>> short frames as they see them coming in.
> >>>
> >>> I think (3) is pretty clearly the worst of these, since it
> >>> requires every NIC model to handle it; it has no advantages
> >>> over (2) that I can see. I don't have a strong take on whether
> >>> we'd rather have (1) or (2): it's a tradeoff between whether
> >>> we support modelling of short frames vs simplicity of code.
> >>> I'd just like us to be clear about what point or points in
> >>> the code have the responsibility for padding short frames.
> >>
> >> I'm not sure how much value we can gain from (1). So (2) looks better to me.
> >>
> >> Bin or Philippe, want to send a new version?
> >>
> > I think this series does what (2) asks for. Or am I missing anything?
>
>
> It only did the padding for user/TAP.

Ah, so we want this:

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

Hi Jason,

On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jason,
>
> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > On 2021/3/9 4:35 下午, Bin Meng wrote:
> > > Hi Jason,
> > >
> > > On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> > >>
> > >> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> > >>> On Mon, 8 Mar 2021 at 03:48, Jason Wang <jasowang@redhat.com> wrote:
> > >>>> Do we need to care about other type of networking backends? E.g socket.
> > >>>>
> > >>>> Or at least we should keep the padding logic if we can't audit all of
> > >>>> the backends.
> > >>> I think the key thing we need to do here is make a decision
> > >>> and be clear about what we're doing. There are three options
> > >>> I can see:
> > >>>
> > >>> (1) we say that the net API demands that backends pad
> > >>> packets they emit to the minimum ethernet frame length
> > >>> unless they specifically are intending to emit a short frame,
> > >>> and we fix any backends that don't comply (or equivalently,
> > >>> add support in the core code for a backend to mark itself
> > >>> as "I don't pad; please do it for me").
> > >>>
> > >>> (2) we say that the networking subsystem doesn't support
> > >>> short packets, and just have the common code always enforce
> > >>> padding short frames to the minimum length somewhere between
> > >>> when it receives a packet from a backend and passes it to
> > >>> a NIC model.
> > >>>
> > >>> (3) we say that it's the job of the NIC models to pad
> > >>> short frames as they see them coming in.
> > >>>
> > >>> I think (3) is pretty clearly the worst of these, since it
> > >>> requires every NIC model to handle it; it has no advantages
> > >>> over (2) that I can see. I don't have a strong take on whether
> > >>> we'd rather have (1) or (2): it's a tradeoff between whether
> > >>> we support modelling of short frames vs simplicity of code.
> > >>> I'd just like us to be clear about what point or points in
> > >>> the code have the responsibility for padding short frames.
> > >>
> > >> I'm not sure how much value we can gain from (1). So (2) looks better to me.
> > >>
> > >> Bin or Philippe, want to send a new version?
> > >>
> > > I think this series does what (2) asks for. Or am I missing anything?
> >
> >
> > It only did the padding for user/TAP.
>

(hit send too soon ...)

Ah, so we want this:

if (sender->info->type != NET_CLIENT_DRIVER_NIC)

correct?

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Peter Maydell]

On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Jason,
>
> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Jason,
> >
> > On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > >
> > > On 2021/3/9 4:35 下午, Bin Meng wrote:
> > > > Hi Jason,
> > > >
> > > > On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >>
> > > >> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> > > >>> I think the key thing we need to do here is make a decision
> > > >>> and be clear about what we're doing. There are three options
> > > >>> I can see:
> > > >>>
> > > >>> (1) we say that the net API demands that backends pad
> > > >>> packets they emit to the minimum ethernet frame length
> > > >>> unless they specifically are intending to emit a short frame,
> > > >>> and we fix any backends that don't comply (or equivalently,
> > > >>> add support in the core code for a backend to mark itself
> > > >>> as "I don't pad; please do it for me").
> > > >>>
> > > >>> (2) we say that the networking subsystem doesn't support
> > > >>> short packets, and just have the common code always enforce
> > > >>> padding short frames to the minimum length somewhere between
> > > >>> when it receives a packet from a backend and passes it to
> > > >>> a NIC model.
> > > >>>
> > > >>> (3) we say that it's the job of the NIC models to pad
> > > >>> short frames as they see them coming in.

> > > >> I'm not sure how much value we can gain from (1). So (2) looks better to me.
> > > >>
> > > >> Bin or Philippe, want to send a new version?
> > > >>
> > > > I think this series does what (2) asks for. Or am I missing anything?
> > >
> > >
> > > It only did the padding for user/TAP.
> >
>
> (hit send too soon ...)
>
> Ah, so we want this:
>
> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
>
> correct?

No, option (2) is "always pad short packets regardless of
sender->info->type". Even if a NIC driver sends out a short
packet, we want to pad it, because we might be feeding it to
something that assumes it does not see short packets.

thanks
-- PMM

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

Hi Peter,

On Tue, Mar 9, 2021 at 6:13 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Jason,
> >
> > On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Jason,
> > >
> > > On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > > On 2021/3/9 4:35 下午, Bin Meng wrote:
> > > > > Hi Jason,
> > > > >
> > > > > On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >>
> > > > >> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> > > > >>> I think the key thing we need to do here is make a decision
> > > > >>> and be clear about what we're doing. There are three options
> > > > >>> I can see:
> > > > >>>
> > > > >>> (1) we say that the net API demands that backends pad
> > > > >>> packets they emit to the minimum ethernet frame length
> > > > >>> unless they specifically are intending to emit a short frame,
> > > > >>> and we fix any backends that don't comply (or equivalently,
> > > > >>> add support in the core code for a backend to mark itself
> > > > >>> as "I don't pad; please do it for me").
> > > > >>>
> > > > >>> (2) we say that the networking subsystem doesn't support
> > > > >>> short packets, and just have the common code always enforce
> > > > >>> padding short frames to the minimum length somewhere between
> > > > >>> when it receives a packet from a backend and passes it to
> > > > >>> a NIC model.
> > > > >>>
> > > > >>> (3) we say that it's the job of the NIC models to pad
> > > > >>> short frames as they see them coming in.
>
> > > > >> I'm not sure how much value we can gain from (1). So (2) looks better to me.
> > > > >>
> > > > >> Bin or Philippe, want to send a new version?
> > > > >>
> > > > > I think this series does what (2) asks for. Or am I missing anything?
> > > >
> > > >
> > > > It only did the padding for user/TAP.
> > >
> >
> > (hit send too soon ...)
> >
> > Ah, so we want this:
> >
> > if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> >
> > correct?
>
> No, option (2) is "always pad short packets regardless of
> sender->info->type". Even if a NIC driver sends out a short
> packet, we want to pad it, because we might be feeding it to
> something that assumes it does not see short packets.

Then this is v1:
http://patchwork.ozlabs.org/project/qemu-devel/patch/1614333786-74258-2-git-send-email-bmeng.cn@gmail.com/

If yes, I will respin v3.

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Yan Vugenfirer]

[-- Attachment #1: Type: text/plain, Size: 2657 bytes --]



> On 9 Mar 2021, at 12:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com <mailto:bmeng.cn@gmail.com>> wrote:
>> 
>> Hi Jason,
>> 
>> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>> 
>>> Hi Jason,
>>> 
>>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
>>>> 
>>>> 
>>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
>>>>> Hi Jason,
>>>>> 
>>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> 
>>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
>>>>>>> I think the key thing we need to do here is make a decision
>>>>>>> and be clear about what we're doing. There are three options
>>>>>>> I can see:
>>>>>>> 
>>>>>>> (1) we say that the net API demands that backends pad
>>>>>>> packets they emit to the minimum ethernet frame length
>>>>>>> unless they specifically are intending to emit a short frame,
>>>>>>> and we fix any backends that don't comply (or equivalently,
>>>>>>> add support in the core code for a backend to mark itself
>>>>>>> as "I don't pad; please do it for me").
>>>>>>> 
>>>>>>> (2) we say that the networking subsystem doesn't support
>>>>>>> short packets, and just have the common code always enforce
>>>>>>> padding short frames to the minimum length somewhere between
>>>>>>> when it receives a packet from a backend and passes it to
>>>>>>> a NIC model.
>>>>>>> 
>>>>>>> (3) we say that it's the job of the NIC models to pad
>>>>>>> short frames as they see them coming in.
> 
>>>>>> I'm not sure how much value we can gain from (1). So (2) looks better to me.
>>>>>> 
>>>>>> Bin or Philippe, want to send a new version?
>>>>>> 
>>>>> I think this series does what (2) asks for. Or am I missing anything?
>>>> 
>>>> 
>>>> It only did the padding for user/TAP.
>>> 
>> 
>> (hit send too soon ...)
>> 
>> Ah, so we want this:
>> 
>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
>> 
>> correct?
> 
> No, option (2) is "always pad short packets regardless of
> sender->info->type". Even if a NIC driver sends out a short
> packet, we want to pad it, because we might be feeding it to
> something that assumes it does not see short packets.

Some thought on this option - in such case with virtio-net, can we also get an indication from the device that the packet will be padded?
Currently we are padding short packets in Windows driver (this is MS certification requirement), and it will be nice not do to this in the guest if device will announce such capability.

Best regards,
Yan.

> 
> thanks
> -- PMM


[-- Attachment #2: Type: text/html, Size: 11349 bytes --]

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Tue, Mar 9, 2021 at 8:30 PM Yan Vugenfirer <yvugenfi@redhat.com> wrote:
>
>
>
> On 9 Mar 2021, at 12:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
>
>
> Hi Jason,
>
> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
>
> Hi Jason,
>
> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2021/3/9 4:35 下午, Bin Meng wrote:
>
> Hi Jason,
>
> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/8 6:22 下午, Peter Maydell wrote:
>
> I think the key thing we need to do here is make a decision
> and be clear about what we're doing. There are three options
> I can see:
>
> (1) we say that the net API demands that backends pad
> packets they emit to the minimum ethernet frame length
> unless they specifically are intending to emit a short frame,
> and we fix any backends that don't comply (or equivalently,
> add support in the core code for a backend to mark itself
> as "I don't pad; please do it for me").
>
> (2) we say that the networking subsystem doesn't support
> short packets, and just have the common code always enforce
> padding short frames to the minimum length somewhere between
> when it receives a packet from a backend and passes it to
> a NIC model.
>
> (3) we say that it's the job of the NIC models to pad
> short frames as they see them coming in.
>
>
> I'm not sure how much value we can gain from (1). So (2) looks better to me.
>
> Bin or Philippe, want to send a new version?
>
> I think this series does what (2) asks for. Or am I missing anything?
>
>
>
> It only did the padding for user/TAP.
>
>
>
> (hit send too soon ...)
>
> Ah, so we want this:
>
> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
>
> correct?
>
>
> No, option (2) is "always pad short packets regardless of
> sender->info->type". Even if a NIC driver sends out a short
> packet, we want to pad it, because we might be feeding it to
> something that assumes it does not see short packets.
>
>
> Some thought on this option - in such case with virtio-net, can we also get an indication from the device that the packet will be padded?
> Currently we are padding short packets in Windows driver (this is MS certification requirement), and it will be nice not do to this in the guest if device will announce such capability.
>

This is more of a virtio-net specification question. virtio-net should
expose a register bit to control this behavior, just like a real world
NIC does.

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/9 6:13 下午, Peter Maydell wrote:
> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Jason,
>>
>> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Jason,
>>>
>>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
>>>>> Hi Jason,
>>>>>
>>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
>>>>>>> I think the key thing we need to do here is make a decision
>>>>>>> and be clear about what we're doing. There are three options
>>>>>>> I can see:
>>>>>>>
>>>>>>> (1) we say that the net API demands that backends pad
>>>>>>> packets they emit to the minimum ethernet frame length
>>>>>>> unless they specifically are intending to emit a short frame,
>>>>>>> and we fix any backends that don't comply (or equivalently,
>>>>>>> add support in the core code for a backend to mark itself
>>>>>>> as "I don't pad; please do it for me").
>>>>>>>
>>>>>>> (2) we say that the networking subsystem doesn't support
>>>>>>> short packets, and just have the common code always enforce
>>>>>>> padding short frames to the minimum length somewhere between
>>>>>>> when it receives a packet from a backend and passes it to
>>>>>>> a NIC model.
>>>>>>>
>>>>>>> (3) we say that it's the job of the NIC models to pad
>>>>>>> short frames as they see them coming in.
>>>>>> I'm not sure how much value we can gain from (1). So (2) looks better to me.
>>>>>>
>>>>>> Bin or Philippe, want to send a new version?
>>>>>>
>>>>> I think this series does what (2) asks for. Or am I missing anything?
>>>>
>>>> It only did the padding for user/TAP.
>> (hit send too soon ...)
>>
>> Ah, so we want this:
>>
>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
>>
>> correct?
> No, option (2) is "always pad short packets regardless of
> sender->info->type". Even if a NIC driver sends out a short
> packet, we want to pad it, because we might be feeding it to
> something that assumes it does not see short packets.
>
> thanks
> -- PMM


So I'm not sure this is correct. There're NIC that has its own logic 
that choose whether to pad the frame during TX (e.g e1000).

And after a discussion 10 years ago [1]. Michael (cced) seems to want to 
keep the padding logic in the NIC itself (probably with a generic helper 
in the core). Since 1) the padding is only required for ethernet 2) 
virito-net doesn't need that (it can pass incomplete packet by design).

Thanks

[1] 
https://patchwork.ozlabs.org/project/qemu-devel/patch/1284842625-13920-1-git-send-email-stefanha@linux.vnet.ibm.com/


>

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Thu, Mar 11, 2021 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/9 6:13 下午, Peter Maydell wrote:
> > On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> >> Hi Jason,
> >>
> >> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>> Hi Jason,
> >>>
> >>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>
> >>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
> >>>>> Hi Jason,
> >>>>>
> >>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> >>>>>>> I think the key thing we need to do here is make a decision
> >>>>>>> and be clear about what we're doing. There are three options
> >>>>>>> I can see:
> >>>>>>>
> >>>>>>> (1) we say that the net API demands that backends pad
> >>>>>>> packets they emit to the minimum ethernet frame length
> >>>>>>> unless they specifically are intending to emit a short frame,
> >>>>>>> and we fix any backends that don't comply (or equivalently,
> >>>>>>> add support in the core code for a backend to mark itself
> >>>>>>> as "I don't pad; please do it for me").
> >>>>>>>
> >>>>>>> (2) we say that the networking subsystem doesn't support
> >>>>>>> short packets, and just have the common code always enforce
> >>>>>>> padding short frames to the minimum length somewhere between
> >>>>>>> when it receives a packet from a backend and passes it to
> >>>>>>> a NIC model.
> >>>>>>>
> >>>>>>> (3) we say that it's the job of the NIC models to pad
> >>>>>>> short frames as they see them coming in.
> >>>>>> I'm not sure how much value we can gain from (1). So (2) looks better to me.
> >>>>>>
> >>>>>> Bin or Philippe, want to send a new version?
> >>>>>>
> >>>>> I think this series does what (2) asks for. Or am I missing anything?
> >>>>
> >>>> It only did the padding for user/TAP.
> >> (hit send too soon ...)
> >>
> >> Ah, so we want this:
> >>
> >> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> >>
> >> correct?
> > No, option (2) is "always pad short packets regardless of
> > sender->info->type". Even if a NIC driver sends out a short
> > packet, we want to pad it, because we might be feeding it to
> > something that assumes it does not see short packets.
> >
> > thanks
> > -- PMM
>
>
> So I'm not sure this is correct. There're NIC that has its own logic
> that choose whether to pad the frame during TX (e.g e1000).

Yes, that's why I mentioned in v2's cover letter that we should
probably only do the padding for SLiRP and TAP. For NIC models, we can
still support sending short frames in QEMU.

>
> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> keep the padding logic in the NIC itself (probably with a generic helper
> in the core). Since 1) the padding is only required for ethernet 2)
> virito-net doesn't need that (it can pass incomplete packet by design).
>

I did read this discussion before working on this patch series.
Providing a helper for NICs to call does NOT fix the issue for SLiRP
and TAP.

> Thanks
>
> [1]
> https://patchwork.ozlabs.org/project/qemu-devel/patch/1284842625-13920-1-git-send-email-stefanha@linux.vnet.ibm.com/
>

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/11 11:12 上午, Bin Meng wrote:
> On Thu, Mar 11, 2021 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/3/9 6:13 下午, Peter Maydell wrote:
>>> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Jason,
>>>>
>>>> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> Hi Jason,
>>>>>
>>>>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
>>>>>>> Hi Jason,
>>>>>>>
>>>>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
>>>>>>>>> I think the key thing we need to do here is make a decision
>>>>>>>>> and be clear about what we're doing. There are three options
>>>>>>>>> I can see:
>>>>>>>>>
>>>>>>>>> (1) we say that the net API demands that backends pad
>>>>>>>>> packets they emit to the minimum ethernet frame length
>>>>>>>>> unless they specifically are intending to emit a short frame,
>>>>>>>>> and we fix any backends that don't comply (or equivalently,
>>>>>>>>> add support in the core code for a backend to mark itself
>>>>>>>>> as "I don't pad; please do it for me").
>>>>>>>>>
>>>>>>>>> (2) we say that the networking subsystem doesn't support
>>>>>>>>> short packets, and just have the common code always enforce
>>>>>>>>> padding short frames to the minimum length somewhere between
>>>>>>>>> when it receives a packet from a backend and passes it to
>>>>>>>>> a NIC model.
>>>>>>>>>
>>>>>>>>> (3) we say that it's the job of the NIC models to pad
>>>>>>>>> short frames as they see them coming in.
>>>>>>>> I'm not sure how much value we can gain from (1). So (2) looks better to me.
>>>>>>>>
>>>>>>>> Bin or Philippe, want to send a new version?
>>>>>>>>
>>>>>>> I think this series does what (2) asks for. Or am I missing anything?
>>>>>> It only did the padding for user/TAP.
>>>> (hit send too soon ...)
>>>>
>>>> Ah, so we want this:
>>>>
>>>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
>>>>
>>>> correct?
>>> No, option (2) is "always pad short packets regardless of
>>> sender->info->type". Even if a NIC driver sends out a short
>>> packet, we want to pad it, because we might be feeding it to
>>> something that assumes it does not see short packets.
>>>
>>> thanks
>>> -- PMM
>>
>> So I'm not sure this is correct. There're NIC that has its own logic
>> that choose whether to pad the frame during TX (e.g e1000).
> Yes, that's why I mentioned in v2's cover letter that we should
> probably only do the padding for SLiRP and TAP.


Actually it's a partail implementation of Peter's method 1. If we go 
that way, you need to make sure the packet is padded for every ethernet 
backend not just TAP and SLIRP.


>   For NIC models, we can
> still support sending short frames in QEMU.


Then it will be padded as well.


>
>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
>> keep the padding logic in the NIC itself (probably with a generic helper
>> in the core). Since 1) the padding is only required for ethernet 2)
>> virito-net doesn't need that (it can pass incomplete packet by design).
>>
> I did read this discussion before working on this patch series.
> Providing a helper for NICs to call does NOT fix the issue for SLiRP
> and TAP.


I'm not sure I get here.

For TX, the padding is controlled by the guest driver. So we just need 
to emulate what real NIC did, pad only when it was required explicitly 
by the driver.

For RX, if we receive short frames from an ethternet backend, we simply 
pad them to make sure it won't be dropped by the NIC model.

So we're actually fine here?

Thanks


>
>> Thanks
>>
>> [1]
>> https://patchwork.ozlabs.org/project/qemu-devel/patch/1284842625-13920-1-git-send-email-stefanha@linux.vnet.ibm.com/
>>
> Regards,
> Bin
>

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Thu, Mar 11, 2021 at 11:33 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/11 11:12 上午, Bin Meng wrote:
> > On Thu, Mar 11, 2021 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2021/3/9 6:13 下午, Peter Maydell wrote:
> >>> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>> Hi Jason,
> >>>>
> >>>> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>>> Hi Jason,
> >>>>>
> >>>>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
> >>>>>>> Hi Jason,
> >>>>>>>
> >>>>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> >>>>>>>>> I think the key thing we need to do here is make a decision
> >>>>>>>>> and be clear about what we're doing. There are three options
> >>>>>>>>> I can see:
> >>>>>>>>>
> >>>>>>>>> (1) we say that the net API demands that backends pad
> >>>>>>>>> packets they emit to the minimum ethernet frame length
> >>>>>>>>> unless they specifically are intending to emit a short frame,
> >>>>>>>>> and we fix any backends that don't comply (or equivalently,
> >>>>>>>>> add support in the core code for a backend to mark itself
> >>>>>>>>> as "I don't pad; please do it for me").
> >>>>>>>>>
> >>>>>>>>> (2) we say that the networking subsystem doesn't support
> >>>>>>>>> short packets, and just have the common code always enforce
> >>>>>>>>> padding short frames to the minimum length somewhere between
> >>>>>>>>> when it receives a packet from a backend and passes it to
> >>>>>>>>> a NIC model.
> >>>>>>>>>
> >>>>>>>>> (3) we say that it's the job of the NIC models to pad
> >>>>>>>>> short frames as they see them coming in.
> >>>>>>>> I'm not sure how much value we can gain from (1). So (2) looks better to me.
> >>>>>>>>
> >>>>>>>> Bin or Philippe, want to send a new version?
> >>>>>>>>
> >>>>>>> I think this series does what (2) asks for. Or am I missing anything?
> >>>>>> It only did the padding for user/TAP.
> >>>> (hit send too soon ...)
> >>>>
> >>>> Ah, so we want this:
> >>>>
> >>>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> >>>>
> >>>> correct?
> >>> No, option (2) is "always pad short packets regardless of
> >>> sender->info->type". Even if a NIC driver sends out a short
> >>> packet, we want to pad it, because we might be feeding it to
> >>> something that assumes it does not see short packets.
> >>>
> >>> thanks
> >>> -- PMM
> >>
> >> So I'm not sure this is correct. There're NIC that has its own logic
> >> that choose whether to pad the frame during TX (e.g e1000).
> > Yes, that's why I mentioned in v2's cover letter that we should
> > probably only do the padding for SLiRP and TAP.
>
>
> Actually it's a partail implementation of Peter's method 1. If we go
> that way, you need to make sure the packet is padded for every ethernet
> backend not just TAP and SLIRP.
>

This latest version series implemented the method 1, except for
providing a flag to indicate when NIC can send short frames and not
pad in the network codes.

>
> >   For NIC models, we can
> > still support sending short frames in QEMU.
>
>
> Then it will be padded as well.
>
>
> >
> >> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> >> keep the padding logic in the NIC itself (probably with a generic helper
> >> in the core). Since 1) the padding is only required for ethernet 2)
> >> virito-net doesn't need that (it can pass incomplete packet by design).
> >>
> > I did read this discussion before working on this patch series.
> > Providing a helper for NICs to call does NOT fix the issue for SLiRP
> > and TAP.
>
>
> I'm not sure I get here.
>
> For TX, the padding is controlled by the guest driver. So we just need
> to emulate what real NIC did, pad only when it was required explicitly
> by the driver.

Correct, version 2 of the RFC series do allow NIC models to send short
frames, but the latest version changed to pad short frames for every
network backends including NICs.

>
> For RX, if we receive short frames from an ethternet backend, we simply
> pad them to make sure it won't be dropped by the NIC model.
>

No, we don't do this. Hardware NICs don't do such. They either simply
drop the short frames, or receive it as it is.

> So we're actually fine here?
>

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Peter Maydell]

On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/9 6:13 下午, Peter Maydell wrote:
> > On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> >> Ah, so we want this:
> >>
> >> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> >>
> >> correct?

> > No, option (2) is "always pad short packets regardless of
> > sender->info->type". Even if a NIC driver sends out a short
> > packet, we want to pad it, because we might be feeding it to
> > something that assumes it does not see short packets.
>
> So I'm not sure this is correct. There're NIC that has its own logic
> that choose whether to pad the frame during TX (e.g e1000).

Yes; that would be dead-code if we go for "always pad", the same
as the code in devices to handle incoming short packets would also
be dead.

> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> keep the padding logic in the NIC itself (probably with a generic helper
> in the core). Since 1) the padding is only required for ethernet 2)
> virito-net doesn't need that (it can pass incomplete packet by design).

Like I said, we need to decide; either:

 (1) we do want to support short packets in the net core:
every sender needs to either pad, or to have some flag to say
"my implementation can't pad, please can the net core do it for me",
unless they are deliberately sending a short packet. Every
receiver needs to be able to cope with short packets, at least
in the sense of not crashing (they should report them as a rx
error if they have that kind of error reporting status register).
I think we have mostly implemented our NIC models this way.

 (2) we simply don't support short packets in the net core:
nobody (not NICs, not network backends) needs to pad, because
they can rely on the core to do it. Some existing senders and
receivers may have now-dead code to do their own padding which
could be removed.

MST is advocating for (1) in that old thread. That's a coherent
position. You've said earlier that we want (2). That's also
a coherent position. A mix of the two doesn't seem to
me to be very workable.

thanks
-- PMM

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > On 2021/3/9 6:13 下午, Peter Maydell wrote:
> > > On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >> Ah, so we want this:
> > >>
> > >> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> > >>
> > >> correct?
>
> > > No, option (2) is "always pad short packets regardless of
> > > sender->info->type". Even if a NIC driver sends out a short
> > > packet, we want to pad it, because we might be feeding it to
> > > something that assumes it does not see short packets.
> >
> > So I'm not sure this is correct. There're NIC that has its own logic
> > that choose whether to pad the frame during TX (e.g e1000).
>
> Yes; that would be dead-code if we go for "always pad", the same
> as the code in devices to handle incoming short packets would also
> be dead.
>
> > And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> > keep the padding logic in the NIC itself (probably with a generic helper
> > in the core). Since 1) the padding is only required for ethernet 2)
> > virito-net doesn't need that (it can pass incomplete packet by design).
>
> Like I said, we need to decide; either:
>
>  (1) we do want to support short packets in the net core:
> every sender needs to either pad, or to have some flag to say
> "my implementation can't pad, please can the net core do it for me",
> unless they are deliberately sending a short packet. Every
> receiver needs to be able to cope with short packets, at least
> in the sense of not crashing (they should report them as a rx
> error if they have that kind of error reporting status register).
> I think we have mostly implemented our NIC models this way.
>
>  (2) we simply don't support short packets in the net core:
> nobody (not NICs, not network backends) needs to pad, because
> they can rely on the core to do it. Some existing senders and
> receivers may have now-dead code to do their own padding which
> could be removed.
>
> MST is advocating for (1) in that old thread. That's a coherent
> position.

But it's a wrong approach. As Edgar and Stefan also said in the old
discussion thread, padding in the RX is wrong as real world NICs don't
do this.

> You've said earlier that we want (2). That's also
> a coherent position. A mix of the two doesn't seem to
> me to be very workable.

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Peter Maydell]

On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
> > > And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> > > keep the padding logic in the NIC itself (probably with a generic helper
> > > in the core). Since 1) the padding is only required for ethernet 2)
> > > virito-net doesn't need that (it can pass incomplete packet by design).
> >
> > Like I said, we need to decide; either:
> >
> >  (1) we do want to support short packets in the net core:
> > every sender needs to either pad, or to have some flag to say
> > "my implementation can't pad, please can the net core do it for me",
> > unless they are deliberately sending a short packet. Every
> > receiver needs to be able to cope with short packets, at least
> > in the sense of not crashing (they should report them as a rx
> > error if they have that kind of error reporting status register).
> > I think we have mostly implemented our NIC models this way.
> >
> >  (2) we simply don't support short packets in the net core:
> > nobody (not NICs, not network backends) needs to pad, because
> > they can rely on the core to do it. Some existing senders and
> > receivers may have now-dead code to do their own padding which
> > could be removed.
> >
> > MST is advocating for (1) in that old thread. That's a coherent
> > position.
>
> But it's a wrong approach. As Edgar and Stefan also said in the old
> discussion thread, padding in the RX is wrong as real world NICs don't
> do this.

Neither option (1) nor option (2) involve padding in RX.

Option (1) is:
 * no NIC implementation pads on TX, except as defined
   by whatever NIC-specific config registers or h/w behaviour
   might require (ie if the guest wants to send a short packet
   it can do that)
 * non-NIC sources like slirp need to pad on TX unless they're
   deliberately trying to send a short packet
 * all receivers of packets need to cope with being given a
   short packet; this is usually going to mean "flag it to the
   guest as an RX error", but exact behaviour is NIC-dependent

Option (2) is:
 * the net core code pads any packet that goes through it
 * no NIC implementation needs to pad on TX (it is harmless if they do)
 * non-NIC sources don't need to pad on TX
 * no receivers of packets need to cope with being given short packets

Option 1 is what the real world does. Option 2 is a simplification
which throws away the ability to emulate handling of short packets,
in exchange for not having to sort out senders like slirp and not
having to be so careful about short-packet handling in NIC models.

If MST is correct that some use cases require short-packet support,
then we need to go for option 1, I think.

thanks
-- PMM

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
> > > > And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> > > > keep the padding logic in the NIC itself (probably with a generic helper
> > > > in the core). Since 1) the padding is only required for ethernet 2)
> > > > virito-net doesn't need that (it can pass incomplete packet by design).
> > >
> > > Like I said, we need to decide; either:
> > >
> > >  (1) we do want to support short packets in the net core:
> > > every sender needs to either pad, or to have some flag to say
> > > "my implementation can't pad, please can the net core do it for me",
> > > unless they are deliberately sending a short packet. Every
> > > receiver needs to be able to cope with short packets, at least
> > > in the sense of not crashing (they should report them as a rx
> > > error if they have that kind of error reporting status register).
> > > I think we have mostly implemented our NIC models this way.
> > >
> > >  (2) we simply don't support short packets in the net core:
> > > nobody (not NICs, not network backends) needs to pad, because
> > > they can rely on the core to do it. Some existing senders and
> > > receivers may have now-dead code to do their own padding which
> > > could be removed.
> > >
> > > MST is advocating for (1) in that old thread. That's a coherent
> > > position.
> >
> > But it's a wrong approach. As Edgar and Stefan also said in the old
> > discussion thread, padding in the RX is wrong as real world NICs don't
> > do this.
>
> Neither option (1) nor option (2) involve padding in RX.

Correct. What I referred to is the current approach used in many NIC
modes, which is wrong, and we have to correct this.

>
> Option (1) is:
>  * no NIC implementation pads on TX, except as defined
>    by whatever NIC-specific config registers or h/w behaviour
>    might require (ie if the guest wants to send a short packet
>    it can do that)
>  * non-NIC sources like slirp need to pad on TX unless they're
>    deliberately trying to send a short packet
>  * all receivers of packets need to cope with being given a
>    short packet; this is usually going to mean "flag it to the
>    guest as an RX error", but exact behaviour is NIC-dependent
>

My patch series in RFC v2/v3 does almost exactly this option (1),
except "flag it to the guest as an RX error".

> Option (2) is:
>  * the net core code pads any packet that goes through it
>  * no NIC implementation needs to pad on TX (it is harmless if they do)
>  * non-NIC sources don't need to pad on TX
>  * no receivers of packets need to cope with being given short packets
>
> Option 1 is what the real world does. Option 2 is a simplification
> which throws away the ability to emulate handling of short packets,
> in exchange for not having to sort out senders like slirp and not
> having to be so careful about short-packet handling in NIC models.
>
> If MST is correct that some use cases require short-packet support,
> then we need to go for option 1, I think.

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/11 6:27 下午, Bin Meng wrote:
> On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
>>>>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
>>>>> keep the padding logic in the NIC itself (probably with a generic helper
>>>>> in the core). Since 1) the padding is only required for ethernet 2)
>>>>> virito-net doesn't need that (it can pass incomplete packet by design).
>>>> Like I said, we need to decide; either:
>>>>
>>>>   (1) we do want to support short packets in the net core:
>>>> every sender needs to either pad, or to have some flag to say
>>>> "my implementation can't pad, please can the net core do it for me",
>>>> unless they are deliberately sending a short packet. Every
>>>> receiver needs to be able to cope with short packets, at least
>>>> in the sense of not crashing (they should report them as a rx
>>>> error if they have that kind of error reporting status register).
>>>> I think we have mostly implemented our NIC models this way.
>>>>
>>>>   (2) we simply don't support short packets in the net core:
>>>> nobody (not NICs, not network backends) needs to pad, because
>>>> they can rely on the core to do it. Some existing senders and
>>>> receivers may have now-dead code to do their own padding which
>>>> could be removed.
>>>>
>>>> MST is advocating for (1) in that old thread. That's a coherent
>>>> position.
>>> But it's a wrong approach. As Edgar and Stefan also said in the old
>>> discussion thread, padding in the RX is wrong as real world NICs don't
>>> do this.
>> Neither option (1) nor option (2) involve padding in RX.
> Correct. What I referred to is the current approach used in many NIC
> modes, which is wrong, and we have to correct this.
>
>> Option (1) is:
>>   * no NIC implementation pads on TX, except as defined
>>     by whatever NIC-specific config registers or h/w behaviour
>>     might require (ie if the guest wants to send a short packet
>>     it can do that)
>>   * non-NIC sources like slirp need to pad on TX unless they're
>>     deliberately trying to send a short packet
>>   * all receivers of packets need to cope with being given a
>>     short packet; this is usually going to mean "flag it to the
>>     guest as an RX error", but exact behaviour is NIC-dependent
>>
> My patch series in RFC v2/v3 does almost exactly this option (1),
> except "flag it to the guest as an RX error".


Is it? You did it at net core instead of netdevs if I read the code 
correctly.


>
>> Option (2) is:
>>   * the net core code pads any packet that goes through it
>>   * no NIC implementation needs to pad on TX (it is harmless if they do)
>>   * non-NIC sources don't need to pad on TX
>>   * no receivers of packets need to cope with being given short packets
>>
>> Option 1 is what the real world does. Option 2 is a simplification
>> which throws away the ability to emulate handling of short packets,
>> in exchange for not having to sort out senders like slirp and not
>> having to be so careful about short-packet handling in NIC models.
>>
>> If MST is correct that some use cases require short-packet support,
>> then we need to go for option 1, I think.


For NIC RX, I wonder whether we can introduce a boolean to whether it 
requries padding. And then the netdevs can only do the padding when it's 
required. E.g virito-net doesn't need padding.

Thanks


> Regards,
> Bin
>

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]


On 2021/3/9 8:30 下午, Yan Vugenfirer wrote:
>
>
>> On 9 Mar 2021, at 12:13 PM, Peter Maydell <peter.maydell@linaro.org 
>> <mailto:peter.maydell@linaro.org>> wrote:
>>
>> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com 
>> <mailto:bmeng.cn@gmail.com>> wrote:
>>>
>>> Hi Jason,
>>>
>>> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com 
>>> <mailto:bmeng.cn@gmail.com>> wrote:
>>>>
>>>> Hi Jason,
>>>>
>>>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com 
>>>> <mailto:jasowang@redhat.com>> wrote:
>>>>>
>>>>>
>>>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
>>>>>> Hi Jason,
>>>>>>
>>>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com 
>>>>>> <mailto:jasowang@redhat.com>> wrote:
>>>>>>>
>>>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
>>>>>>>> I think the key thing we need to do here is make a decision
>>>>>>>> and be clear about what we're doing. There are three options
>>>>>>>> I can see:
>>>>>>>>
>>>>>>>> (1) we say that the net API demands that backends pad
>>>>>>>> packets they emit to the minimum ethernet frame length
>>>>>>>> unless they specifically are intending to emit a short frame,
>>>>>>>> and we fix any backends that don't comply (or equivalently,
>>>>>>>> add support in the core code for a backend to mark itself
>>>>>>>> as "I don't pad; please do it for me").
>>>>>>>>
>>>>>>>> (2) we say that the networking subsystem doesn't support
>>>>>>>> short packets, and just have the common code always enforce
>>>>>>>> padding short frames to the minimum length somewhere between
>>>>>>>> when it receives a packet from a backend and passes it to
>>>>>>>> a NIC model.
>>>>>>>>
>>>>>>>> (3) we say that it's the job of the NIC models to pad
>>>>>>>> short frames as they see them coming in.
>>
>>>>>>> I'm not sure how much value we can gain from (1). So (2) looks 
>>>>>>> better to me.
>>>>>>>
>>>>>>> Bin or Philippe, want to send a new version?
>>>>>>>
>>>>>> I think this series does what (2) asks for. Or am I missing anything?
>>>>>
>>>>>
>>>>> It only did the padding for user/TAP.
>>>>
>>>
>>> (hit send too soon ...)
>>>
>>> Ah, so we want this:
>>>
>>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
>>>
>>> correct?
>>
>> No, option (2) is "always pad short packets regardless of
>> sender->info->type". Even if a NIC driver sends out a short
>> packet, we want to pad it, because we might be feeding it to
>> something that assumes it does not see short packets.
>
> Some thought on this option - in such case with virtio-net, can we 
> also get an indication from the device that the packet will be padded?
> Currently we are padding short packets in Windows driver (this is MS 
> certification requirement), and it will be nice not do to this in the 
> guest if device will announce such capability.


Just to make sure I understand the requirement.

Is it a way to control padding from the driver or soemthing in the vnet 
header that tells the driver that a packet has been padded?

Thanks


>
> Best regards,
> Yan.
>
>>
>> thanks
>> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 16444 bytes --]

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Fri, Mar 12, 2021 at 2:23 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/11 6:27 下午, Bin Meng wrote:
> > On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
> >>>>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> >>>>> keep the padding logic in the NIC itself (probably with a generic helper
> >>>>> in the core). Since 1) the padding is only required for ethernet 2)
> >>>>> virito-net doesn't need that (it can pass incomplete packet by design).
> >>>> Like I said, we need to decide; either:
> >>>>
> >>>>   (1) we do want to support short packets in the net core:
> >>>> every sender needs to either pad, or to have some flag to say
> >>>> "my implementation can't pad, please can the net core do it for me",
> >>>> unless they are deliberately sending a short packet. Every
> >>>> receiver needs to be able to cope with short packets, at least
> >>>> in the sense of not crashing (they should report them as a rx
> >>>> error if they have that kind of error reporting status register).
> >>>> I think we have mostly implemented our NIC models this way.
> >>>>
> >>>>   (2) we simply don't support short packets in the net core:
> >>>> nobody (not NICs, not network backends) needs to pad, because
> >>>> they can rely on the core to do it. Some existing senders and
> >>>> receivers may have now-dead code to do their own padding which
> >>>> could be removed.
> >>>>
> >>>> MST is advocating for (1) in that old thread. That's a coherent
> >>>> position.
> >>> But it's a wrong approach. As Edgar and Stefan also said in the old
> >>> discussion thread, padding in the RX is wrong as real world NICs don't
> >>> do this.
> >> Neither option (1) nor option (2) involve padding in RX.
> > Correct. What I referred to is the current approach used in many NIC
> > modes, which is wrong, and we have to correct this.
> >
> >> Option (1) is:
> >>   * no NIC implementation pads on TX, except as defined
> >>     by whatever NIC-specific config registers or h/w behaviour
> >>     might require (ie if the guest wants to send a short packet
> >>     it can do that)
> >>   * non-NIC sources like slirp need to pad on TX unless they're
> >>     deliberately trying to send a short packet
> >>   * all receivers of packets need to cope with being given a
> >>     short packet; this is usually going to mean "flag it to the
> >>     guest as an RX error", but exact behaviour is NIC-dependent
> >>
> > My patch series in RFC v2/v3 does almost exactly this option (1),
> > except "flag it to the guest as an RX error".
>
>
> Is it? You did it at net core instead of netdevs if I read the code
> correctly.
>

Literally I don't see Peter requested option (1) to be done in net
core or net devs.

If doing it in netdevs, the following codes need to be duplicated in
both SLiRP and TAP codes.

if (sender->info->type == NET_CLIENT_DRIVER_USER ||
    sender->info->type == NET_CLIENT_DRIVER_TAP) {
    do the short frames padding;
}

>
> >
> >> Option (2) is:
> >>   * the net core code pads any packet that goes through it
> >>   * no NIC implementation needs to pad on TX (it is harmless if they do)
> >>   * non-NIC sources don't need to pad on TX
> >>   * no receivers of packets need to cope with being given short packets
> >>
> >> Option 1 is what the real world does. Option 2 is a simplification
> >> which throws away the ability to emulate handling of short packets,
> >> in exchange for not having to sort out senders like slirp and not
> >> having to be so careful about short-packet handling in NIC models.
> >>
> >> If MST is correct that some use cases require short-packet support,
> >> then we need to go for option 1, I think.
>
>
> For NIC RX, I wonder whether we can introduce a boolean to whether it
> requries padding. And then the netdevs can only do the padding when it's
> required. E.g virito-net doesn't need padding.
>

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/12 2:28 下午, Bin Meng wrote:
> On Fri, Mar 12, 2021 at 2:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/3/11 6:27 下午, Bin Meng wrote:
>>> On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
>>>>>>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
>>>>>>> keep the padding logic in the NIC itself (probably with a generic helper
>>>>>>> in the core). Since 1) the padding is only required for ethernet 2)
>>>>>>> virito-net doesn't need that (it can pass incomplete packet by design).
>>>>>> Like I said, we need to decide; either:
>>>>>>
>>>>>>    (1) we do want to support short packets in the net core:
>>>>>> every sender needs to either pad, or to have some flag to say
>>>>>> "my implementation can't pad, please can the net core do it for me",
>>>>>> unless they are deliberately sending a short packet. Every
>>>>>> receiver needs to be able to cope with short packets, at least
>>>>>> in the sense of not crashing (they should report them as a rx
>>>>>> error if they have that kind of error reporting status register).
>>>>>> I think we have mostly implemented our NIC models this way.
>>>>>>
>>>>>>    (2) we simply don't support short packets in the net core:
>>>>>> nobody (not NICs, not network backends) needs to pad, because
>>>>>> they can rely on the core to do it. Some existing senders and
>>>>>> receivers may have now-dead code to do their own padding which
>>>>>> could be removed.
>>>>>>
>>>>>> MST is advocating for (1) in that old thread. That's a coherent
>>>>>> position.
>>>>> But it's a wrong approach. As Edgar and Stefan also said in the old
>>>>> discussion thread, padding in the RX is wrong as real world NICs don't
>>>>> do this.
>>>> Neither option (1) nor option (2) involve padding in RX.
>>> Correct. What I referred to is the current approach used in many NIC
>>> modes, which is wrong, and we have to correct this.
>>>
>>>> Option (1) is:
>>>>    * no NIC implementation pads on TX, except as defined
>>>>      by whatever NIC-specific config registers or h/w behaviour
>>>>      might require (ie if the guest wants to send a short packet
>>>>      it can do that)
>>>>    * non-NIC sources like slirp need to pad on TX unless they're
>>>>      deliberately trying to send a short packet
>>>>    * all receivers of packets need to cope with being given a
>>>>      short packet; this is usually going to mean "flag it to the
>>>>      guest as an RX error", but exact behaviour is NIC-dependent
>>>>
>>> My patch series in RFC v2/v3 does almost exactly this option (1),
>>> except "flag it to the guest as an RX error".
>>
>> Is it? You did it at net core instead of netdevs if I read the code
>> correctly.
>>
> Literally I don't see Peter requested option (1) to be done in net
> core or net devs.
>
> If doing it in netdevs, the following codes need to be duplicated in
> both SLiRP and TAP codes.
>
> if (sender->info->type == NET_CLIENT_DRIVER_USER ||
>      sender->info->type == NET_CLIENT_DRIVER_TAP) {
>      do the short frames padding;
> }


So my understanding is that it's better to be done at netdev where we 
know whether it's a ethernet dev, core should be protocol independent.

Thanks


>
>>>> Option (2) is:
>>>>    * the net core code pads any packet that goes through it
>>>>    * no NIC implementation needs to pad on TX (it is harmless if they do)
>>>>    * non-NIC sources don't need to pad on TX
>>>>    * no receivers of packets need to cope with being given short packets
>>>>
>>>> Option 1 is what the real world does. Option 2 is a simplification
>>>> which throws away the ability to emulate handling of short packets,
>>>> in exchange for not having to sort out senders like slirp and not
>>>> having to be so careful about short-packet handling in NIC models.
>>>>
>>>> If MST is correct that some use cases require short-packet support,
>>>> then we need to go for option 1, I think.
>>
>> For NIC RX, I wonder whether we can introduce a boolean to whether it
>> requries padding. And then the netdevs can only do the padding when it's
>> required. E.g virito-net doesn't need padding.
>>
> Regards,
> Bin
>

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Fri, Mar 12, 2021 at 2:50 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/12 2:28 下午, Bin Meng wrote:
> > On Fri, Mar 12, 2021 at 2:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2021/3/11 6:27 下午, Bin Meng wrote:
> >>> On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>>> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>>> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
> >>>>>>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> >>>>>>> keep the padding logic in the NIC itself (probably with a generic helper
> >>>>>>> in the core). Since 1) the padding is only required for ethernet 2)
> >>>>>>> virito-net doesn't need that (it can pass incomplete packet by design).
> >>>>>> Like I said, we need to decide; either:
> >>>>>>
> >>>>>>    (1) we do want to support short packets in the net core:
> >>>>>> every sender needs to either pad, or to have some flag to say
> >>>>>> "my implementation can't pad, please can the net core do it for me",
> >>>>>> unless they are deliberately sending a short packet. Every
> >>>>>> receiver needs to be able to cope with short packets, at least
> >>>>>> in the sense of not crashing (they should report them as a rx
> >>>>>> error if they have that kind of error reporting status register).
> >>>>>> I think we have mostly implemented our NIC models this way.
> >>>>>>
> >>>>>>    (2) we simply don't support short packets in the net core:
> >>>>>> nobody (not NICs, not network backends) needs to pad, because
> >>>>>> they can rely on the core to do it. Some existing senders and
> >>>>>> receivers may have now-dead code to do their own padding which
> >>>>>> could be removed.
> >>>>>>
> >>>>>> MST is advocating for (1) in that old thread. That's a coherent
> >>>>>> position.
> >>>>> But it's a wrong approach. As Edgar and Stefan also said in the old
> >>>>> discussion thread, padding in the RX is wrong as real world NICs don't
> >>>>> do this.
> >>>> Neither option (1) nor option (2) involve padding in RX.
> >>> Correct. What I referred to is the current approach used in many NIC
> >>> modes, which is wrong, and we have to correct this.
> >>>
> >>>> Option (1) is:
> >>>>    * no NIC implementation pads on TX, except as defined
> >>>>      by whatever NIC-specific config registers or h/w behaviour
> >>>>      might require (ie if the guest wants to send a short packet
> >>>>      it can do that)
> >>>>    * non-NIC sources like slirp need to pad on TX unless they're
> >>>>      deliberately trying to send a short packet
> >>>>    * all receivers of packets need to cope with being given a
> >>>>      short packet; this is usually going to mean "flag it to the
> >>>>      guest as an RX error", but exact behaviour is NIC-dependent
> >>>>
> >>> My patch series in RFC v2/v3 does almost exactly this option (1),
> >>> except "flag it to the guest as an RX error".
> >>
> >> Is it? You did it at net core instead of netdevs if I read the code
> >> correctly.
> >>
> > Literally I don't see Peter requested option (1) to be done in net
> > core or net devs.
> >
> > If doing it in netdevs, the following codes need to be duplicated in
> > both SLiRP and TAP codes.
> >
> > if (sender->info->type == NET_CLIENT_DRIVER_USER ||
> >      sender->info->type == NET_CLIENT_DRIVER_TAP) {
> >      do the short frames padding;
> > }
>
>
> So my understanding is that it's better to be done at netdev where we
> know whether it's a ethernet dev, core should be protocol independent.

OK, will change to pad short frames in SLiRP and TAP codes in the next version.

Regards,
Bin

Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Jason Wang]

On 2021/3/12 2:53 下午, Bin Meng wrote:
> On Fri, Mar 12, 2021 at 2:50 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/3/12 2:28 下午, Bin Meng wrote:
>>> On Fri, Mar 12, 2021 at 2:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2021/3/11 6:27 下午, Bin Meng wrote:
>>>>> On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>>> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>>> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
>>>>>>>>> keep the padding logic in the NIC itself (probably with a generic helper
>>>>>>>>> in the core). Since 1) the padding is only required for ethernet 2)
>>>>>>>>> virito-net doesn't need that (it can pass incomplete packet by design).
>>>>>>>> Like I said, we need to decide; either:
>>>>>>>>
>>>>>>>>     (1) we do want to support short packets in the net core:
>>>>>>>> every sender needs to either pad, or to have some flag to say
>>>>>>>> "my implementation can't pad, please can the net core do it for me",
>>>>>>>> unless they are deliberately sending a short packet. Every
>>>>>>>> receiver needs to be able to cope with short packets, at least
>>>>>>>> in the sense of not crashing (they should report them as a rx
>>>>>>>> error if they have that kind of error reporting status register).
>>>>>>>> I think we have mostly implemented our NIC models this way.
>>>>>>>>
>>>>>>>>     (2) we simply don't support short packets in the net core:
>>>>>>>> nobody (not NICs, not network backends) needs to pad, because
>>>>>>>> they can rely on the core to do it. Some existing senders and
>>>>>>>> receivers may have now-dead code to do their own padding which
>>>>>>>> could be removed.
>>>>>>>>
>>>>>>>> MST is advocating for (1) in that old thread. That's a coherent
>>>>>>>> position.
>>>>>>> But it's a wrong approach. As Edgar and Stefan also said in the old
>>>>>>> discussion thread, padding in the RX is wrong as real world NICs don't
>>>>>>> do this.
>>>>>> Neither option (1) nor option (2) involve padding in RX.
>>>>> Correct. What I referred to is the current approach used in many NIC
>>>>> modes, which is wrong, and we have to correct this.
>>>>>
>>>>>> Option (1) is:
>>>>>>     * no NIC implementation pads on TX, except as defined
>>>>>>       by whatever NIC-specific config registers or h/w behaviour
>>>>>>       might require (ie if the guest wants to send a short packet
>>>>>>       it can do that)
>>>>>>     * non-NIC sources like slirp need to pad on TX unless they're
>>>>>>       deliberately trying to send a short packet
>>>>>>     * all receivers of packets need to cope with being given a
>>>>>>       short packet; this is usually going to mean "flag it to the
>>>>>>       guest as an RX error", but exact behaviour is NIC-dependent
>>>>>>
>>>>> My patch series in RFC v2/v3 does almost exactly this option (1),
>>>>> except "flag it to the guest as an RX error".
>>>> Is it? You did it at net core instead of netdevs if I read the code
>>>> correctly.
>>>>
>>> Literally I don't see Peter requested option (1) to be done in net
>>> core or net devs.
>>>
>>> If doing it in netdevs, the following codes need to be duplicated in
>>> both SLiRP and TAP codes.
>>>
>>> if (sender->info->type == NET_CLIENT_DRIVER_USER ||
>>>       sender->info->type == NET_CLIENT_DRIVER_TAP) {
>>>       do the short frames padding;
>>> }
>>
>> So my understanding is that it's better to be done at netdev where we
>> know whether it's a ethernet dev, core should be protocol independent.
> OK, will change to pad short frames in SLiRP and TAP codes in the next version.
>
> Regards,
> Bin


And we probably need a need_padding in the NetClientState, and only do 
the padding if it was required by the peer.

Then we can say netdevs and virtio-net doesn't need padding.

Thanks