All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] net: checksum: Skip fragmented IP packets
@ 2020-12-06  2:14 Bin Meng
  2020-12-06  2:14 ` [PATCH 2/3] net: checksum: Add IP header checksum calculation Bin Meng
  2020-12-06  2:14   ` Bin Meng
  0 siblings, 2 replies; 10+ messages in thread
From: Bin Meng @ 2020-12-06  2:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jason Wang, Bin Meng, Markus Carlstedt

From: Markus Carlstedt <markus.carlstedt@windriver.com>

To calculate the TCP/UDP checksum we need the whole datagram. Unless
the hardware has some logic to collect all fragments before sending
the whole datagram first, it can only be done by the network stack,
which is normally the case for the NICs we have seen so far.

Skip these fragmented IP packets to avoid checksum corruption.

Signed-off-by: Markus Carlstedt <markus.carlstedt@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 net/checksum.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/checksum.c b/net/checksum.c
index aaa4000..5cb8b2c 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -106,6 +106,10 @@ void net_checksum_calculate(uint8_t *data, int length)
         return; /* not IPv4 */
     }
 
+    if (IP4_IS_FRAGMENT(ip)) {
+        return; /* a fragmented IP packet */
+    }
+
     ip_len = lduw_be_p(&ip->ip_len);
 
     /* Last, check that we have enough data for the all IP frame */
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 2/3] net: checksum: Add IP header checksum calculation
  2020-12-06  2:14 [PATCH 1/3] net: checksum: Skip fragmented IP packets Bin Meng
@ 2020-12-06  2:14 ` Bin Meng
  2020-12-06  2:14   ` Bin Meng
  1 sibling, 0 replies; 10+ messages in thread
From: Bin Meng @ 2020-12-06  2:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Bin Meng, Yabing Liu, Jason Wang, Guishan Qin

From: Guishan Qin <guishan.qin@windriver.com>

At present net_checksum_calculate() only calculates TCP/UDP checksum
in an IP packet, but assumes the IP header checksum to be provided
by the software, e.g.: Linux kernel always calculates the IP header
checksum. However this might not always be the case, e.g.: for an IP
checksum offload enabled stack like VxWorks, the IP header chcksum
can be zero.

This adds the checksum calculation of the IP header.

Signed-off-by: Guishan Qin <guishan.qin@windriver.com>
Signed-off-by: Yabing Liu <yabing.liu@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 net/checksum.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/checksum.c b/net/checksum.c
index 5cb8b2c..dabd290 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -61,6 +61,7 @@ void net_checksum_calculate(uint8_t *data, int length)
 {
     int mac_hdr_len, ip_len;
     struct ip_header *ip;
+    uint16_t csum;
 
     /*
      * Note: We cannot assume "data" is aligned, so the all code uses
@@ -106,6 +107,11 @@ void net_checksum_calculate(uint8_t *data, int length)
         return; /* not IPv4 */
     }
 
+    /* Calculate IP checksum */
+    stw_he_p(&ip->ip_sum, 0);
+    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
+    stw_be_p(&ip->ip_sum, csum);
+
     if (IP4_IS_FRAGMENT(ip)) {
         return; /* a fragmented IP packet */
     }
@@ -122,7 +128,6 @@ void net_checksum_calculate(uint8_t *data, int length)
     switch (ip->ip_p) {
     case IP_PROTO_TCP:
     {
-        uint16_t csum;
         tcp_header *tcp = (tcp_header *)(ip + 1);
 
         if (ip_len < sizeof(tcp_header)) {
@@ -143,7 +148,6 @@ void net_checksum_calculate(uint8_t *data, int length)
     }
     case IP_PROTO_UDP:
     {
-        uint16_t csum;
         udp_header *udp = (udp_header *)(ip + 1);
 
         if (ip_len < sizeof(udp_header)) {
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 3/3] net: checksum: Introduce fine control over checksum type
  2020-12-06  2:14 [PATCH 1/3] net: checksum: Skip fragmented IP packets Bin Meng
@ 2020-12-06  2:14   ` Bin Meng
  2020-12-06  2:14   ` Bin Meng
  1 sibling, 0 replies; 10+ messages in thread
From: Bin Meng @ 2020-12-06  2:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Alistair Francis, Paul Durrant, Li Zhijian,
	Michael S. Tsirkin, Andrew Jeffery, Jason Wang, Bin Meng,
	Joel Stanley, Beniamino Galvani, Zhang Chen, Stefano Stabellini,
	Peter Chubb, Cédric Le Goater, qemu-arm, xen-devel,
	Anthony Perard, Edgar E. Iglesias, qemu-ppc, David Gibson

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

At present net_checksum_calculate() blindly calculates all types of
checksums (IP, TCP, UDP). Some NICs may have a per type setting in
their BDs to control what checksum should be offloaded. To support
such hardware behavior, introduce a 'csum_flag' parameter to the
net_checksum_calculate() API to allow fine control over what type
checksum is calculated.

Existing users of this API are updated accordingly.

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

---

 hw/net/allwinner-sun8i-emac.c |  2 +-
 hw/net/cadence_gem.c          |  2 +-
 hw/net/fsl_etsec/rings.c      |  8 +++-----
 hw/net/ftgmac100.c            | 10 +++++++++-
 hw/net/imx_fec.c              | 15 +++------------
 hw/net/virtio-net.c           |  2 +-
 hw/net/xen_nic.c              |  2 +-
 include/net/checksum.h        |  7 ++++++-
 net/checksum.c                | 18 ++++++++++++++----
 net/filter-rewriter.c         |  4 ++--
 10 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
index 38d3285..0427689 100644
--- a/hw/net/allwinner-sun8i-emac.c
+++ b/hw/net/allwinner-sun8i-emac.c
@@ -514,7 +514,7 @@ static void allwinner_sun8i_emac_transmit(AwSun8iEmacState *s)
         /* After the last descriptor, send the packet */
         if (desc.status2 & TX_DESC_STATUS2_LAST_DESC) {
             if (desc.status2 & TX_DESC_STATUS2_CHECKSUM_MASK) {
-                net_checksum_calculate(packet_buf, packet_bytes);
+                net_checksum_calculate(packet_buf, packet_bytes, CSUM_ALL);
             }
 
             qemu_send_packet(nc, packet_buf, packet_bytes);
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 7a53469..9a4474a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1266,7 +1266,7 @@ static void gem_transmit(CadenceGEMState *s)
 
                 /* Is checksum offload enabled? */
                 if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
-                    net_checksum_calculate(s->tx_packet, total_bytes);
+                    net_checksum_calculate(s->tx_packet, total_bytes, CSUM_ALL);
                 }
 
                 /* Update MAC statistics */
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index 628648a..2b0afee 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -186,10 +186,8 @@ static void process_tx_fcb(eTSEC *etsec)
 
     /* if packet is IP4 and IP checksum is requested */
     if (flags & FCB_TX_IP && flags & FCB_TX_CIP) {
-        /* do IP4 checksum (TODO This function does TCP/UDP checksum
-         * but not sure if it also does IP4 checksum.) */
         net_checksum_calculate(etsec->tx_buffer + 8,
-                etsec->tx_buffer_len - 8);
+                etsec->tx_buffer_len - 8, CSUM_IP);
     }
     /* TODO Check the correct usage of the PHCS field of the FCB in case the NPH
      * flag is on */
@@ -203,7 +201,7 @@ static void process_tx_fcb(eTSEC *etsec)
                 /* do UDP checksum */
 
                 net_checksum_calculate(etsec->tx_buffer + 8,
-                        etsec->tx_buffer_len - 8);
+                        etsec->tx_buffer_len - 8, CSUM_UDP);
             } else {
                 /* set checksum field to 0 */
                 l4_header[6] = 0;
@@ -212,7 +210,7 @@ static void process_tx_fcb(eTSEC *etsec)
         } else if (flags & FCB_TX_CTU) { /* if TCP and checksum is requested */
             /* do TCP checksum */
             net_checksum_calculate(etsec->tx_buffer + 8,
-                                   etsec->tx_buffer_len - 8);
+                                   etsec->tx_buffer_len - 8, CSUM_TCP);
         }
     }
 }
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 782ff19..fbae1f1 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -573,7 +573,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
             }
 
             if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
