From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <maxime.coquelin@redhat.com>
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
by dpdk.org (Postfix) with ESMTP id 82EA81B119
for <dev@dpdk.org>; Mon, 17 Dec 2018 17:15:41 +0100 (CET)
Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com
[10.5.11.12])
(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
(No client certificate requested)
by mx1.redhat.com (Postfix) with ESMTPS id A88A113AA0;
Mon, 17 Dec 2018 16:15:40 +0000 (UTC)
Received: from [10.36.112.10] (unknown [10.36.112.10])
by smtp.corp.redhat.com (Postfix) with ESMTPS id CB5D651DF5;
Mon, 17 Dec 2018 16:15:16 +0000 (UTC)
To: Jens Freimann <jfreimann@redhat.com>, dev@dpdk.org
Cc: tiwei.bie@intel.com, Gavin.Hu@arm.com
References: <20181214155916.1142-1-jfreimann@redhat.com>
<20181214155916.1142-4-jfreimann@redhat.com>
From: Maxime Coquelin <maxime.coquelin@redhat.com>
Message-ID: <cea88094-4924-f007-56b8-34087d0273a1@redhat.com>
Date: Mon, 17 Dec 2018 17:15:14 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
Thunderbird/60.3.1
MIME-Version: 1.0
In-Reply-To: <20181214155916.1142-4-jfreimann@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16
(mx1.redhat.com [10.5.110.29]); Mon, 17 Dec 2018 16:15:40 +0000 (UTC)
Subject: Re: [dpdk-dev] [PATCH v13 03/10] net/virtio: vring init for packed
queues
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 17 Dec 2018 16:15:41 -0000
On 12/14/18 4:59 PM, Jens Freimann wrote:
> Add and initialize descriptor data structures.
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 32 ++++++++++++++++++----------
> drivers/net/virtio/virtio_ring.h | 34 ++++++++++++++++++++++++------
> drivers/net/virtio/virtqueue.h | 2 +-
> 3 files changed, 49 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index cb2b2e0bf..e6ba1282b 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -299,20 +299,22 @@ virtio_init_vring(struct virtqueue *vq)
>
> PMD_INIT_FUNC_TRACE();
>
> - /*
> - * Reinitialise since virtio port might have been stopped and restarted
> - */
> memset(ring_mem, 0, vq->vq_ring_size);
> - vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
> +
> vq->vq_used_cons_idx = 0;
> vq->vq_desc_head_idx = 0;
> vq->vq_avail_idx = 0;
> vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
> vq->vq_free_cnt = vq->vq_nentries;
> memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
> -
> - vring_desc_init(vr->desc, size);
> -
> + if (vtpci_packed_queue(vq->hw)) {
> + vring_init_packed(&vq->ring_packed, ring_mem,
> + VIRTIO_PCI_VRING_ALIGN, size);
> + vring_desc_init_packed(vq, size);
> + } else {
> + vring_init_split(vr, ring_mem, VIRTIO_PCI_VRING_ALIGN, size);
> + vring_desc_init_split(vr->desc, size);
> + }
> /*
> * Disable device(host) interrupting guest
> */
> @@ -384,11 +386,16 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
> vq->hw = hw;
> vq->vq_queue_index = vtpci_queue_idx;
> vq->vq_nentries = vq_size;
> + vq->event_flags_shadow = 0;
> + if (vtpci_packed_queue(hw)) {
> + vq->avail_wrap_counter = 1;
> + vq->used_wrap_counter = 1;
> + }
>
> /*
> * Reserve a memzone for vring elements
> */
> - size = vring_size(vq_size, VIRTIO_PCI_VRING_ALIGN);
> + size = vring_size(hw, vq_size, VIRTIO_PCI_VRING_ALIGN);
> vq->vq_ring_size = RTE_ALIGN_CEIL(size, VIRTIO_PCI_VRING_ALIGN);
> PMD_INIT_LOG(DEBUG, "vring_size: %d, rounded_vring_size: %d",
> size, vq->vq_ring_size);
> @@ -491,7 +498,8 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
> for (i = 0; i < vq_size; i++) {
> struct vring_desc *start_dp = txr[i].tx_indir;
>
> - vring_desc_init(start_dp, RTE_DIM(txr[i].tx_indir));
> + vring_desc_init_split(start_dp,
> + RTE_DIM(txr[i].tx_indir));
>
> /* first indirect descriptor is always the tx header */
> start_dp->addr = txvq->virtio_net_hdr_mem
> @@ -1488,7 +1496,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>
> /* Setting up rx_header size for the device */
> if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
> - vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
> + vtpci_with_feature(hw, VIRTIO_F_VERSION_1) ||
> + vtpci_with_feature(hw, VIRTIO_F_RING_PACKED))
> hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> else
> hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr);
> @@ -1908,7 +1917,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>
> if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) {
> hw->use_inorder_tx = 1;
> - if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
> + if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) &&
> + !vtpci_packed_queue(hw)) {
> hw->use_inorder_rx = 1;
> hw->use_simple_rx = 0;
> } else {
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index 464449074..9ab007c11 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -86,7 +86,7 @@ struct vring_packed {
>
> struct vring {
> unsigned int num;
> - struct vring_desc *desc;
> + struct vring_desc *desc;
Unrelated change, please revert back.
> struct vring_avail *avail;
> struct vring_used *used;
> };
> @@ -125,10 +125,18 @@ struct vring {
> #define vring_avail_event(vr) (*(uint16_t *)&(vr)->used->ring[(vr)->num])
>
> static inline size_t
> -vring_size(unsigned int num, unsigned long align)
> +vring_size(struct virtio_hw *hw, unsigned int num, unsigned long align)
> {
> size_t size;
>
> + if (vtpci_packed_queue(hw)) {
> + size = num * sizeof(struct vring_packed_desc);
> + size += sizeof(struct vring_packed_desc_event);
> + size = RTE_ALIGN_CEIL(size, align);
> + size += sizeof(struct vring_packed_desc_event);
> + return size;
> + }
> +
> size = num * sizeof(struct vring_desc);
> size += sizeof(struct vring_avail) + (num * sizeof(uint16_t));
> size = RTE_ALIGN_CEIL(size, align);
> @@ -136,17 +144,29 @@ vring_size(unsigned int num, unsigned long align)
> (num * sizeof(struct vring_used_elem));
> return size;
> }
> -
> static inline void
> -vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> - unsigned long align)
> +vring_init_split(struct vring *vr, uint8_t *p, unsigned long align,
> + unsigned int num)
> {
> vr->num = num;
> vr->desc = (struct vring_desc *) p;
> vr->avail = (struct vring_avail *) (p +
> - num * sizeof(struct vring_desc));
> + vr->num * sizeof(struct vring_desc));
> vr->used = (void *)
> - RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> + RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[vr->num]), align);
> +}
Not a big deal, but it is unrelated to patch purpose.
> +
> +static inline void
> +vring_init_packed(struct vring_packed *vr, uint8_t *p, unsigned long align,
> + unsigned int num)
> +{
> + vr->num = num;
> + vr->desc_packed = (struct vring_packed_desc *)p;
> + vr->driver_event = (struct vring_packed_desc_event *)(p +
> + vr->num * sizeof(struct vring_packed_desc));
> + vr->device_event = (struct vring_packed_desc_event *)
> + RTE_ALIGN_CEIL((uintptr_t)(vr->driver_event +
> + sizeof(struct vring_packed_desc_event)), align);
> }
>
> /*
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 19fabae0b..809d15879 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -291,7 +291,7 @@ vring_desc_init_packed(struct virtqueue *vq, int n)
>
> /* Chain all the descriptors in the ring with an END */
> static inline void
> -vring_desc_init(struct vring_desc *dp, uint16_t n)
> +vring_desc_init_split(struct vring_desc *dp, uint16_t n)
> {
> uint16_t i;
>
>
With above minor comments fixed:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime