[dpdk-dev] [PATCH v4 0/8] add virtio offload support in us-vhost

[dpdk-dev] [PATCH v4 0/8] add virtio offload support in us-vhost

[Jijiang Liu]

Adds virtio offload support in us-vhost.
 
The patch set adds the negotiation support between us-vhost and vanilla Linux virtio guest for TX offload(checksum and TSO), and add the offload support in the vhost libs and change vhost sample to test these changes.
 
v4 change:
  remove virtio-net change, only keep vhost changes.
  add guest TX offload capabilities to support VM to VM case.
  split the cleanup code as a seperate patch.
 
v3 change:
  rebase latest codes.
 
v2 change:
  fill virtio device information for TX offloads.

*** BLURB HERE ***

Jijiang Liu (8):
  add virtual addr of virtio net header
  store vir address of virtio hdr
  add vhost TX offload capabilities(CSUM/TSO)
  add dequeue offload handle in vhost lib
  remove ip_hdr defination in vhost sample
  change vhost app to support virtio offload test
  add guest offload(CSUM/TSO)
  add guest tx offload handle in vhost lib

 drivers/net/virtio/virtio_ethdev.c |    3 +
 drivers/net/virtio/virtqueue.h     |    1 +
 examples/vhost/main.c              |  125 ++++++++++++++++++++++++++----
 lib/librte_vhost/vhost_rxtx.c      |  149 +++++++++++++++++++++++++++++++++++-
 lib/librte_vhost/virtio-net.c      |    9 ++-
 5 files changed, 266 insertions(+), 21 deletions(-)

-- 
1.7.7.6

[dpdk-dev] [PATCH v4 1/8] driver/virtio:add virtual address for virtio net header

[Jijiang Liu]

The virtual address for virtio net header need to be recorded.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 drivers/net/virtio/virtqueue.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 689c321..5b43eeb 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -190,6 +190,7 @@ struct virtqueue {
 	uint16_t vq_avail_idx;
 	uint64_t mbuf_initializer; /**< value to init mbufs. */
 	phys_addr_t virtio_net_hdr_mem; /**< hdr for each xmit packet */
+	uint64_t virtio_net_hdr_addr; /**< virtual addr for virtio net header */
 
 	struct rte_mbuf **sw_ring; /**< RX software ring. */
 	/* dummy mbuf, for wraparound when processing RX ring. */
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 2/8] driver/virtio:record virtual address of virtio net header

[Jijiang Liu]

Record virtual address of virtio net header.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 74c00ee..dd39715 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -414,6 +414,9 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 		}
 		vq->virtio_net_hdr_mem =
 			vq->virtio_net_hdr_mz->phys_addr;
+		vq->virtio_net_hdr_addr =
+			(uint64_t)(uintptr_t)vq->virtio_net_hdr_mz->addr;
+
 		memset(vq->virtio_net_hdr_mz->addr, 0,
 			vq_size * hw->vtnet_hdr_size);
 	} else if (queue_type == VTNET_CQ) {
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 3/8] virtio/lib:add vhost TX checksum support capabilities

[Jijiang Liu]

Add vhost TX offload(CSUM and TSO) support capabilities.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_vhost/virtio-net.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 14278de..81bd309 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -77,7 +77,11 @@ static struct virtio_net_config_ll *ll_root;
 				(VHOST_SUPPORTS_MQ)            | \
 				(1ULL << VIRTIO_F_VERSION_1)   | \
 				(1ULL << VHOST_F_LOG_ALL)      | \
-				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
+				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
+				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
+				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
+				(1ULL << VIRTIO_NET_F_CSUM))
+
 static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
 
 
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 4/8] virtio/lib:dequeue vhost TX offload

[Jijiang Liu]

Dequeue vhost TX offload(CSUM and TSO) in vhost lib

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_vhost/vhost_rxtx.c |  104 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 103 insertions(+), 1 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 9322ce6..9e70990 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -37,7 +37,12 @@
 
 #include <rte_mbuf.h>
 #include <rte_memcpy.h>
+#include <rte_ether.h>
+#include <rte_ip.h>
 #include <rte_virtio_net.h>
+#include <rte_tcp.h>
+#include <rte_udp.h>
+#include <rte_sctp.h>
 
 #include "vhost-net.h"
 
@@ -568,6 +573,97 @@ rte_vhost_enqueue_burst(struct virtio_net *dev, uint16_t queue_id,
 		return virtio_dev_rx(dev, queue_id, pkts, count);
 }
 