-                net_checksum_calculate(s->frame, frame_size);
+                /*
+                 * TODO:
+                 * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
+                 * however previous net_checksum_calculate() did not calculate
+                 * IP checksum at all. Passing CSUM_ALL for now until someone
+                 * who is familar with this MAC to figure out what should be
+                 * properly added for TCP/UDP checksum offload.
+                 */
+                net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
             }
             /* Last buffer in frame.  */
             qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 2c14804..45f96fa 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -562,20 +562,11 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
         frame_size += len;
         if (bd.flags & ENET_BD_L) {
             if (bd.option & ENET_BD_PINS) {
-                struct ip_header *ip_hd = PKT_GET_IP_HDR(s->frame);
-                if (IP_HEADER_VERSION(ip_hd) == 4) {
-                    net_checksum_calculate(s->frame, frame_size);
-                }
+                net_checksum_calculate(s->frame, frame_size,
+                                       CSUM_TCP | CSUM_UDP);
             }
             if (bd.option & ENET_BD_IINS) {
-                struct ip_header *ip_hd = PKT_GET_IP_HDR(s->frame);
-                /* We compute checksum only for IPv4 frames */
-                if (IP_HEADER_VERSION(ip_hd) == 4) {
-                    uint16_t csum;
-                    ip_hd->ip_sum = 0;
-                    csum = net_raw_checksum((uint8_t *)ip_hd, sizeof(*ip_hd));
-                    ip_hd->ip_sum = cpu_to_be16(csum);
-                }
+                net_checksum_calculate(s->frame, frame_size, CSUM_IP);
             }
             /* Last buffer in frame.  */
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9179013..77e9ded 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1482,7 +1482,7 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
         (buf[12] == 0x08 && buf[13] == 0x00) && /* ethertype == IPv4 */
         (buf[23] == 17) && /* ip.protocol == UDP */
         (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
-        net_checksum_calculate(buf, size);
+        net_checksum_calculate(buf, size, CSUM_UDP);
         hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
     }
 }
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 00a7fdf..5c815b4 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -174,7 +174,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
                     tmpbuf = g_malloc(XC_PAGE_SIZE);
                 }
                 memcpy(tmpbuf, page + txreq.offset, txreq.size);
-                net_checksum_calculate(tmpbuf, txreq.size);
+                net_checksum_calculate(tmpbuf, txreq.size, CSUM_ALL);
                 qemu_send_packet(qemu_get_queue(netdev->nic), tmpbuf,
                                  txreq.size);
             } else {
diff --git a/include/net/checksum.h b/include/net/checksum.h
index 05a0d27..7dec37e 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -21,11 +21,16 @@
 #include "qemu/bswap.h"
 struct iovec;
 
+#define CSUM_IP     0x01
+#define CSUM_TCP    0x02
+#define CSUM_UDP    0x04
+#define CSUM_ALL    (CSUM_IP | CSUM_TCP | CSUM_UDP)
+
 uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
 uint16_t net_checksum_finish(uint32_t sum);
 uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
                              uint8_t *addrs, uint8_t *buf);
-void net_checksum_calculate(uint8_t *data, int length);
+void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
 
 static inline uint32_t
 net_checksum_add(int len, uint8_t *buf)
diff --git a/net/checksum.c b/net/checksum.c
index dabd290..70f4eae 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
     return net_checksum_finish(sum);
 }
 
-void net_checksum_calculate(uint8_t *data, int length)
+void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
 {
     int mac_hdr_len, ip_len;
     struct ip_header *ip;
@@ -108,9 +108,11 @@ void net_checksum_calculate(uint8_t *data, int length)
     }
 
     /* Calculate IP checksum */
-    stw_he_p(&ip->ip_sum, 0);
-    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
-    stw_be_p(&ip->ip_sum, csum);
+    if (csum_flag & CSUM_IP) {
+        stw_he_p(&ip->ip_sum, 0);
+        csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
+        stw_be_p(&ip->ip_sum, csum);
+    }
 
     if (IP4_IS_FRAGMENT(ip)) {
         return; /* a fragmented IP packet */
@@ -128,6 +130,10 @@ void net_checksum_calculate(uint8_t *data, int length)
     switch (ip->ip_p) {
     case IP_PROTO_TCP:
     {
+        if (!(csum_flag & CSUM_TCP)) {
+            return;
+        }
+
         tcp_header *tcp = (tcp_header *)(ip + 1);
 
         if (ip_len < sizeof(tcp_header)) {
@@ -148,6 +154,10 @@ void net_checksum_calculate(uint8_t *data, int length)
     }
     case IP_PROTO_UDP:
     {
+        if (!(csum_flag & CSUM_UDP)) {
+            return;
+        }
+
         udp_header *udp = (udp_header *)(ip + 1);
 
         if (ip_len < sizeof(udp_header)) {
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index e063a81..80caac5 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -114,7 +114,7 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
             tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
 
             net_checksum_calculate((uint8_t *)pkt->data + pkt->vnet_hdr_len,
-                                   pkt->size - pkt->vnet_hdr_len);
+                                   pkt->size - pkt->vnet_hdr_len, CSUM_TCP);
         }
 
         /*
@@ -216,7 +216,7 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
             tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
 
             net_checksum_calculate((uint8_t *)pkt->data + pkt->vnet_hdr_len,
-                                   pkt->size - pkt->vnet_hdr_len);
+                                   pkt->size - pkt->vnet_hdr_len, CSUM_TCP);
         }
     }
 
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 3/3] net: checksum: Introduce fine control over checksum type
@ 2020-12-06  2:14   ` Bin Meng
  0 siblings, 0 replies; 10+ messages in thread
From: Bin Meng @ 2020-12-06  2:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Bin Meng, Alistair Francis, Andrew Jeffery, Anthony Perard,
	Beniamino Galvani, Cédric Le Goater, David Gibson,
	Edgar E. Iglesias, Jason Wang, Joel Stanley, Li Zhijian,
	Michael S. Tsirkin, Paul Durrant, Peter Chubb, Peter Maydell,
	Stefano Stabellini, Zhang Chen, qemu-arm, qemu-ppc, xen-devel

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

At present net_checksum_calculate() blindly calculates all types of
checksums (IP, TCP, UDP). Some NICs may have a per type setting in
their BDs to control what checksum should be offloaded. To support
such hardware behavior, introduce a 'csum_flag' parameter to the
net_checksum_calculate() API to allow fine control over what type
checksum is calculated.

Existing users of this API are updated accordingly.

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

---

 hw/net/allwinner-sun8i-emac.c |  2 +-
 hw/net/cadence_gem.c          |  2 +-
 hw/net/fsl_etsec/rings.c      |  8 +++-----
 hw/net/ftgmac100.c            | 10 +++++++++-
 hw/net/imx_fec.c              | 15 +++------------
 hw/net/virtio-net.c           |  2 +-
 hw/net/xen_nic.c              |  2 +-
 include/net/checksum.h        |  7 ++++++-
 net/checksum.c                | 18 ++++++++++++++----
 net/filter-rewriter.c         |  4 ++--
 10 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
index 38d3285..0427689 100644
--- a/hw/net/allwinner-sun8i-emac.c
+++ b/hw/net/allwinner-sun8i-emac.c
@@ -514,7 +514,7 @@ static void allwinner_sun8i_emac_transmit(AwSun8iEmacState *s)
         /* After the last descriptor, send the packet */
         if (desc.status2 & TX_DESC_STATUS2_LAST_DESC) {
             if (desc.status2 & TX_DESC_STATUS2_CHECKSUM_MASK) {
-                net_checksum_calculate(packet_buf, packet_bytes);
+                net_checksum_calculate(packet_buf, packet_bytes, CSUM_ALL);
             }
 
             qemu_send_packet(nc, packet_buf, packet_bytes);
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 7a53469..9a4474a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1266,7 +1266,7 @@ static void gem_transmit(CadenceGEMState *s)
 
                 /* Is checksum offload enabled? */
                 if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
-                    net_checksum_calculate(s->tx_packet, total_bytes);
+                    net_checksum_calculate(s->tx_packet, total_bytes, CSUM_ALL);
                 }
 
                 /* Update MAC statistics */
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index 628648a..2b0afee 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -186,10 +186,8 @@ static void process_tx_fcb(eTSEC *etsec)
 
     /* if packet is IP4 and IP checksum is requested */
     if (flags & FCB_TX_IP && flags & FCB_TX_CIP) {
-        /* do IP4 checksum (TODO This function does TCP/UDP checksum
-         * but not sure if it also does IP4 checksum.) */
         net_checksum_calculate(etsec->tx_buffer + 8,
-                etsec->tx_buffer_len - 8);
+                etsec->tx_buffer_len - 8, CSUM_IP);
     }
     /* TODO Check the correct usage of the PHCS field of the FCB in case the NPH
      * flag is on */
@@ -203,7 +201,7 @@ static void process_tx_fcb(eTSEC *etsec)
                 /* do UDP checksum */
 
                 net_checksum_calculate(etsec->tx_buffer + 8,
-                        etsec->tx_buffer_len - 8);
+                        etsec->tx_buffer_len - 8, CSUM_UDP);
             } else {
                 /* set checksum field to 0 */
                 l4_header[6] = 0;
@@ -212,7 +210,7 @@ static void process_tx_fcb(eTSEC *etsec)
         } else if (flags & FCB_TX_CTU) { /* if TCP and checksum is requested */
             /* do TCP checksum */
             net_checksum_calculate(etsec->tx_buffer + 8,
-                                   etsec->tx_buffer_len - 8);
+                                   etsec->tx_buffer_len - 8, CSUM_TCP);
         }
     }
 }
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 782ff19..fbae1f1 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -573,7 +573,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
             }
 
             if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
