Hi David,
On 9/30/21 09:26, David Marchand wrote:
Hello Maxime,
On Wed, Sep 29, 2021 at 10:18 PM Maxime Coquelin
<maxime.coque...@redhat.com> 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: sta...@dpdk.org
Reported-by: Olivier Matz <olivier.m...@6wind.com>
Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
This patch does not apply on next-virtio, but you are best placed to
figure this out :-).
:) I can confirm, I missed my RSS series in between.
Quick look, only nits otherwise patch lgtm.
---
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?
Agree, I'll rework these undeed casts.
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.
Yes, clean-ups I did got re-introduced with the revert.
I will rework them in next revision (and will add a few more cleanups I
missed initially).
Thanks,
Maxime