+static void
+parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
+{
+	struct ipv4_hdr *ipv4_hdr;
+	struct ipv6_hdr *ipv6_hdr;
+	void *l3_hdr = NULL;
+	struct ether_hdr *eth_hdr;
+	uint16_t ethertype;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+
+	m->l2_len = sizeof(struct ether_hdr);
+	ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
+
+	if (ethertype == ETHER_TYPE_VLAN) {
+		struct vlan_hdr *vlan_hdr = (struct vlan_hdr *)(eth_hdr + 1);
+
+		m->l2_len += sizeof(struct vlan_hdr);
+		ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+	}
+
+	l3_hdr = (char *)eth_hdr + m->l2_len;
+
+	switch (ethertype) {
+	case ETHER_TYPE_IPv4:
+		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
+		*l4_proto = ipv4_hdr->next_proto_id;
+		m->l3_len = (ipv4_hdr->version_ihl & 0x0f) * 4;
+		*l4_hdr = (char *)l3_hdr + m->l3_len;
+		m->ol_flags |= PKT_TX_IPV4;
+		break;
+	case ETHER_TYPE_IPv6:
+		ipv6_hdr = (struct ipv6_hdr *)l3_hdr;
+		*l4_proto = ipv6_hdr->proto;
+		m->ol_flags |= PKT_TX_IPV6;
+		m->l3_len = sizeof(struct ipv6_hdr);
+		*l4_hdr = (char *)l3_hdr + m->l3_len;
+		break;
+	default:
+		m->l3_len = 0;
+		*l4_proto = 0;
+		break;
+	}
+}
+
+static inline void __attribute__((always_inline))
+vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
+{
+	uint16_t l4_proto = 0;
+	void *l4_hdr = NULL;
+	struct tcp_hdr *tcp_hdr = NULL;
+
+	parse_ethernet(m, &l4_proto, &l4_hdr);
+	if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
+		if (hdr->csum_start == (m->l2_len + m->l3_len)) {
+			switch (hdr->csum_offset) {
+			case (offsetof(struct tcp_hdr, cksum)):
+				if (l4_proto == IPPROTO_TCP)
+					m->ol_flags |= PKT_TX_TCP_CKSUM;
+				break;
+			case (offsetof(struct udp_hdr, dgram_cksum)):
+				if (l4_proto == IPPROTO_UDP)
+					m->ol_flags |= PKT_TX_UDP_CKSUM;
+				break;
+			case (offsetof(struct sctp_hdr, cksum)):
+				if (l4_proto == IPPROTO_SCTP)
+					m->ol_flags |= PKT_TX_SCTP_CKSUM;
+				break;
+			default:
+				break;
+			}
+		}
+	}
+
+	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
+		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
+		case VIRTIO_NET_HDR_GSO_TCPV4:
+		case VIRTIO_NET_HDR_GSO_TCPV6:
+			tcp_hdr = (struct tcp_hdr *)l4_hdr;
+			m->ol_flags |= PKT_TX_TCP_SEG;
+			m->tso_segsz = hdr->gso_size;
+			m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2;
+			break;
+		default:
+			RTE_LOG(WARNING, VHOST_DATA,
+				"unsupported gso type %u.\n", hdr->gso_type);
+			break;
+		}
+	}
+}
+
 uint16_t
 rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 	struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts, uint16_t count)
@@ -576,11 +672,13 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 	struct vhost_virtqueue *vq;
 	struct vring_desc *desc;
 	uint64_t vb_addr = 0;
+	uint64_t vb_net_hdr_addr = 0;
 	uint32_t head[MAX_PKT_BURST];
 	uint32_t used_idx;
 	uint32_t i;
 	uint16_t free_entries, entry_success = 0;
 	uint16_t avail_idx;
+	struct virtio_net_hdr *hdr = NULL;
 
 	if (unlikely(!is_valid_virt_queue_idx(queue_id, 1, dev->virt_qp_nb))) {
 		RTE_LOG(ERR, VHOST_DATA,
@@ -632,6 +730,9 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 
 		desc = &vq->desc[head[entry_success]];
 
+		vb_net_hdr_addr = gpa_to_vva(dev, desc->addr);
+		hdr = (struct virtio_net_hdr *)((uintptr_t)vb_net_hdr_addr);
+
 		/* Discard first buffer as it is the virtio header */
 		if (desc->flags & VRING_DESC_F_NEXT) {
 			desc = &vq->desc[desc->next];
@@ -770,7 +871,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 			break;
 
 		m->nb_segs = seg_num;
-
+		if ((hdr->flags != 0) || (hdr->gso_type != 0))
+			vhost_dequeue_offload(hdr, m);
 		pkts[entry_success] = m;
 		vq->last_used_idx++;
 		entry_success++;
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 5/8] sample/vhost:remove ip_hdr structure defination

[Jijiang Liu]

Remove the ip_hdr structure defination.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 examples/vhost/main.c |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index c081b18..044c680 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -50,6 +50,7 @@
 #include <rte_string_fns.h>
 #include <rte_malloc.h>
 #include <rte_virtio_net.h>
+#include <rte_ip.h>
 
 #include "main.h"
 
@@ -292,20 +293,6 @@ struct vlan_ethhdr {
 	__be16          h_vlan_encapsulated_proto;
 };
 
-/* IPv4 Header */
-struct ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */
-	uint8_t  type_of_service;	/**< type of service */
-	uint16_t total_length;		/**< length of packet */
-	uint16_t packet_id;		/**< packet ID */
-	uint16_t fragment_offset;	/**< fragmentation offset */
-	uint8_t  time_to_live;		/**< time to live */
-	uint8_t  next_proto_id;		/**< protocol ID */
-	uint16_t hdr_checksum;		/**< header checksum */
-	uint32_t src_addr;		/**< source address */
-	uint32_t dst_addr;		/**< destination address */
-} __attribute__((__packed__));
-
 /* Header lengths. */
 #define VLAN_HLEN       4
 #define VLAN_ETH_HLEN   18
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 6/8] examples/vhost:support TX offload in vhost sample

[Jijiang Liu]

Change the vhost sample to support and test TX offload.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 examples/vhost/main.c |  110 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 044c680..24e2c26 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -51,6 +51,9 @@
 #include <rte_malloc.h>
 #include <rte_virtio_net.h>
 #include <rte_ip.h>
+#include <rte_tcp.h>
+#include <rte_udp.h>
+#include <rte_sctp.h>
 
 #include "main.h"
 
@@ -198,6 +201,13 @@ typedef enum {
 static uint32_t enable_stats = 0;
 /* Enable retries on RX. */
 static uint32_t enable_retry = 1;
+
+/* Disable TX checksum offload */
+static uint32_t enable_tx_csum;
+
+/* Disable TSO offload */
+static uint32_t enable_tso;
+
 /* Specify timeout (in useconds) between retries on RX. */
 static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;
 /* Specify the number of retries on RX. */
@@ -428,6 +438,14 @@ port_init(uint8_t port)
 
 	if (port >= rte_eth_dev_count()) return -1;
 
+	if (enable_tx_csum == 0)
+		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_CSUM);
+
+	if (enable_tso == 0) {
+		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4);
+		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO6);
+	}
+
 	rx_rings = (uint16_t)dev_info.max_rx_queues;
 	/* Configure ethernet device. */
 	retval = rte_eth_dev_configure(port, rx_rings, tx_rings, &port_conf);