-                net_checksum_calculate(s->frame, frame_size);
+                /*
+                 * TODO:
+                 * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
+                 * however previous net_checksum_calculate() did not calculate
+                 * IP checksum at all. Passing CSUM_ALL for now until someone
+                 * who is familar with this MAC to figure out what should be
+                 * properly added for TCP/UDP checksum offload.
+                 */
+                net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
             }
             /* Last buffer in frame.  */
             qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 2c14804..45f96fa 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -562,20 +562,11 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
         frame_size += len;
         if (bd.flags & ENET_BD_L) {
             if (bd.option & ENET_BD_PINS) {
-                struct ip_header *ip_hd = PKT_GET_IP_HDR(s->frame);
-                if (IP_HEADER_VERSION(ip_hd) == 4) {
-                    net_checksum_calculate(s->frame, frame_size);
-                }
+                net_checksum_calculate(s->frame, frame_size,
+                                       CSUM_TCP | CSUM_UDP);
             }
             if (bd.option & ENET_BD_IINS) {
-                struct ip_header *ip_hd = PKT_GET_IP_HDR(s->frame);
-                /* We compute checksum only for IPv4 frames */
-                if (IP_HEADER_VERSION(ip_hd) == 4) {
-                    uint16_t csum;
-                    ip_hd->ip_sum = 0;
-                    csum = net_raw_checksum((uint8_t *)ip_hd, sizeof(*ip_hd));
-                    ip_hd->ip_sum = cpu_to_be16(csum);
-                }
+                net_checksum_calculate(s->frame, frame_size, CSUM_IP);
             }
             /* Last buffer in frame.  */
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9179013..77e9ded 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1482,7 +1482,7 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
         (buf[12] == 0x08 && buf[13] == 0x00) && /* ethertype == IPv4 */
         (buf[23] == 17) && /* ip.protocol == UDP */
         (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
-        net_checksum_calculate(buf, size);
+        net_checksum_calculate(buf, size, CSUM_UDP);
         hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
     }
 }
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 00a7fdf..5c815b4 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -174,7 +174,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
                     tmpbuf = g_malloc(XC_PAGE_SIZE);
                 }
                 memcpy(tmpbuf, page + txreq.offset, txreq.size);
-                net_checksum_calculate(tmpbuf, txreq.size);
+                net_checksum_calculate(tmpbuf, txreq.size, CSUM_ALL);
                 qemu_send_packet(qemu_get_queue(netdev->nic), tmpbuf,
                                  txreq.size);
             } else {
diff --git a/include/net/checksum.h b/include/net/checksum.h
index 05a0d27..7dec37e 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -21,11 +21,16 @@
 #include "qemu/bswap.h"
 struct iovec;
 
+#define CSUM_IP     0x01
+#define CSUM_TCP    0x02
+#define CSUM_UDP    0x04
+#define CSUM_ALL    (CSUM_IP | CSUM_TCP | CSUM_UDP)
+
 uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
 uint16_t net_checksum_finish(uint32_t sum);
 uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
                              uint8_t *addrs, uint8_t *buf);
-void net_checksum_calculate(uint8_t *data, int length);
+void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
 
 static inline uint32_t
 net_checksum_add(int len, uint8_t *buf)
diff --git a/net/checksum.c b/net/checksum.c
index dabd290..70f4eae 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
     return net_checksum_finish(sum);
 }
 
-void net_checksum_calculate(uint8_t *data, int length)
+void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
 {
     int mac_hdr_len, ip_len;
     struct ip_header *ip;
@@ -108,9 +108,11 @@ void net_checksum_calculate(uint8_t *data, int length)
     }
 
     /* Calculate IP checksum */
