[PATCH v2 0/3] Vhost & Virtio fixes for v18.02

[PATCH v2 0/3] Vhost & Virtio fixes for v18.02

[Maxime Coquelin]

The second revision of this series includes Olivier's patch as first
patch in order to ease Thomas job when applying.

Patch 2 rewords the commit message with detailed informations
provided by Tiwei and olivier (Thanks!). It also moves call to
virtio_rxq_vec_setup() and removes virtqueue_enqueue_recv_refill_simple()
as no more used.

Maxime Coquelin (2):
  virtio: fix resuming port with rx vector path
  vhost: don't take access_lock on VHOST_USER_RESET_OWNER

Olivier Matz (1):
  net/virtio: fix mbuf data offset for simple Rx function

 drivers/net/virtio/virtio_rxtx.c        | 38 ++++++++++++++++++---------------
 drivers/net/virtio/virtio_rxtx.h        |  3 ---
 drivers/net/virtio/virtio_rxtx_simple.c | 30 +-------------------------
 drivers/net/virtio/virtio_rxtx_simple.h |  2 +-
 lib/librte_vhost/vhost_user.c           | 10 ++++-----
 5 files changed, 28 insertions(+), 55 deletions(-)

-- 
2.14.3

[PATCH v2 1/3] net/virtio: fix mbuf data offset for simple Rx function

[Maxime Coquelin]

From: Olivier Matz <olivier.matz@6wind.com>

The mbuf->data_off was was not properly set for the first received
mbufs. Fix this by setting it in virtqueue_enqueue_recv_refill_simple(),
which is used to enqueue the first mbuf in the ring.

The function virtio_rxq_rearm_vec(), which is used to rearm the ring
with new mbufs, is valid and does not need to be updated.

Fixes: cab0461234e7 ("virtio: fill Rx avail ring with blank mbufs")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_rxtx_simple.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
index 7247a0822..b1f610ffa 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.c
+++ b/drivers/net/virtio/virtio_rxtx_simple.c
@@ -36,6 +36,7 @@ virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
 	uint16_t desc_idx;
 
 	cookie->port = vq->rxq.port_id;
+	cookie->data_off = RTE_PKTMBUF_HEADROOM;
 
 	desc_idx = vq->vq_avail_idx & (vq->vq_nentries - 1);
 	dxp = &vq->vq_descx[desc_idx];
-- 
2.14.3

Re: [PATCH v2 1/3] net/virtio: fix mbuf data offset for simple Rx function

[Maxime Coquelin]

On 02/12/2018 04:46 PM, Maxime Coquelin wrote:
> From: Olivier Matz <olivier.matz@6wind.com>
> 
> The mbuf->data_off was was not properly set for the first received
> mbufs. Fix this by setting it in virtqueue_enqueue_recv_refill_simple(),
> which is used to enqueue the first mbuf in the ring.
> 
> The function virtio_rxq_rearm_vec(), which is used to rearm the ring
> with new mbufs, is valid and does not need to be updated.
> 
> Fixes: cab0461234e7 ("virtio: fill Rx avail ring with blank mbufs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>   drivers/net/virtio/virtio_rxtx_simple.c | 1 +
>   1 file changed, 1 insertion(+)
> 

I forgot to apply my r-b when re-posting:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Maxime

[PATCH v2 2/3] virtio: fix resuming port with rx vector path

[Maxime Coquelin]

Since commit efc83a1e7fc3 ("net/virtio: fix queue setup consistency"),
when resuming a virtio port, the rx rings are refilled with new mbufs
until they are full (vq->vq_free_cnt == 0). This is done without
ensuring that the descriptor index remains a multiple of
RTE_VIRTIO_VPMD_RX_REARM_THRESH, which is a prerequisite when using the
vector mode. This can cause an out of bound access in the rx ring.

This commit changes the vector refill method from
virtqueue_enqueue_recv_refill_simple() to virtio_rxq_rearm_vec(), which
properly checks that the refill is done by batch of
RTE_VIRTIO_VPMD_RX_REARM_THRESH.

As virtqueue_enqueue_recv_refill_simple() is no more used, this
patch also removes the function.

Fixes: efc83a1e7fc3 ("net/virtio: fix queue setup consistency")

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c        | 38 ++++++++++++++++++---------------
 drivers/net/virtio/virtio_rxtx.h        |  3 ---
 drivers/net/virtio/virtio_rxtx_simple.c | 31 +--------------------------
 drivers/net/virtio/virtio_rxtx_simple.h |  2 +-
 4 files changed, 23 insertions(+), 51 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 854af399e..8dbf2a30e 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -30,6 +30,7 @@
 #include "virtio_pci.h"
 #include "virtqueue.h"
 #include "virtio_rxtx.h"
+#include "virtio_rxtx_simple.h"
 
 #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
 #define VIRTIO_DUMP_PACKET(m, len) rte_pktmbuf_dump(stdout, m, len)
@@ -437,6 +438,8 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 			vq->vq_ring.desc[desc_idx].flags =
 				VRING_DESC_F_WRITE;
 		}
+
+		virtio_rxq_vec_setup(rxvq);
 	}
 
 	memset(&rxvq->fake_mbuf, 0, sizeof(rxvq->fake_mbuf));
@@ -446,30 +449,31 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 			&rxvq->fake_mbuf;
 	}
 
-	while (!virtqueue_full(vq)) {
-		m = rte_mbuf_raw_alloc(rxvq->mpool);
-		if (m == NULL)
-			break;
+	if (hw->use_simple_rx) {
+		while (vq->vq_free_cnt >= RTE_VIRTIO_VPMD_RX_REARM_THRESH) {
+			virtio_rxq_rearm_vec(rxvq);
+			nbufs += RTE_VIRTIO_VPMD_RX_REARM_THRESH;
+		}
+	} else {
+		while (!virtqueue_full(vq)) {
+			m = rte_mbuf_raw_alloc(rxvq->mpool);
+			if (m == NULL)
+				break;
 
-		/* Enqueue allocated buffers */
-		if (hw->use_simple_rx)
-			error = virtqueue_enqueue_recv_refill_simple(vq, m);
-		else
+			/* Enqueue allocated buffers */
 			error = virtqueue_enqueue_recv_refill(vq, m);
-
-		if (error) {
-			rte_pktmbuf_free(m);
-			break;
+			if (error) {
+				rte_pktmbuf_free(m);
+				break;
+			}
+			nbufs++;
 		}
-		nbufs++;
-	}
 
-	vq_update_avail_idx(vq);
+		vq_update_avail_idx(vq);
+	}
 
 	PMD_INIT_LOG(DEBUG, "Allocated %d bufs", nbufs);
 
-	virtio_rxq_vec_setup(rxvq);
-
 	VIRTQUEUE_DUMP(vq);
 
 	return 0;
diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virtio_rxtx.h
index 49e9d98ee..685cc4f81 100644
--- a/drivers/net/virtio/virtio_rxtx.h
+++ b/drivers/net/virtio/virtio_rxtx.h
@@ -60,7 +60,4 @@ struct virtnet_ctl {
 
 int virtio_rxq_vec_setup(struct virtnet_rx *rxvq);
 
-int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
-	struct rte_mbuf *m);
-
 #endif /* _VIRTIO_RXTX_H_ */
diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
index b1f610ffa..515207581 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.c
+++ b/drivers/net/virtio/virtio_rxtx_simple.c
@@ -27,35 +27,6 @@
 #pragma GCC diagnostic ignored "-Wcast-qual"
 #endif
 
-int __attribute__((cold))
-virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
-	struct rte_mbuf *cookie)
-{
-	struct vq_desc_extra *dxp;
-	struct vring_desc *start_dp;
-	uint16_t desc_idx;
-
-	cookie->port = vq->rxq.port_id;
-	cookie->data_off = RTE_PKTMBUF_HEADROOM;
-
-	desc_idx = vq->vq_avail_idx & (vq->vq_nentries - 1);
-	dxp = &vq->vq_descx[desc_idx];
-	dxp->cookie = (void *)cookie;
-	vq->sw_ring[desc_idx] = cookie;
-
-	start_dp = vq->vq_ring.desc;
-	start_dp[desc_idx].addr =
-		VIRTIO_MBUF_ADDR(cookie, vq) +
-		RTE_PKTMBUF_HEADROOM - vq->hw->vtnet_hdr_size;
-	start_dp[desc_idx].len = cookie->buf_len -
-		RTE_PKTMBUF_HEADROOM + vq->hw->vtnet_hdr_size;
-
-	vq->vq_free_cnt--;
-	vq->vq_avail_idx++;
-
-	return 0;
-}
-
 uint16_t
 virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
 	uint16_t nb_pkts)
@@ -78,7 +49,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
 	rte_compiler_barrier();
 
 	if (nb_used >= VIRTIO_TX_FREE_THRESH)
-		virtio_xmit_cleanup(vq);
+		virtio_xmit_cleanup_simple(vq);
 
 	nb_commit = nb_pkts = RTE_MIN((vq->vq_free_cnt >> 1), nb_pkts);
 	desc_idx = (uint16_t)(vq->vq_avail_idx & desc_idx_max);
diff --git a/drivers/net/virtio/virtio_rxtx_simple.h b/drivers/net/virtio/virtio_rxtx_simple.h
index 2d8e6b14a..303904d64 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.h
+++ b/drivers/net/virtio/virtio_rxtx_simple.h
@@ -60,7 +60,7 @@ virtio_rxq_rearm_vec(struct virtnet_rx *rxvq)
 #define VIRTIO_TX_FREE_NR 32
 /* TODO: vq->tx_free_cnt could mean num of free slots so we could avoid shift */
 static inline void
-virtio_xmit_cleanup(struct virtqueue *vq)
+virtio_xmit_cleanup_simple(struct virtqueue *vq)
 {
 	uint16_t i, desc_idx;
 	uint32_t nb_free = 0;
-- 
2.14.3

[PATCH v2 3/3] vhost: don't take access_lock on VHOST_USER_RESET_OWNER

[Maxime Coquelin]

A deadlock happens when handling VHOST_USER_RESET_OWNER request
for the same reason the lock is not taken for
VHOST_USER_GET_VRING_BASE.

It is safe not to take the lock, as the queues are no more used
by the application when the virtqueues and the device are reset.

Fixes: a3688046995f ("vhost: protect active rings from async ring changes")
Cc: stable@dpdk.org

Cc: Victor Kaplansky <victork@redhat.com>
Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>
Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 65ee33919..90ed2112e 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1348,16 +1348,16 @@ vhost_user_msg_handler(int vid, int fd)
 	}
 
 	/*
-	 * Note: we don't lock all queues on VHOST_USER_GET_VRING_BASE,
-	 * since it is sent when virtio stops and device is destroyed.
-	 * destroy_device waits for queues to be inactive, so it is safe.
-	 * Otherwise taking the access_lock would cause a dead lock.
+	 * Note: we don't lock all queues on VHOST_USER_GET_VRING_BASE
+	 * and VHOST_USER_RESET_OWNER, since it is sent when virtio stops
+	 * and device is destroyed. destroy_device waits for queues to be
+	 * inactive, so it is safe. Otherwise taking the access_lock
+	 * would cause a dead lock.
 	 */
 	switch (msg.request.master) {
 	case VHOST_USER_SET_FEATURES:
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
 	case VHOST_USER_SET_OWNER:
-	case VHOST_USER_RESET_OWNER:
 	case VHOST_USER_SET_MEM_TABLE:
 	case VHOST_USER_SET_LOG_BASE:
 	case VHOST_USER_SET_LOG_FD:
-- 
2.14.3

Re: [dpdk-stable] [PATCH v2 0/3] Vhost & Virtio fixes for v18.02

[Thomas Monjalon]

12/02/2018 16:46, Maxime Coquelin:
> The second revision of this series includes Olivier's patch as first
> patch in order to ease Thomas job when applying.
> 
> Patch 2 rewords the commit message with detailed informations
> provided by Tiwei and olivier (Thanks!). It also moves call to
> virtio_rxq_vec_setup() and removes virtqueue_enqueue_recv_refill_simple()
> as no more used.
> 
> Maxime Coquelin (2):
>   virtio: fix resuming port with rx vector path
>   vhost: don't take access_lock on VHOST_USER_RESET_OWNER
> 
> Olivier Matz (1):
>   net/virtio: fix mbuf data offset for simple Rx function

Applied, thanks