Re: [dpdk-dev] [PATCH] net/virtio: revert forcing IOVA as VA mode for virtio-user
From: David Marchand <hidden>
Date: 2021-09-30 07:26:41
Hello Maxime, On Wed, Sep 29, 2021 at 10:18 PM Maxime Coquelin [off-list ref] wrote:
This patch removes the simplification in Virtio descriptors
handling, where their buffer addresses are IOVAs for Virtio
PCI devices, and VA-only for Virtio-user devices, which
added a requirement on Virtio-user that it only supported
IOVA as VA.
This change introduced a regression for applications using
Virtio-user and other physical PMDs that require IOVA as PA
because they don't use an IOMMU.
This patch reverts to the old behaviour, but needed to be
reworked because of the refactoring that happened in v21.02.
Fixes: 17043a2909bb ("net/virtio: force IOVA as VA mode for virtio-user")
Cc: stable@dpdk.org
Reported-by: Olivier Matz <redacted>
Signed-off-by: Maxime Coquelin <redacted>This patch does not apply on next-virtio, but you are best placed to figure this out :-). Quick look, only nits otherwise patch lgtm.
quoted hunk ↗ jump to hunk
--- drivers/net/virtio/virtio.h | 1 + drivers/net/virtio/virtio_ethdev.c | 25 +++++++++++++---- drivers/net/virtio/virtio_rxtx.c | 28 ++++++++++++-------- drivers/net/virtio/virtio_rxtx_packed.h | 2 +- drivers/net/virtio/virtio_rxtx_packed_avx.h | 8 +++--- drivers/net/virtio/virtio_rxtx_packed_neon.h | 8 +++--- drivers/net/virtio/virtio_rxtx_simple.h | 3 ++- drivers/net/virtio/virtio_user_ethdev.c | 7 ++++- drivers/net/virtio/virtqueue.h | 22 ++++++++++++++- 9 files changed, 76 insertions(+), 28 deletions(-)diff --git a/drivers/net/virtio/virtio.h b/drivers/net/virtio/virtio.h index b4f21dc0c7..7118e5d24c 100644 --- a/drivers/net/virtio/virtio.h +++ b/drivers/net/virtio/virtio.h@@ -221,6 +221,7 @@ struct virtio_hw { uint8_t *rss_key; uint64_t req_guest_features; struct virtnet_ctl *cvq; + bool use_va; }; struct virtio_ops {diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index b4bd1f07c1..8055be88a2 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c@@ -567,12 +567,16 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t queue_idx) memset(mz->addr, 0, mz->len); - vq->vq_ring_mem = mz->iova; + if (hw->use_va) + vq->vq_ring_mem = (uintptr_t)mz->addr; + else + vq->vq_ring_mem = mz->iova; + vq->vq_ring_virt_mem = mz->addr; PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem: 0x%" PRIx64, - (uint64_t)mz->iova); + (uint64_t)vq->vq_ring_mem);
vq_ring_mem is a rte_iova_t which is a uint64_t. Cast is unneeded.
PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%" PRIx64, - (uint64_t)(uintptr_t)mz->addr); + (uint64_t)(uintptr_t)vq->vq_ring_virt_mem);
Why not display with %p and drop casts?
quoted hunk ↗ jump to hunk
virtio_init_vring(vq);@@ -622,17 +626,28 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t queue_idx) txvq->port_id = dev->data->port_id; txvq->mz = mz; txvq->virtio_net_hdr_mz = hdr_mz; - txvq->virtio_net_hdr_mem = hdr_mz->iova; + if (hw->use_va) + txvq->virtio_net_hdr_mem = (uintptr_t)hdr_mz->addr; + else + txvq->virtio_net_hdr_mem = hdr_mz->iova; } else if (queue_type == VTNET_CQ) { cvq = &vq->cq; cvq->mz = mz; cvq->virtio_net_hdr_mz = hdr_mz; - cvq->virtio_net_hdr_mem = hdr_mz->iova; + if (hw->use_va) + cvq->virtio_net_hdr_mem = (uintptr_t)hdr_mz->addr; + else + cvq->virtio_net_hdr_mem = hdr_mz->iova; memset(cvq->virtio_net_hdr_mz->addr, 0, rte_mem_page_size()); hw->cvq = cvq; } + if (hw->use_va) + vq->mbuf_addr_offset = offsetof(struct rte_mbuf, buf_addr); + else + vq->mbuf_addr_offset = offsetof(struct rte_mbuf, buf_iova); + if (queue_type == VTNET_TQ) { struct virtio_tx_region *txr; unsigned int i;diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c index b9d7c8d18f..0f3c286438 100644 --- a/drivers/net/virtio/virtio_rxtx.c +++ b/drivers/net/virtio/virtio_rxtx.c@@ -271,10 +271,13 @@ virtqueue_enqueue_refill_inorder(struct virtqueue *vq, dxp->cookie = (void *)cookies[i]; dxp->ndescs = 1; - start_dp[idx].addr = cookies[i]->buf_iova + - RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size; - start_dp[idx].len = cookies[i]->buf_len - - RTE_PKTMBUF_HEADROOM + hw->vtnet_hdr_size; + start_dp[idx].addr = + VIRTIO_MBUF_ADDR(cookies[i], vq) + + RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
A single <tab> is enough indent.
+ start_dp[idx].len = + cookies[i]->buf_len - + RTE_PKTMBUF_HEADROOM + + hw->vtnet_hdr_size;
This part needs no update. In the end for this hunk, we only need: - start_dp[idx].addr = cookies[i]->buf_iova + + start_dp[idx].addr = VIRTIO_MBUF_ADDR(cookies[i], vq) +
quoted hunk ↗ jump to hunk
start_dp[idx].flags = VRING_DESC_F_WRITE; vq_update_avail_ring(vq, idx);@@ -310,10 +313,12 @@ virtqueue_enqueue_recv_refill(struct virtqueue *vq, struct rte_mbuf **cookie, dxp->cookie = (void *)cookie[i]; dxp->ndescs = 1; - start_dp[idx].addr = cookie[i]->buf_iova + + start_dp[idx].addr = + VIRTIO_MBUF_ADDR(cookie[i], vq) + RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size; - start_dp[idx].len = cookie[i]->buf_len - - RTE_PKTMBUF_HEADROOM + hw->vtnet_hdr_size; + start_dp[idx].len = + cookie[i]->buf_len - RTE_PKTMBUF_HEADROOM + + hw->vtnet_hdr_size; start_dp[idx].flags = VRING_DESC_F_WRITE; vq->vq_desc_head_idx = start_dp[idx].next; vq_update_avail_ring(vq, idx);
Same comment as above, we only need: - start_dp[idx].addr = cookie[i]->buf_iova + + start_dp[idx].addr = VIRTIO_MBUF_ADDR(cookie[i], vq) +
quoted hunk ↗ jump to hunk
@@ -336,7 +341,7 @@ virtqueue_refill_single_packed(struct virtqueue *vq, uint16_t flags = vq->vq_packed.cached_flags; struct virtio_hw *hw = vq->hw; - dp->addr = cookie->buf_iova + + dp->addr = VIRTIO_MBUF_ADDR(cookie, vq) + RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size; dp->len = cookie->buf_len - RTE_PKTMBUF_HEADROOM + hw->vtnet_hdr_size;@@ -482,7 +487,8 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq, else virtqueue_xmit_offload(hdr, cookies[i]); - start_dp[idx].addr = rte_mbuf_data_iova(cookies[i]) - head_size; + start_dp[idx].addr = + VIRTIO_MBUF_DATA_DMA_ADDR(cookies[i], vq) - head_size;
We could go a little over 80 columns.
quoted hunk ↗ jump to hunk
start_dp[idx].len = cookies[i]->data_len + head_size; start_dp[idx].flags = 0;@@ -529,7 +535,7 @@ virtqueue_enqueue_xmit_packed_fast(struct virtnet_tx *txvq, else virtqueue_xmit_offload(hdr, cookie); - dp->addr = rte_mbuf_data_iova(cookie) - head_size; + dp->addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq) - head_size; dp->len = cookie->data_len + head_size; dp->id = id;@@ -617,7 +623,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie, virtqueue_xmit_offload(hdr, cookie); do { - start_dp[idx].addr = rte_mbuf_data_iova(cookie); + start_dp[idx].addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq); start_dp[idx].len = cookie->data_len; if (prepend_header) { start_dp[idx].addr -= head_size;
[snip]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/virtio/virtio_rxtx_simple.h b/drivers/net/virtio/virtio_rxtx_simple.h index f258771fcf..497c9a0e32 100644 --- a/drivers/net/virtio/virtio_rxtx_simple.h +++ b/drivers/net/virtio/virtio_rxtx_simple.h@@ -43,7 +43,8 @@ virtio_rxq_rearm_vec(struct virtnet_rx *rxvq) p = (uintptr_t)&sw_ring[i]->rearm_data; *(uint64_t *)p = rxvq->mbuf_initializer; - start_dp[i].addr = sw_ring[i]->buf_iova + + start_dp[i].addr = + VIRTIO_MBUF_ADDR(sw_ring[i], vq) +
This fits in 80 columns.
RTE_PKTMBUF_HEADROOM - vq->hw->vtnet_hdr_size;
start_dp[i].len = sw_ring[i]->buf_len -
RTE_PKTMBUF_HEADROOM + vq->hw->vtnet_hdr_size;[snip] -- David Marchand