@@ -563,7 +581,9 @@ us_vhost_usage(const char *prgname)
 	"		--rx-desc-num [0-N]: the number of descriptors on rx, "
 			"used only when zero copy is enabled.\n"
 	"		--tx-desc-num [0-N]: the number of descriptors on tx, "
-			"used only when zero copy is enabled.\n",
+			"used only when zero copy is enabled.\n"
+	"		--tx-csum [0|1] disable/enable TX checksum offload.\n"
+	"		--tso [0|1] disable/enable TCP segement offload.\n",
 	       prgname);
 }
 
@@ -589,6 +609,8 @@ us_vhost_parse_args(int argc, char **argv)
 		{"zero-copy", required_argument, NULL, 0},
 		{"rx-desc-num", required_argument, NULL, 0},
 		{"tx-desc-num", required_argument, NULL, 0},
+		{"tx-csum", required_argument, NULL, 0},
+		{"tso", required_argument, NULL, 0},
 		{NULL, 0, 0, 0},
 	};
 
@@ -643,6 +665,28 @@ us_vhost_parse_args(int argc, char **argv)
 				}
 			}
 
+			/* Enable/disable TX checksum offload. */
+			if (!strncmp(long_option[option_index].name, "tx-csum", MAX_LONG_OPT_SZ)) {
+				ret = parse_num_opt(optarg, 1);
+				if (ret == -1) {
+					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for tx-csum [0|1]\n");
+					us_vhost_usage(prgname);
+					return -1;
+				} else
+					enable_tx_csum = ret;
+			}
+
+			/* Enable/disable TSO offload. */
+			if (!strncmp(long_option[option_index].name, "tso", MAX_LONG_OPT_SZ)) {
+				ret = parse_num_opt(optarg, 1);
+				if (ret == -1) {
+					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for tso [0|1]\n");
+					us_vhost_usage(prgname);
+					return -1;
+				} else
+					enable_tso = ret;
+			}
+
 			/* Specify the retries delay time (in useconds) on RX. */
 			if (!strncmp(long_option[option_index].name, "rx-retry-delay", MAX_LONG_OPT_SZ)) {
 				ret = parse_num_opt(optarg, INT32_MAX);
@@ -1101,6 +1145,63 @@ find_local_dest(struct virtio_net *dev, struct rte_mbuf *m,
 	return 0;
 }
 
+static uint16_t
+get_psd_sum(void *l3_hdr, uint64_t ol_flags)
+{
+	if (ol_flags & PKT_TX_IPV4)
+		return rte_ipv4_phdr_cksum(l3_hdr, ol_flags);
+	else /* assume ethertype == ETHER_TYPE_IPv6 */
+		return rte_ipv6_phdr_cksum(l3_hdr, ol_flags);
+}
+
+static void virtio_tx_offload(struct rte_mbuf *m)
+{
+	void *l3_hdr;
+	struct ipv4_hdr *ipv4_hdr = NULL;
+	struct tcp_hdr *tcp_hdr = NULL;
+	struct udp_hdr *udp_hdr = NULL;
+	struct sctp_hdr *sctp_hdr = NULL;
+	struct ether_hdr *eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+
+	l3_hdr = (char *)eth_hdr + m->l2_len;
+
+	if (m->ol_flags & PKT_TX_IPV4) {
+		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
+		if (m->ol_flags & PKT_TX_IP_CKSUM)
+			ipv4_hdr->hdr_checksum = 0;
+	}
+
+	if (m->ol_flags & PKT_TX_L4_MASK) {
+		switch (m->ol_flags & PKT_TX_L4_MASK) {
+		case PKT_TX_TCP_CKSUM:
+			tcp_hdr = (struct tcp_hdr *)
+					((char *)l3_hdr + m->l3_len);
+			tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
+			break;
+		case PKT_TX_UDP_CKSUM:
+			udp_hdr = (struct udp_hdr *)
+					((char *)l3_hdr + m->l3_len);
+			udp_hdr->dgram_cksum = get_psd_sum(l3_hdr, m->ol_flags);
+			break;
+		case PKT_TX_SCTP_CKSUM:
+			sctp_hdr = (struct sctp_hdr *)
+					((char *)l3_hdr + m->l3_len);
+			sctp_hdr->cksum = 0;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (m->tso_segsz != 0) {
+		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
+		tcp_hdr = (struct tcp_hdr *)((char *)l3_hdr + m->l3_len);
+		m->ol_flags |= PKT_TX_IP_CKSUM;
+		ipv4_hdr->hdr_checksum = 0;
+		tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
+	}
+}
+
 /*
  * This function routes the TX packet to the correct interface. This may be a local device
  * or the physical port.
@@ -1143,7 +1244,7 @@ virtio_tx_route(struct vhost_dev *vdev, struct rte_mbuf *m, uint16_t vlan_tag)
 			(vh->vlan_tci != vlan_tag_be))
 			vh->vlan_tci = vlan_tag_be;
 	} else {
-		m->ol_flags = PKT_TX_VLAN_PKT;
+		m->ol_flags |= PKT_TX_VLAN_PKT;
 
 		/*
 		 * Find the right seg to adjust the data len when offset is
@@ -1167,6 +1268,9 @@ virtio_tx_route(struct vhost_dev *vdev, struct rte_mbuf *m, uint16_t vlan_tag)
 		m->vlan_tci = vlan_tag;
 	}
 
+	if ((m->ol_flags & PKT_TX_L4_MASK) || (m->ol_flags & PKT_TX_TCP_SEG))
+		virtio_tx_offload(m);
+
 	tx_q->m_table[len] = m;
 	len++;
 	if (enable_stats) {
@@ -1828,7 +1932,7 @@ virtio_tx_route_zcp(struct virtio_net *dev, struct rte_mbuf *m,
 		mbuf->buf_physaddr = m->buf_physaddr;
 		mbuf->buf_addr = m->buf_addr;
 	}
-	mbuf->ol_flags = PKT_TX_VLAN_PKT;
+	mbuf->ol_flags |= PKT_TX_VLAN_PKT;
 	mbuf->vlan_tci = vlan_tag;
 	mbuf->l2_len = sizeof(struct ether_hdr);
 	mbuf->l3_len = sizeof(struct ipv4_hdr);
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Jijiang Liu]

Add virtio guest offload capabilities.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_vhost/virtio-net.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 81bd309..839a333 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -80,7 +80,10 @@ static struct virtio_net_config_ll *ll_root;
 				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
 				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
 				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
-				(1ULL << VIRTIO_NET_F_CSUM))
+				(1ULL << VIRTIO_NET_F_CSUM)    | \
+				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
+				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
+				(1ULL << VIRTIO_NET_F_GUEST_TSO6))
 
 static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
 
-- 
1.7.7.6

[dpdk-dev] [PATCH v4 8/8] virtio/lib:add guest offload handle

[Jijiang Liu]

Enqueue guest offload(CSUM and TSO) handle.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_vhost/vhost_rxtx.c |   45 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 9e70990..468fed8 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -54,6 +54,42 @@ is_valid_virt_queue_idx(uint32_t idx, int is_tx, uint32_t qp_nb)
 	return (is_tx ^ (idx & 1)) == 0 && idx < qp_nb * VIRTIO_QNUM;
 }
 
+static void
+virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
+{
+	if (m_buf->ol_flags & PKT_TX_L4_MASK) {
+		net_hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
+		net_hdr->csum_start = m_buf->l2_len + m_buf->l3_len;
+
+		switch (m_buf->ol_flags & PKT_TX_L4_MASK) {
+		case PKT_TX_TCP_CKSUM:
+			net_hdr->csum_offset = (offsetof(struct tcp_hdr,
+						cksum));
+			break;
+		case PKT_TX_UDP_CKSUM:
+			net_hdr->csum_offset = (offsetof(struct udp_hdr,
+						dgram_cksum));
+			break;
+		case PKT_TX_SCTP_CKSUM:
+			net_hdr->csum_offset = (offsetof(struct sctp_hdr,
+						cksum));
+			break;
+		}
+	}
+
+	if (m_buf->ol_flags & PKT_TX_TCP_SEG) {
+		if (m_buf->ol_flags & PKT_TX_IPV4)
+			net_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
+		else
+			net_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+		net_hdr->gso_size = m_buf->tso_segsz;
+		net_hdr->hdr_len = m_buf->l2_len + m_buf->l3_len
+					+ m_buf->l4_len;
+	}
+
+	return;
+}
+
 /**
  * This function adds buffers to the virtio devices RX virtqueue. Buffers can
  * be received from the physical port or from another virtio device. A packet
@@ -67,7 +103,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 {
 	struct vhost_virtqueue *vq;
 	struct vring_desc *desc;
-	struct rte_mbuf *buff;
+	struct rte_mbuf *buff, *first_buff;
 	/* The virtio_hdr is initialised to 0. */
 	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
 	uint64_t buff_addr = 0;
@@ -139,6 +175,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 		desc = &vq->desc[head[packet_success]];
 
 		buff = pkts[packet_success];
+		first_buff = buff;
 
 		/* Convert from gpa to vva (guest physical addr -> vhost virtual addr) */
 		buff_addr = gpa_to_vva(dev, desc->addr);
@@ -221,7 +258,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 
 		if (unlikely(uncompleted_pkt == 1))
 			continue;
-
+		
+		virtio_enqueue_offload(first_buff, &virtio_hdr.hdr);
+		
 		rte_memcpy((void *)(uintptr_t)buff_hdr_addr,
 			(const void *)&virtio_hdr, vq->vhost_hlen);
 
@@ -295,6 +334,8 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 	LOG_DEBUG(VHOST_DATA, "(%"PRIu64") RX: Num merge buffers %d\n",
 		dev->device_fh, virtio_hdr.num_buffers);
 
+	virtio_enqueue_offload(pkt, &virtio_hdr.hdr);	
+
 	rte_memcpy((void *)(uintptr_t)vb_hdr_addr,
 		(const void *)&virtio_hdr, vq->vhost_hlen);
 
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v4 8/8] virtio/lib:add guest offload handle

[Yuanhan Liu]

Regarding to your patch title, there are two minor pits:

- the prefix should be "vhost" but not "virtio/lib".

- you should add an extra space after ":"

On Wed, Nov 11, 2015 at 02:40:46PM +0800, Jijiang Liu wrote:
> Enqueue guest offload(CSUM and TSO) handle.

(ALL) Your patch lacks some explanation. And I don't think it's about
guest offload handling, it's about setting the right offload fields for
RX side, such as VIRTIO_NET_HDR_F_NEEDS_CSUM.

And you need spend few words to state why that is required. Something
like following might help others to review:

    For packet going through from one VM to another VM without passing
    the NIC, and the VM claiming that it supports checksum offload,
    no one will actually calculate the checksum, hence, the packet
    will be dropped at TCP layer, due to checksum validation is failed.

    However, for VM2VM case, there is no need to do checksum, for we
    think the data should be reliable enough, and setting VIRTIO_NET_HDR_F_NEEDS_CSUM
    at RX side will let the TCP layer to bypass the checksum validation,
    so that the RX side could receive the packet in the end.

    At RX side, the offload information is inherited from mbuf, which is
    in turn inherited from TX side. If we can still get those info at RX
    side, it means the packet is from another VM at same host.  So, it's
    safe to set the VIRTIO_NET_HDR_F_NEEDS_CSUM, to skip checksum validation.


> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>  lib/librte_vhost/vhost_rxtx.c |   45 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index 9e70990..468fed8 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -54,6 +54,42 @@ is_valid_virt_queue_idx(uint32_t idx, int is_tx, uint32_t qp_nb)
>  	return (is_tx ^ (idx & 1)) == 0 && idx < qp_nb * VIRTIO_QNUM;
>  }
>  
> +static void
> +virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
> +{

As virtio_hdr is set per mbuf, you'd better reset net_hdr first before
setting it. Otherwise, if this mbuf has no offload related stuff, you
may still get a net_hdr with offload related fields set, due to last
mbuf has that.

I know the chance is rare, but it's for code logic.


	--yliu

Re: [dpdk-dev] [PATCH v4 3/8] virtio/lib:add vhost TX checksum support capabilities

[Yuanhan Liu]

On Wed, Nov 11, 2015 at 02:40:41PM +0800, Jijiang Liu wrote:
> Add vhost TX offload(CSUM and TSO) support capabilities.

Claiming first that we support something, and then actually implementing
in a later patch is wrong, as at this stage, we actually does not support
that, hence, the functionality is broken.

	--yliu

> 
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>  lib/librte_vhost/virtio-net.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 14278de..81bd309 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -77,7 +77,11 @@ static struct virtio_net_config_ll *ll_root;
>  				(VHOST_SUPPORTS_MQ)            | \
>  				(1ULL << VIRTIO_F_VERSION_1)   | \
>  				(1ULL << VHOST_F_LOG_ALL)      | \
> -				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> +				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
> +				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> +				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> +				(1ULL << VIRTIO_NET_F_CSUM))
> +
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
>  
>  
> -- 
> 1.7.7.6

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Yuanhan Liu]