-    stw_he_p(&ip->ip_sum, 0);
-    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
-    stw_be_p(&ip->ip_sum, csum);
+    if (csum_flag & CSUM_IP) {
+        stw_he_p(&ip->ip_sum, 0);
+        csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
+        stw_be_p(&ip->ip_sum, csum);
+    }
 
     if (IP4_IS_FRAGMENT(ip)) {
         return; /* a fragmented IP packet */
@@ -128,6 +130,10 @@ void net_checksum_calculate(uint8_t *data, int length)
     switch (ip->ip_p) {
     case IP_PROTO_TCP:
     {
+        if (!(csum_flag & CSUM_TCP)) {
+            return;
+        }
+
         tcp_header *tcp = (tcp_header *)(ip + 1);
 
         if (ip_len < sizeof(tcp_header)) {
@@ -148,6 +154,10 @@ void net_checksum_calculate(uint8_t *data, int length)
     }
     case IP_PROTO_UDP:
     {
+        if (!(csum_flag & CSUM_UDP)) {
+            return;
+        }
+
         udp_header *udp = (udp_header *)(ip + 1);
 
         if (ip_len < sizeof(udp_header)) {
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index e063a81..80caac5 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -114,7 +114,7 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
             tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
 
             net_checksum_calculate((uint8_t *)pkt->data + pkt->vnet_hdr_len,
-                                   pkt->size - pkt->vnet_hdr_len);
+                                   pkt->size - pkt->vnet_hdr_len, CSUM_TCP);
         }
 
         /*
@@ -216,7 +216,7 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
             tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
 
             net_checksum_calculate((uint8_t *)pkt->data + pkt->vnet_hdr_len,
-                                   pkt->size - pkt->vnet_hdr_len);
+                                   pkt->size - pkt->vnet_hdr_len, CSUM_TCP);
         }
     }
 
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
  2020-12-06  2:14   ` Bin Meng
@ 2020-12-06 11:50     ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-12-06 11:50 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Peter Maydell, Bin Meng, Li Zhijian, Michael S. Tsirkin,
	Andrew Jeffery, Jason Wang, Alistair Francis, Paul Durrant,
	Zhang Chen, Stefano Stabellini, Peter Chubb, Joel Stanley,
	Beniamino Galvani, Edgar E. Iglesias, Anthony Perard, xen-devel,
	David Gibson, qemu-ppc, qemu-arm, Cédric Le Goater

Hi Ben,

On 12/6/20 3:14 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present net_checksum_calculate() blindly calculates all types of
> checksums (IP, TCP, UDP). Some NICs may have a per type setting in
> their BDs to control what checksum should be offloaded. To support
> such hardware behavior, introduce a 'csum_flag' parameter to the
> net_checksum_calculate() API to allow fine control over what type
> checksum is calculated.
> 
> Existing users of this API are updated accordingly.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
>  hw/net/allwinner-sun8i-emac.c |  2 +-
>  hw/net/cadence_gem.c          |  2 +-
>  hw/net/fsl_etsec/rings.c      |  8 +++-----
>  hw/net/ftgmac100.c            | 10 +++++++++-
>  hw/net/imx_fec.c              | 15 +++------------
>  hw/net/virtio-net.c           |  2 +-
>  hw/net/xen_nic.c              |  2 +-
>  include/net/checksum.h        |  7 ++++++-

When sending a such API refactor, patch is easier to
review if you setup the scripts/git.orderfile config.

>  net/checksum.c                | 18 ++++++++++++++----
>  net/filter-rewriter.c         |  4 ++--
>  10 files changed, 41 insertions(+), 29 deletions(-)
...
> diff --git a/include/net/checksum.h b/include/net/checksum.h
> index 05a0d27..7dec37e 100644
> --- a/include/net/checksum.h
> +++ b/include/net/checksum.h
> @@ -21,11 +21,16 @@
>  #include "qemu/bswap.h"
>  struct iovec;
>  
> +#define CSUM_IP     0x01

IMO this is IP_HEADER,

> +#define CSUM_TCP    0x02
> +#define CSUM_UDP    0x04

and these IP_PAYLOAD, regardless the payload protocol.

> +#define CSUM_ALL    (CSUM_IP | CSUM_TCP | CSUM_UDP)

Maybe CSUM_HEADER / CSUM_PAYLOAD / CSUM_FULL (aka RAW?).

> +
>  uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
>  uint16_t net_checksum_finish(uint32_t sum);
>  uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
>                               uint8_t *addrs, uint8_t *buf);
> -void net_checksum_calculate(uint8_t *data, int length);
> +void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
>  
>  static inline uint32_t
>  net_checksum_add(int len, uint8_t *buf)
> diff --git a/net/checksum.c b/net/checksum.c
> index dabd290..70f4eae 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
>      return net_checksum_finish(sum);
>  }
>  
> -void net_checksum_calculate(uint8_t *data, int length)
> +void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
>  {
>      int mac_hdr_len, ip_len;
>      struct ip_header *ip;
> @@ -108,9 +108,11 @@ void net_checksum_calculate(uint8_t *data, int length)
>      }
>  
>      /* Calculate IP checksum */
> -    stw_he_p(&ip->ip_sum, 0);
> -    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> -    stw_be_p(&ip->ip_sum, csum);
> +    if (csum_flag & CSUM_IP) {
> +        stw_he_p(&ip->ip_sum, 0);
> +        csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> +        stw_be_p(&ip->ip_sum, csum);
> +    }
>  
>      if (IP4_IS_FRAGMENT(ip)) {
>          return; /* a fragmented IP packet */
> @@ -128,6 +130,10 @@ void net_checksum_calculate(uint8_t *data, int length)
>      switch (ip->ip_p) {
>      case IP_PROTO_TCP:
>      {
> +        if (!(csum_flag & CSUM_TCP)) {
> +            return;
> +        }
> +
>          tcp_header *tcp = (tcp_header *)(ip + 1);
>  
>          if (ip_len < sizeof(tcp_header)) {
> @@ -148,6 +154,10 @@ void net_checksum_calculate(uint8_t *data, int length)
>      }
>      case IP_PROTO_UDP:
>      {
> +        if (!(csum_flag & CSUM_UDP)) {
> +            return;
> +        }
> +
>          udp_header *udp = (udp_header *)(ip + 1);
>  
>          if (ip_len < sizeof(udp_header)) {
...



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
@ 2020-12-06 11:50     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-12-06 11:50 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Peter Maydell, Alistair Francis, Paul Durrant, Li Zhijian,
	Michael S. Tsirkin, Andrew Jeffery, Jason Wang, Bin Meng,
	Joel Stanley, Beniamino Galvani, Zhang Chen, Stefano Stabellini,
	Peter Chubb, Cédric Le Goater, qemu-arm, xen-devel,
	Anthony Perard, Edgar E. Iglesias, qemu-ppc, David Gibson

Hi Ben,

On 12/6/20 3:14 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present net_checksum_calculate() blindly calculates all types of
> checksums (IP, TCP, UDP). Some NICs may have a per type setting in
> their BDs to control what checksum should be offloaded. To support
> such hardware behavior, introduce a 'csum_flag' parameter to the
> net_checksum_calculate() API to allow fine control over what type
> checksum is calculated.
> 
> Existing users of this API are updated accordingly.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
>  hw/net/allwinner-sun8i-emac.c |  2 +-
>  hw/net/cadence_gem.c          |  2 +-
>  hw/net/fsl_etsec/rings.c      |  8 +++-----
>  hw/net/ftgmac100.c            | 10 +++++++++-
>  hw/net/imx_fec.c              | 15 +++------------
>  hw/net/virtio-net.c           |  2 +-
>  hw/net/xen_nic.c              |  2 +-
>  include/net/checksum.h        |  7 ++++++-

When sending a such API refactor, patch is easier to
review if you setup the scripts/git.orderfile config.

>  net/checksum.c                | 18 ++++++++++++++----
>  net/filter-rewriter.c         |  4 ++--
>  10 files changed, 41 insertions(+), 29 deletions(-)
...
> diff --git a/include/net/checksum.h b/include/net/checksum.h
> index 05a0d27..7dec37e 100644
> --- a/include/net/checksum.h
> +++ b/include/net/checksum.h
> @@ -21,11 +21,16 @@
>  #include "qemu/bswap.h"
>  struct iovec;
>  
> +#define CSUM_IP     0x01

IMO this is IP_HEADER,

> +#define CSUM_TCP    0x02
> +#define CSUM_UDP    0x04

and these IP_PAYLOAD, regardless the payload protocol.

> +#define CSUM_ALL    (CSUM_IP | CSUM_TCP | CSUM_UDP)

Maybe CSUM_HEADER / CSUM_PAYLOAD / CSUM_FULL (aka RAW?).

> +
>  uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
>  uint16_t net_checksum_finish(uint32_t sum);
>  uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
>                               uint8_t *addrs, uint8_t *buf);
> -void net_checksum_calculate(uint8_t *data, int length);
> +void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
>  
>  static inline uint32_t
>  net_checksum_add(int len, uint8_t *buf)
> diff --git a/net/checksum.c b/net/checksum.c
> index dabd290..70f4eae 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
>      return net_checksum_finish(sum);
>  }
>  
> -void net_checksum_calculate(uint8_t *data, int length)
> +void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
>  {
>      int mac_hdr_len, ip_len;
>      struct ip_header *ip;
> @@ -108,9 +108,11 @@ void net_checksum_calculate(uint8_t *data, int length)
>      }
>  
>      /* Calculate IP checksum */
> -    stw_he_p(&ip->ip_sum, 0);
> -    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> -    stw_be_p(&ip->ip_sum, csum);
> +    if (csum_flag & CSUM_IP) {
> +        stw_he_p(&ip->ip_sum, 0);
> +        csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> +        stw_be_p(&ip->ip_sum, csum);
> +    }
>  
>      if (IP4_IS_FRAGMENT(ip)) {
>          return; /* a fragmented IP packet */
> @@ -128,6 +130,10 @@ void net_checksum_calculate(uint8_t *data, int length)
>      switch (ip->ip_p) {
>      case IP_PROTO_TCP:
>      {
> +        if (!(csum_flag & CSUM_TCP)) {
> +            return;
> +        }
> +
>          tcp_header *tcp = (tcp_header *)(ip + 1);
>  
>          if (ip_len < sizeof(tcp_header)) {
> @@ -148,6 +154,10 @@ void net_checksum_calculate(uint8_t *data, int length)
>      }
>      case IP_PROTO_UDP:
>      {
> +        if (!(csum_flag & CSUM_UDP)) {
> +            return;
> +        }
> +
>          udp_header *udp = (udp_header *)(ip + 1);
>  
>          if (ip_len < sizeof(udp_header)) {
...



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
  2020-12-06 11:50     ` Philippe Mathieu-Daudé
@ 2020-12-06 12:44       ` Bin Meng
  -1 siblings, 0 replies; 10+ messages in thread
From: Bin Meng @ 2020-12-06 12:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Peter Maydell, Bin Meng, Li Zhijian, Michael S. Tsirkin,
	Andrew Jeffery, Jason Wang, Alistair Francis,
	qemu-devel@nongnu.org Developers, Paul Durrant, Zhang Chen,
	Stefano Stabellini, Peter Chubb, Joel Stanley, Beniamino Galvani,
	Edgar E. Iglesias, Anthony Perard, xen-devel, David Gibson,
	qemu-ppc, qemu-arm, Cédric Le Goater

Hi Philippe,

On Sun, Dec 6, 2020 at 7:50 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Ben,
>
> On 12/6/20 3:14 AM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present net_checksum_calculate() blindly calculates all types of
> > checksums (IP, TCP, UDP). Some NICs may have a per type setting in
> > their BDs to control what checksum should be offloaded. To support
> > such hardware behavior, introduce a 'csum_flag' parameter to the
> > net_checksum_calculate() API to allow fine control over what type
> > checksum is calculated.
> >
> > Existing users of this API are updated accordingly.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> >  hw/net/allwinner-sun8i-emac.c |  2 +-
> >  hw/net/cadence_gem.c          |  2 +-
> >  hw/net/fsl_etsec/rings.c      |  8 +++-----
> >  hw/net/ftgmac100.c            | 10 +++++++++-
> >  hw/net/imx_fec.c              | 15 +++------------
> >  hw/net/virtio-net.c           |  2 +-
> >  hw/net/xen_nic.c              |  2 +-
> >  include/net/checksum.h        |  7 ++++++-
>
> When sending a such API refactor, patch is easier to
> review if you setup the scripts/git.orderfile config.

Sure. I thought I have done this before but apparently not on the
machine this series was genearated :)

>
> >  net/checksum.c                | 18 ++++++++++++++----
> >  net/filter-rewriter.c         |  4 ++--
> >  10 files changed, 41 insertions(+), 29 deletions(-)
> ...
> > diff --git a/include/net/checksum.h b/include/net/checksum.h
> > index 05a0d27..7dec37e 100644
> > --- a/include/net/checksum.h
> > +++ b/include/net/checksum.h
> > @@ -21,11 +21,16 @@
> >  #include "qemu/bswap.h"
> >  struct iovec;
> >
> > +#define CSUM_IP     0x01
>
> IMO this is IP_HEADER,

Yes, but I believe no one will misread it, no?

>
> > +#define CSUM_TCP    0x02
> > +#define CSUM_UDP    0x04
>
> and these IP_PAYLOAD, regardless the payload protocol.

We have to distinguish TCP and UDP.

>
> > +#define CSUM_ALL    (CSUM_IP | CSUM_TCP | CSUM_UDP)
>
> Maybe CSUM_HEADER / CSUM_PAYLOAD / CSUM_FULL (aka RAW?).
>
> > +
> >  uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
> >  uint16_t net_checksum_finish(uint32_t sum);
> >  uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
> >                               uint8_t *addrs, uint8_t *buf);
> > -void net_checksum_calculate(uint8_t *data, int length);
> > +void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
> >
> >  static inline uint32_t
> >  net_checksum_add(int len, uint8_t *buf)
> > diff --git a/net/checksum.c b/net/checksum.c
> > index dabd290..70f4eae 100644
> > --- a/net/checksum.c
> > +++ b/net/checksum.c
> > @@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
> >      return net_checksum_finish(sum);
> >  }
> >
> > -void net_checksum_calculate(uint8_t *data, int length)
> > +void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
> >  {
> >      int mac_hdr_len, ip_len;
> >      struct ip_header *ip;
> > @@ -108,9 +108,11 @@ void net_checksum_calculate(uint8_t *data, int length)
> >      }
> >
> >      /* Calculate IP checksum */
> > -    stw_he_p(&ip->ip_sum, 0);
> > -    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> > -    stw_be_p(&ip->ip_sum, csum);
> > +    if (csum_flag & CSUM_IP) {
> > +        stw_he_p(&ip->ip_sum, 0);
> > +        csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> > +        stw_be_p(&ip->ip_sum, csum);
> > +    }
> >
> >      if (IP4_IS_FRAGMENT(ip)) {
> >          return; /* a fragmented IP packet */
> > @@ -128,6 +130,10 @@ void net_checksum_calculate(uint8_t *data, int length)
> >      switch (ip->ip_p) {
> >      case IP_PROTO_TCP:
> >      {
> > +        if (!(csum_flag & CSUM_TCP)) {
> > +            return;
> > +        }
> > +
> >          tcp_header *tcp = (tcp_header *)(ip + 1);
> >
> >          if (ip_len < sizeof(tcp_header)) {
> > @@ -148,6 +154,10 @@ void net_checksum_calculate(uint8_t *data, int length)
> >      }
> >      case IP_PROTO_UDP:
> >      {
> > +        if (!(csum_flag & CSUM_UDP)) {
> > +            return;
> > +        }
> > +
> >          udp_header *udp = (udp_header *)(ip + 1);
> >
> >          if (ip_len < sizeof(udp_header)) {
> ...

Regards,
Bin


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
@ 2020-12-06 12:44       ` Bin Meng
  0 siblings, 0 replies; 10+ messages in thread
From: Bin Meng @ 2020-12-06 12:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Alistair Francis, Paul Durrant, Li Zhijian, Michael S. Tsirkin,
	Andrew Jeffery, Jason Wang, Bin Meng, Joel Stanley,
	Beniamino Galvani, Zhang Chen, Stefano Stabellini, Peter Chubb,
	Cédric Le Goater, qemu-arm, xen-devel, Anthony Perard,
	Edgar E. Iglesias, qemu-ppc, David Gibson

Hi Philippe,

On Sun, Dec 6, 2020 at 7:50 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Ben,
>
> On 12/6/20 3:14 AM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present net_checksum_calculate() blindly calculates all types of
> > checksums (IP, TCP, UDP). Some NICs may have a per type setting in
> > their BDs to control what checksum should be offloaded. To support
> > such hardware behavior, introduce a 'csum_flag' parameter to the
> > net_checksum_calculate() API to allow fine control over what type
> > checksum is calculated.
> >
> > Existing users of this API are updated accordingly.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> >  hw/net/allwinner-sun8i-emac.c |  2 +-
> >  hw/net/cadence_gem.c          |  2 +-
> >  hw/net/fsl_etsec/rings.c      |  8 +++-----
> >  hw/net/ftgmac100.c            | 10 +++++++++-
> >  hw/net/imx_fec.c              | 15 +++------------
> >  hw/net/virtio-net.c           |  2 +-
> >  hw/net/xen_nic.c              |  2 +-
> >  include/net/checksum.h        |  7 ++++++-
>
> When sending a such API refactor, patch is easier to
> review if you setup the scripts/git.orderfile config.

Sure. I thought I have done this before but apparently not on the
machine this series was genearated :)

>
> >  net/checksum.c                | 18 ++++++++++++++----
> >  net/filter-rewriter.c         |  4 ++--
> >  10 files changed, 41 insertions(+), 29 deletions(-)
> ...
> > diff --git a/include/net/checksum.h b/include/net/checksum.h
> > index 05a0d27..7dec37e 100644
> > --- a/include/net/checksum.h
> > +++ b/include/net/checksum.h
> > @@ -21,11 +21,16 @@
> >  #include "qemu/bswap.h"
> >  struct iovec;
> >
> > +#define CSUM_IP     0x01
>
> IMO this is IP_HEADER,

Yes, but I believe no one will misread it, no?

>
> > +#define CSUM_TCP    0x02
> > +#define CSUM_UDP    0x04
>
> and these IP_PAYLOAD, regardless the payload protocol.

We have to distinguish TCP and UDP.

>
> > +#define CSUM_ALL    (CSUM_IP | CSUM_TCP | CSUM_UDP)
>
> Maybe CSUM_HEADER / CSUM_PAYLOAD / CSUM_FULL (aka RAW?).
>
> > +
> >  uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
> >  uint16_t net_checksum_finish(uint32_t sum);
> >  uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
> >                               uint8_t *addrs, uint8_t *buf);
> > -void net_checksum_calculate(uint8_t *data, int length);
> > +void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
> >
> >  static inline uint32_t
> >  net_checksum_add(int len, uint8_t *buf)
> > diff --git a/net/checksum.c b/net/checksum.c
> > index dabd290..70f4eae 100644
> > --- a/net/checksum.c
> > +++ b/net/checksum.c
> > @@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
> >      return net_checksum_finish(sum);
> >  }
> >
> > -void net_checksum_calculate(uint8_t *data, int length)
> > +void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
> >  {
> >      int mac_hdr_len, ip_len;
> >      struct ip_header *ip;
> > @@ -108,9 +108,11 @@ void net_checksum_calculate(uint8_t *data, int length)
> >      }
> >
> >      /* Calculate IP checksum */
> > -    stw_he_p(&ip->ip_sum, 0);
> > -    csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> > -    stw_be_p(&ip->ip_sum, csum);
> > +    if (csum_flag & CSUM_IP) {
> > +        stw_he_p(&ip->ip_sum, 0);
> > +        csum = net_raw_checksum((uint8_t *)ip, IP_HDR_GET_LEN(ip));
> > +        stw_be_p(&ip->ip_sum, csum);
> > +    }
> >
> >      if (IP4_IS_FRAGMENT(ip)) {
> >          return; /* a fragmented IP packet */
> > @@ -128,6 +130,10 @@ void net_checksum_calculate(uint8_t *data, int length)
> >      switch (ip->ip_p) {
> >      case IP_PROTO_TCP:
> >      {
> > +        if (!(csum_flag & CSUM_TCP)) {
> > +            return;
> > +        }
> > +
> >          tcp_header *tcp = (tcp_header *)(ip + 1);
> >
> >          if (ip_len < sizeof(tcp_header)) {
> > @@ -148,6 +154,10 @@ void net_checksum_calculate(uint8_t *data, int length)
> >      }
> >      case IP_PROTO_UDP:
> >      {
> > +        if (!(csum_flag & CSUM_UDP)) {
> > +            return;
> > +        }
> > +
> >          udp_header *udp = (udp_header *)(ip + 1);
> >
> >          if (ip_len < sizeof(udp_header)) {
> ...

Regards,
Bin


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
  2020-12-06  2:14   ` Bin Meng
@ 2020-12-09  8:33     ` Cédric Le Goater
  -1 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2020-12-09  8:33 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Peter Maydell, Alistair Francis, Paul Durrant, Li Zhijian,
	Michael S. Tsirkin, Andrew Jeffery, Jason Wang, Bin Meng,
	Beniamino Galvani, Zhang Chen, Stefano Stabellini, Peter Chubb,
	Joel Stanley, qemu-arm, xen-devel, Anthony Perard,
	Edgar E. Iglesias, qemu-ppc, David Gibson

Hello !

> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 782ff19..fbae1f1 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -573,7 +573,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>              }
>  
>              if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
> -                net_checksum_calculate(s->frame, frame_size);
> +                /*
> +                 * TODO:
> +                 * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
> +                 * however previous net_checksum_calculate() did not calculate
> +                 * IP checksum at all. Passing CSUM_ALL for now until someone
> +                 * who is familar with this MAC to figure out what should be
> +                 * properly added for TCP/UDP checksum offload.
> +                 */
> +                net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
>              }
>              /* Last buffer in frame.  */
>              qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);


You can test your changes using the HOWTO Joel provided here : 

  https://github.com/openbmc/qemu/wiki/Usage

Please also check the Linux driver  :

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/faraday/ftgmac100.c#n685

That said, something like the change below should be more appropriate.

Thanks,

C. 

+static int ftgmac100_convert_csum_flag(uint32_t flags)
+{
+    int csum = 0;
+
+    if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
+        csum |= CSUM_IP;
+    }
+    if (flags & FTGMAC100_TXDES1_TCP_CHKSUM) {
+        csum |= CSUM_TCP;
+    }
+    if (flags & FTGMAC100_TXDES1_UDP_CHKSUM) {
+        csum |= CSUM_UDP;
+    }
+    return csum;
+}
+
 static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
                             uint32_t tx_descriptor)
 {
@@ -602,6 +618,7 @@ static void ftgmac100_do_tx(FTGMAC100Sta
         ptr += len;
         frame_size += len;
         if (bd.des0 & FTGMAC100_TXDES0_LTS) {
+            int csum = ftgmac100_convert_csum_flag(flags);
 
             /* Check for VLAN */
             if (flags & FTGMAC100_TXDES1_INS_VLANTAG &&
@@ -610,16 +627,8 @@ static void ftgmac100_do_tx(FTGMAC100Sta
                                             FTGMAC100_TXDES1_VLANTAG_CI(flags));
             }
 
-            if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
-                /*
-                 * TODO:
-                 * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
-                 * however previous net_checksum_calculate() did not calculate
-                 * IP checksum at all. Passing CSUM_ALL for now until someone
-                 * who is familar with this MAC to figure out what should be
-                 * properly added for TCP/UDP checksum offload.
-                 */
-                net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
+            if (csum) {
+                net_checksum_calculate(s->frame, frame_size, csum);
             }
             /* Last buffer in frame.  */
             qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
@ 2020-12-09  8:33     ` Cédric Le Goater
  0 siblings, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2020-12-09  8:33 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Bin Meng, Alistair Francis, Andrew Jeffery, Anthony Perard,
	Beniamino Galvani, David Gibson, Edgar E. Iglesias, Jason Wang,
	Joel Stanley, Li Zhijian, Michael S. Tsirkin, Paul Durrant,
	Peter Chubb, Peter Maydell, Stefano Stabellini, Zhang Chen,
	qemu-arm, qemu-ppc, xen-devel

Hello !

> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 782ff19..fbae1f1 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -573,7 +573,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>              }
>  
>              if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
> -                net_checksum_calculate(s->frame, frame_size);
> +                /*
> +                 * TODO:
> +                 * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
> +                 * however previous net_checksum_calculate() did not calculate
> +                 * IP checksum at all. Passing CSUM_ALL for now until someone
> +                 * who is familar with this MAC to figure out what should be
> +                 * properly added for TCP/UDP checksum offload.
> +                 */
> +                net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
>              }
>              /* Last buffer in frame.  */
>              qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);


You can test your changes using the HOWTO Joel provided here : 

  https://github.com/openbmc/qemu/wiki/Usage

Please also check the Linux driver  :

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/faraday/ftgmac100.c#n685

That said, something like the change below should be more appropriate.

Thanks,

C. 

+static int ftgmac100_convert_csum_flag(uint32_t flags)
+{
+    int csum = 0;
+
+    if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
+        csum |= CSUM_IP;
+    }
+    if (flags & FTGMAC100_TXDES1_TCP_CHKSUM) {
+        csum |= CSUM_TCP;
+    }
+    if (flags & FTGMAC100_TXDES1_UDP_CHKSUM) {
+        csum |= CSUM_UDP;
+    }
+    return csum;
+}
+
 static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
                             uint32_t tx_descriptor)
 {
@@ -602,6 +618,7 @@ static void ftgmac100_do_tx(FTGMAC100Sta
         ptr += len;
         frame_size += len;
         if (bd.des0 & FTGMAC100_TXDES0_LTS) {
+            int csum = ftgmac100_convert_csum_flag(flags);
 
             /* Check for VLAN */
             if (flags & FTGMAC100_TXDES1_INS_VLANTAG &&
@@ -610,16 +627,8 @@ static void ftgmac100_do_tx(FTGMAC100Sta
                                             FTGMAC100_TXDES1_VLANTAG_CI(flags));
             }
 
-            if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
-                /*
-                 * TODO:
-                 * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
-                 * however previous net_checksum_calculate() did not calculate
-                 * IP checksum at all. Passing CSUM_ALL for now until someone
-                 * who is familar with this MAC to figure out what should be
-                 * properly added for TCP/UDP checksum offload.
-                 */
-                net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
+            if (csum) {
+                net_checksum_calculate(s->frame, frame_size, csum);
             }
             /* Last buffer in frame.  */
             qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);


^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-09  8:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-06  2:14 [PATCH 1/3] net: checksum: Skip fragmented IP packets Bin Meng
2020-12-06  2:14 ` [PATCH 2/3] net: checksum: Add IP header checksum calculation Bin Meng
2020-12-06  2:14 ` [PATCH 3/3] net: checksum: Introduce fine control over checksum type Bin Meng
2020-12-06  2:14   ` Bin Meng
2020-12-06 11:50   ` Philippe Mathieu-Daudé
2020-12-06 11:50     ` Philippe Mathieu-Daudé
2020-12-06 12:44     ` Bin Meng
2020-12-06 12:44       ` Bin Meng
2020-12-09  8:33   ` Cédric Le Goater
2020-12-09  8:33     ` Cédric Le Goater

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.