DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: David Marchand <david.marchand@redhat.com>
Cc: Olivier Matz <olivier.matz@6wind.com>,
	"Xia, Chenbo" <chenbo.xia@intel.com>, dev <dev@dpdk.org>,
	dpdk stable <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/virtio: revert forcing IOVA as VA mode for virtio-user
Date: Thu, 30 Sep 2021 09:43:45 +0200	[thread overview]
Message-ID: <afa29174-928a-8e86-f4fd-935ba8e95e82@redhat.com> (raw)
In-Reply-To: <CAJFAV8znJhstJ0cs9NWecpwdhB4znJPbBctk9OEAgcu2h8dDvw@mail.gmail.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


      reply	other threads:[~2021-09-30  7:44 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-29 20:17 Maxime Coquelin
2021-09-29 21:15 ` Olivier Matz
2021-09-30  8:25   ` Maxime Coquelin
2021-09-30  7:26 ` David Marchand
2021-09-30  7:43   ` Maxime Coquelin [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=afa29174-928a-8e86-f4fd-935ba8e95e82@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).