On Wed, Nov 11, 2015 at 02:40:45PM +0800, Jijiang Liu wrote:
> Add virtio guest offload capabilities.
> 
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>  lib/librte_vhost/virtio-net.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 81bd309..839a333 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -80,7 +80,10 @@ static struct virtio_net_config_ll *ll_root;
>  				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
>  				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
>  				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> -				(1ULL << VIRTIO_NET_F_CSUM))
> +				(1ULL << VIRTIO_NET_F_CSUM)    | \
> +				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> +				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> +				(1ULL << VIRTIO_NET_F_GUEST_TSO6))

I don't think we need that, and it might be wrong to set those fields at
vhost.

And, TBH, I am not 100% sure that I understand what those flags truely are
and for. All I know is that they seem have something to do with QEMU/TAP
device.

Hopefully the virtio expert, Michael, could shine some lights on that.

	--yliu

>  
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
>  
> -- 
> 1.7.7.6

Re: [dpdk-dev] [PATCH v4 5/8] sample/vhost:remove ip_hdr structure defination

[Yuanhan Liu]

On Wed, Nov 11, 2015 at 02:40:43PM +0800, Jijiang Liu wrote:
> Remove the ip_hdr structure defination.

Even for such simple patch, you need state why. Stating that
"remove ip_hdr structure defination because we already have
that in rte_ip.h" will definitely let others to accept your
patch eaiser.

	--yliu
