From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4D9F3A0C41; Thu, 30 Sep 2021 09:44:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8AD8D410EE; Thu, 30 Sep 2021 09:44:16 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id 2CFBE410EA for ; Thu, 30 Sep 2021 09:44:15 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1632987854; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sWwmX76mdc3RIn8/dE820+jsVseOYgKbGDfKNzlwLXA=; b=W7YArpK+GA0kajJ9VsfIHYNO3LiqGnc3FJLIpPrmHtsbBzQvwz56qvYcpBGKXfyukRLqIi fGh90ndvMgNr7q4NyNF8Zfzb/j5KsSwj/cMWn2GdFpj+pb9dYFT/YE8W8sY6fnknD3JhUK 9yodRBNqzrrRaOj21vJilsDBv0ZZRcg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-305--3jqpVlxP-aO2avDqb_AGA-1; Thu, 30 Sep 2021 03:44:12 -0400 X-MC-Unique: -3jqpVlxP-aO2avDqb_AGA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 49751108468B; Thu, 30 Sep 2021 07:44:11 +0000 (UTC) Received: from [10.39.208.6] (unknown [10.39.208.6]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 26A3327073; Thu, 30 Sep 2021 07:43:48 +0000 (UTC) Message-ID: Date: Thu, 30 Sep 2021 09:43:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0 To: David Marchand Cc: Olivier Matz , "Xia, Chenbo" , dev , dpdk stable References: <20210929201739.176306-1-maxime.coquelin@redhat.com> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] net/virtio: revert forcing IOVA as VA mode for virtio-user X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi David, On 9/30/21 09:26, David Marchand wrote: > Hello Maxime, > > > On Wed, Sep 29, 2021 at 10:18 PM Maxime Coquelin > 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 >> Signed-off-by: Maxime Coquelin > > 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 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