* [dpdk-stable] [PATCH 17.11] net/virtio: fix resuming port with Rx vector path
@ 2019-03-08 10:48 Tiwei Bie
2019-03-08 16:31 ` Yongseok Koh
0 siblings, 1 reply; 2+ messages in thread
From: Tiwei Bie @ 2019-03-08 10:48 UTC (permalink / raw)
To: yskoh, stable; +Cc: maxime.coquelin, zhihong.wang, Olivier Matz
[ backported from upstream commit 478574706638ff78cbc7e82731a4eae743322ac6 ]
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")
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
Hi Yongseok,
This fix patch is backported from v18.02, and targets for v17.11.6.
It fixes a port resuming issue for Rx vector path in virtio PMD.
Thanks,
Tiwei
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 +-
4 files changed, 23 insertions(+), 50 deletions(-)
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 390c137c8..275b65f43 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -59,6 +59,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)
@@ -465,6 +466,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));
@@ -474,30 +477,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 54f1e849b..4143551a1 100644
--- a/drivers/net/virtio/virtio_rxtx.h
+++ b/drivers/net/virtio/virtio_rxtx.h
@@ -88,7 +88,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 b5bc1c49f..86737e416 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.c
+++ b/drivers/net/virtio/virtio_rxtx_simple.c
@@ -56,34 +56,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;
-
- 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)
@@ -106,7 +78,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 f531c5428..5f6dabc8a 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.h
+++ b/drivers/net/virtio/virtio_rxtx_simple.h
@@ -89,7 +89,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.17.1
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [dpdk-stable] [PATCH 17.11] net/virtio: fix resuming port with Rx vector path
2019-03-08 10:48 [dpdk-stable] [PATCH 17.11] net/virtio: fix resuming port with Rx vector path Tiwei Bie
@ 2019-03-08 16:31 ` Yongseok Koh
0 siblings, 0 replies; 2+ messages in thread
From: Yongseok Koh @ 2019-03-08 16:31 UTC (permalink / raw)
To: Tiwei Bie; +Cc: dpdk stable, Maxime Coquelin, zhihong.wang, Olivier Matz
> On Mar 8, 2019, at 2:48 AM, Tiwei Bie <tiwei.bie@intel.com> wrote:
>
> [ backported from upstream commit 478574706638ff78cbc7e82731a4eae743322ac6 ]
>
> 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")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> Hi Yongseok,
>
> This fix patch is backported from v18.02, and targets for v17.11.6.
> It fixes a port resuming issue for Rx vector path in virtio PMD.
>
> Thanks,
> Tiwei
Applied to stable/17.11.
Thanks,
Yongseok
> 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 +-
> 4 files changed, 23 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 390c137c8..275b65f43 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -59,6 +59,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)
> @@ -465,6 +466,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));
> @@ -474,30 +477,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 54f1e849b..4143551a1 100644
> --- a/drivers/net/virtio/virtio_rxtx.h
> +++ b/drivers/net/virtio/virtio_rxtx.h
> @@ -88,7 +88,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 b5bc1c49f..86737e416 100644
> --- a/drivers/net/virtio/virtio_rxtx_simple.c
> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
> @@ -56,34 +56,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;
> -
> - 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)
> @@ -106,7 +78,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 f531c5428..5f6dabc8a 100644
> --- a/drivers/net/virtio/virtio_rxtx_simple.h
> +++ b/drivers/net/virtio/virtio_rxtx_simple.h
> @@ -89,7 +89,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.17.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-03-08 16:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-08 10:48 [dpdk-stable] [PATCH 17.11] net/virtio: fix resuming port with Rx vector path Tiwei Bie
2019-03-08 16:31 ` Yongseok Koh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).