Re: [dpdk-dev] [PATCH] net/virtio: revert forcing IOVA as VA mode for virtio-user

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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.coquelin@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: stable@dpdk.org
>>
>> Reported-by: Olivier Matz <olivier.matz@6wind.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@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