> 
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>  examples/vhost/main.c |   15 +--------------
>  1 files changed, 1 insertions(+), 14 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index c081b18..044c680 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -50,6 +50,7 @@
>  #include <rte_string_fns.h>
>  #include <rte_malloc.h>
>  #include <rte_virtio_net.h>
> +#include <rte_ip.h>
>  
>  #include "main.h"
>  
> @@ -292,20 +293,6 @@ struct vlan_ethhdr {
>  	__be16          h_vlan_encapsulated_proto;
>  };
>  
> -/* IPv4 Header */
> -struct ipv4_hdr {
> -	uint8_t  version_ihl;		/**< version and header length */
> -	uint8_t  type_of_service;	/**< type of service */
> -	uint16_t total_length;		/**< length of packet */
> -	uint16_t packet_id;		/**< packet ID */
> -	uint16_t fragment_offset;	/**< fragmentation offset */
> -	uint8_t  time_to_live;		/**< time to live */
> -	uint8_t  next_proto_id;		/**< protocol ID */
> -	uint16_t hdr_checksum;		/**< header checksum */
> -	uint32_t src_addr;		/**< source address */
> -	uint32_t dst_addr;		/**< destination address */
> -} __attribute__((__packed__));
> -
>  /* Header lengths. */
>  #define VLAN_HLEN       4
>  #define VLAN_ETH_HLEN   18
> -- 
> 1.7.7.6

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Liu, Jijiang]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, November 11, 2015 4:31 PM
> To: Liu, Jijiang; Michael S. Tsirkin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload
> capabilities
> 
> On Wed, Nov 11, 2015 at 02:40:45PM +0800, Jijiang Liu wrote:
> > Add virtio guest offload capabilities.
> >
> > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> > ---
> >  lib/librte_vhost/virtio-net.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/lib/librte_vhost/virtio-net.c
> > b/lib/librte_vhost/virtio-net.c index 81bd309..839a333 100644
> > --- a/lib/librte_vhost/virtio-net.c
> > +++ b/lib/librte_vhost/virtio-net.c
> > @@ -80,7 +80,10 @@ static struct virtio_net_config_ll *ll_root;
> >  				(1ULL <<
> VHOST_USER_F_PROTOCOL_FEATURES) | \
> >  				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> >  				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> > -				(1ULL << VIRTIO_NET_F_CSUM))
> > +				(1ULL << VIRTIO_NET_F_CSUM)    | \
> > +				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> > +				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > +				(1ULL << VIRTIO_NET_F_GUEST_TSO6))
> 
> I don't think we need that, and it might be wrong to set those fields at vhost.
> 
> And, TBH, I am not 100% sure that I understand what those flags truely are
> and for. All I know is that they seem have something to do with QEMU/TAP
> device.

According to virtio standard, those fileds should be set.
Yes, I'd like to listen other guys comments.

> Hopefully the virtio expert, Michael, could shine some lights on that.
> 
> 	--yliu
> 
> >
> >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> >
> > --
> > 1.7.7.6

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Yuanhan Liu]

On Wed, Nov 11, 2015 at 08:38:29AM +0000, Liu, Jijiang wrote:
> 
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Wednesday, November 11, 2015 4:31 PM
> > To: Liu, Jijiang; Michael S. Tsirkin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload
> > capabilities
> > 
> > On Wed, Nov 11, 2015 at 02:40:45PM +0800, Jijiang Liu wrote:
> > > Add virtio guest offload capabilities.
> > >
> > > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> > > ---
> > >  lib/librte_vhost/virtio-net.c |    5 ++++-
> > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/lib/librte_vhost/virtio-net.c
> > > b/lib/librte_vhost/virtio-net.c index 81bd309..839a333 100644
> > > --- a/lib/librte_vhost/virtio-net.c
> > > +++ b/lib/librte_vhost/virtio-net.c
> > > @@ -80,7 +80,10 @@ static struct virtio_net_config_ll *ll_root;
> > >  				(1ULL <<
> > VHOST_USER_F_PROTOCOL_FEATURES) | \
> > >  				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> > >  				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> > > -				(1ULL << VIRTIO_NET_F_CSUM))
> > > +				(1ULL << VIRTIO_NET_F_CSUM)    | \
> > > +				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> > > +				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > +				(1ULL << VIRTIO_NET_F_GUEST_TSO6))
> > 
> > I don't think we need that, and it might be wrong to set those fields at vhost.
> > 
> > And, TBH, I am not 100% sure that I understand what those flags truely are
> > and for. All I know is that they seem have something to do with QEMU/TAP
> > device.
> 
> According to virtio standard, those fileds should be set.

If so, you'd better quote them here, or even to your patch commit.

	--yliu

> Yes, I'd like to listen other guys comments.
> 
> > Hopefully the virtio expert, Michael, could shine some lights on that.
> > 
> > 	--yliu
> > 
> > >
> > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > >
> > > --
> > > 1.7.7.6

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Liu, Jijiang]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, November 11, 2015 4:44 PM
> To: Liu, Jijiang
> Cc: Michael S. Tsirkin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload
> capabilities
> 
> On Wed, Nov 11, 2015 at 08:38:29AM +0000, Liu, Jijiang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > Sent: Wednesday, November 11, 2015 4:31 PM
> > > To: Liu, Jijiang; Michael S. Tsirkin
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest
> > > offload capabilities
> > >
> > > On Wed, Nov 11, 2015 at 02:40:45PM +0800, Jijiang Liu wrote:
> > > > Add virtio guest offload capabilities.
> > > >
> > > > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> > > > ---
> > > >  lib/librte_vhost/virtio-net.c |    5 ++++-
> > > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/lib/librte_vhost/virtio-net.c
> > > > b/lib/librte_vhost/virtio-net.c index 81bd309..839a333 100644
> > > > --- a/lib/librte_vhost/virtio-net.c
> > > > +++ b/lib/librte_vhost/virtio-net.c
> > > > @@ -80,7 +80,10 @@ static struct virtio_net_config_ll *ll_root;
> > > >  				(1ULL <<
> > > VHOST_USER_F_PROTOCOL_FEATURES) | \
> > > >  				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> > > >  				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> > > > -				(1ULL << VIRTIO_NET_F_CSUM))
> > > > +				(1ULL << VIRTIO_NET_F_CSUM)    | \
> > > > +				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> > > > +				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > +				(1ULL << VIRTIO_NET_F_GUEST_TSO6))
> > >
> > > I don't think we need that, and it might be wrong to set those fields at
> vhost.
> > >
> > > And, TBH, I am not 100% sure that I understand what those flags
> > > truely are and for. All I know is that they seem have something to
> > > do with QEMU/TAP device.
> >
> > According to virtio standard, those fileds should be set.
> 
> If so, you'd better quote them here, or even to your patch commit.

In the Virtual I/O Device (VIRTIO) Version 1.0 Committee Specification 02, 

(1) VIRTIO_NET_F_GUEST_CSUM (1) Driver handles packets with partial checksum.

(2) If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_NEEDS_-
CSUM bit in flags MAY be set: if so, the checksum on the packet is incomplete and csum_start
and csum_offset indicate how to calculate it (see Packet Transmission point 1).

(3) If the VIRTIO_NET_F_GUEST_TSO4, TSO6 or UFO options were negotiated, then gso_type
MAY be something other than VIRTIO_NET_HDR_GSO_NONE, and gso_size field indicates
the desired MSS (see Packet Transmission point 2).

The information can be included in the next version of patch set.

> 	--yliu
> 
> > Yes, I'd like to listen other guys comments.
> >
> > > Hopefully the virtio expert, Michael, could shine some lights on that.
> > >
> > > 	--yliu
> > >
> > > >
> > > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > > >
> > > > --
> > > > 1.7.7.6

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Xu, Qian Q]

Frank, 


Thanks
Qian

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
Sent: Wednesday, November 11, 2015 4:53 PM
To: Yuanhan Liu
Cc: dev@dpdk.org; Michael S. Tsirkin
Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities



> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, November 11, 2015 4:44 PM
> To: Liu, Jijiang
> Cc: Michael S. Tsirkin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest 
> offload capabilities
> 
> On Wed, Nov 11, 2015 at 08:38:29AM +0000, Liu, Jijiang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > Sent: Wednesday, November 11, 2015 4:31 PM
> > > To: Liu, Jijiang; Michael S. Tsirkin
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest 
> > > offload capabilities
> > >
> > > On Wed, Nov 11, 2015 at 02:40:45PM +0800, Jijiang Liu wrote:
> > > > Add virtio guest offload capabilities.
> > > >
> > > > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> > > > ---
> > > >  lib/librte_vhost/virtio-net.c |    5 ++++-
> > > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/lib/librte_vhost/virtio-net.c 
> > > > b/lib/librte_vhost/virtio-net.c index 81bd309..839a333 100644
> > > > --- a/lib/librte_vhost/virtio-net.c
> > > > +++ b/lib/librte_vhost/virtio-net.c
> > > > @@ -80,7 +80,10 @@ static struct virtio_net_config_ll *ll_root;
> > > >  				(1ULL <<
> > > VHOST_USER_F_PROTOCOL_FEATURES) | \
> > > >  				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
> > > >  				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
> > > > -				(1ULL << VIRTIO_NET_F_CSUM))
> > > > +				(1ULL << VIRTIO_NET_F_CSUM)    | \
> > > > +				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
> > > > +				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > +				(1ULL << VIRTIO_NET_F_GUEST_TSO6))
> > >
> > > I don't think we need that, and it might be wrong to set those 
> > > fields at
> vhost.
> > >
> > > And, TBH, I am not 100% sure that I understand what those flags 
> > > truely are and for. All I know is that they seem have something to 
> > > do with QEMU/TAP device.
> >
> > According to virtio standard, those fileds should be set.
> 
> If so, you'd better quote them here, or even to your patch commit.

In the Virtual I/O Device (VIRTIO) Version 1.0 Committee Specification 02, 


[QIAN] Do we support virtio 1.0 in the vhost offload case? Currently it's virtio 0.95 Spec that we are testing. 


(1) VIRTIO_NET_F_GUEST_CSUM (1) Driver handles packets with partial checksum.

(2) If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_NEEDS_- CSUM bit in flags MAY be set: if so, the checksum on the packet is incomplete and csum_start and csum_offset indicate how to calculate it (see Packet Transmission point 1).

(3) If the VIRTIO_NET_F_GUEST_TSO4, TSO6 or UFO options were negotiated, then gso_type MAY be something other than VIRTIO_NET_HDR_GSO_NONE, and gso_size field indicates the desired MSS (see Packet Transmission point 2).

The information can be included in the next version of patch set.

> 	--yliu
> 
> > Yes, I'd like to listen other guys comments.
> >
> > > Hopefully the virtio expert, Michael, could shine some lights on that.
> > >
> > > 	--yliu
> > >
> > > >
> > > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > > >
> > > > --
> > > > 1.7.7.6

Re: [dpdk-dev] [PATCH v4 5/8] sample/vhost:remove ip_hdr structure defination

[Thomas Monjalon]

2015-11-11 16:34, Yuanhan Liu:
> On Wed, Nov 11, 2015 at 02:40:43PM +0800, Jijiang Liu wrote:
> > Remove the ip_hdr structure defination.
> 
> Even for such simple patch, you need state why. Stating that
> "remove ip_hdr structure defination because we already have
> that in rte_ip.h" will definitely let others to accept your
> patch eaiser.

Yes, thanks Yuanhan.

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Thomas Monjalon]

2015-11-11 08:53, Liu, Jijiang:
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > On Wed, Nov 11, 2015 at 08:38:29AM +0000, Liu, Jijiang wrote:
> > > According to virtio standard, those fileds should be set.
> > 
> > If so, you'd better quote them here, or even to your patch commit.
> 
> In the Virtual I/O Device (VIRTIO) Version 1.0 Committee Specification 02, 
> 
> (1) VIRTIO_NET_F_GUEST_CSUM (1) Driver handles packets with partial checksum.
> 
> (2) If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_NEEDS_-
> CSUM bit in flags MAY be set: if so, the checksum on the packet is incomplete and csum_start
> and csum_offset indicate how to calculate it (see Packet Transmission point 1).
> 
> (3) If the VIRTIO_NET_F_GUEST_TSO4, TSO6 or UFO options were negotiated, then gso_type
> MAY be something other than VIRTIO_NET_HDR_GSO_NONE, and gso_size field indicates
> the desired MSS (see Packet Transmission point 2).
> 
> The information can be included in the next version of patch set.

Yes quoting specifications or datasheets in commit messages is a really good idea.
Thanks for improving the quality of the git history.

Re: [dpdk-dev] [PATCH v4 8/8] virtio/lib:add guest offload handle

[Thomas Monjalon]

Yuanhan,
You deserve a "review award"!
Thanks a lot

2015-11-11 16:23, Yuanhan Liu:
> Regarding to your patch title, there are two minor pits:
> 
> - the prefix should be "vhost" but not "virtio/lib".
> 
> - you should add an extra space after ":"
> 
> On Wed, Nov 11, 2015 at 02:40:46PM +0800, Jijiang Liu wrote:
> > Enqueue guest offload(CSUM and TSO) handle.
> 
> (ALL) Your patch lacks some explanation. And I don't think it's about
> guest offload handling, it's about setting the right offload fields for
> RX side, such as VIRTIO_NET_HDR_F_NEEDS_CSUM.
> 
> And you need spend few words to state why that is required. Something
> like following might help others to review:
> 
>     For packet going through from one VM to another VM without passing
>     the NIC, and the VM claiming that it supports checksum offload,
>     no one will actually calculate the checksum, hence, the packet
>     will be dropped at TCP layer, due to checksum validation is failed.
> 
>     However, for VM2VM case, there is no need to do checksum, for we
>     think the data should be reliable enough, and setting VIRTIO_NET_HDR_F_NEEDS_CSUM
>     at RX side will let the TCP layer to bypass the checksum validation,
>     so that the RX side could receive the packet in the end.
> 
>     At RX side, the offload information is inherited from mbuf, which is
>     in turn inherited from TX side. If we can still get those info at RX
>     side, it means the packet is from another VM at same host.  So, it's
>     safe to set the VIRTIO_NET_HDR_F_NEEDS_CSUM, to skip checksum validation.
> 
> 
> > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> > ---
> >  lib/librte_vhost/vhost_rxtx.c |   45 +++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> > index 9e70990..468fed8 100644
> > --- a/lib/librte_vhost/vhost_rxtx.c
> > +++ b/lib/librte_vhost/vhost_rxtx.c
> > @@ -54,6 +54,42 @@ is_valid_virt_queue_idx(uint32_t idx, int is_tx, uint32_t qp_nb)
> >  	return (is_tx ^ (idx & 1)) == 0 && idx < qp_nb * VIRTIO_QNUM;
> >  }
> >  
> > +static void
> > +virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
> > +{
> 
> As virtio_hdr is set per mbuf, you'd better reset net_hdr first before
> setting it. Otherwise, if this mbuf has no offload related stuff, you
> may still get a net_hdr with offload related fields set, due to last
> mbuf has that.
> 
> I know the chance is rare, but it's for code logic.
> 
> 
> 	--yliu

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Liu, Jijiang]

Hi Qian,


> 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> Sent: Wednesday, November 11, 2015 4:53 PM
> To: Yuanhan Liu
> Cc: dev@dpdk.org; Michael S. Tsirkin
> Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload
> capabilities
> 
> 
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Wednesday, November 11, 2015 4:44 PM
> > To: Liu, Jijiang
> > Cc: Michael S. Tsirkin; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest
> > offload capabilities
> >
> > On Wed, Nov 11, 2015 at 08:38:29AM +0000, Liu, Jijiang wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > > Sent: Wednesday, November 11, 2015 4:31 PM
> > > > To: Liu, Jijiang; Michael S. Tsirkin
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest
> > > > offload capabilities
> > > >
> > > > I don't think we need that, and it might be wrong to set those
> > > > fields at
> > vhost.
> > > >
> > > > And, TBH, I am not 100% sure that I understand what those flags
> > > > truely are and for. All I know is that they seem have something to
> > > > do with QEMU/TAP device.
> > >
> > > According to virtio standard, those fileds should be set.
> >
> > If so, you'd better quote them here, or even to your patch commit.
> 
> In the Virtual I/O Device (VIRTIO) Version 1.0 Committee Specification 02,
> 
> 
> [QIAN] Do we support virtio 1.0 in the vhost offload case? Currently it's virtio
> 0.95 Spec that we are testing.
> 
There are almost same description in  0.95 Spec,

(1)VIRTIO_NET_F_GUEST_CSUM (1) Guest handles packets with
partial checksum
(2) If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
VIRTIO_NET_HDR_F_NEEDS_CSUM bit in the \x10\x1dags\x11 \x1celd may be
set: if so, the checksum on the packet is incomplete and the \x10csum_start\x11
and \x10csum_o^[set\x11 \x1celds indicate how to calculate it (see 1).

(3) The converse features are also available: a driver can save the virtual device
some work by negotiating these features.8 The VIRTIO_NET_F_GUEST_CSUM
feature indicates that partially checksummed packets can be received,
and if it can do that then the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_
NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO and
VIRTIO_NET_F_GUEST_ECN are the input equivalents of the features
described above. See \x10Receiving Packets\x11 below.

Re: [dpdk-dev] [PATCH v4 7/8] virtio/lib:add virtio guest offload capabilities

[Yuanhan Liu]

On Wed, Nov 11, 2015 at 08:53:08AM +0000, Liu, Jijiang wrote:
...
> > If so, you'd better quote them here, or even to your patch commit.
> 
> In the Virtual I/O Device (VIRTIO) Version 1.0 Committee Specification 02, 
> 
> (1) VIRTIO_NET_F_GUEST_CSUM (1) Driver handles packets with partial checksum.
> 
> (2) If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the VIRTIO_NET_HDR_F_NEEDS_-
> CSUM bit in flags MAY be set: if so, the checksum on the packet is incomplete and csum_start
> and csum_offset indicate how to calculate it (see Packet Transmission point 1).
> 
> (3) If the VIRTIO_NET_F_GUEST_TSO4, TSO6 or UFO options were negotiated, then gso_type
> MAY be something other than VIRTIO_NET_HDR_GSO_NONE, and gso_size field indicates
> the desired MSS (see Packet Transmission point 2).

Thanks. Yes, sound like we need set them.

	--yliu
> 
> The information can be included in the next version of patch set.
> 
> > 	--yliu
> > 
> > > Yes, I'd like to listen other guys comments.
> > >
> > > > Hopefully the virtio expert, Michael, could shine some lights on that.
> > > >
> > > > 	--yliu
> > > >
> > > > >
> > > > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > > > >
> > > > > --
> > > > > 1.7.7.6

Re: [dpdk-dev] [PATCH v4 3/8] virtio/lib:add vhost TX checksum support capabilities

[Stephen Hemminger]

On Wed, 11 Nov 2015 16:26:57 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> On Wed, Nov 11, 2015 at 02:40:41PM +0800, Jijiang Liu wrote:
> > Add vhost TX offload(CSUM and TSO) support capabilities.
> 
> Claiming first that we support something, and then actually implementing
> in a later patch is wrong, as at this stage, we actually does not support
> that, hence, the functionality is broken.
> 
> 	--yliu

Actually in this case it is okay to claim that driver "might" use offload
cabability but never do it. But agree in general better to keep both together.

Re: [dpdk-dev] [PATCH v4 3/8] virtio/lib:add vhost TX checksum support capabilities

[Yuanhan Liu]

On Wed, Nov 11, 2015 at 09:31:14AM -0800, Stephen Hemminger wrote:
> On Wed, 11 Nov 2015 16:26:57 +0800
> Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> 
> > On Wed, Nov 11, 2015 at 02:40:41PM +0800, Jijiang Liu wrote:
> > > Add vhost TX offload(CSUM and TSO) support capabilities.
> > 
> > Claiming first that we support something, and then actually implementing
> > in a later patch is wrong, as at this stage, we actually does not support
> > that, hence, the functionality is broken.
> > 
> > 	--yliu
> 
> Actually in this case it is okay to claim that driver "might" use offload
> cabability but never do it.

But it will not work once it does use it, right?

	--yliu

> But agree in general better to keep both together.