From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 22E2A1CBEE for ; Thu, 5 Apr 2018 16:24:20 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B6D454068056; Thu, 5 Apr 2018 14:24:19 +0000 (UTC) Received: from localhost (dhcp-192-241.str.redhat.com [10.33.192.241]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 39B47215CDAF; Thu, 5 Apr 2018 14:24:19 +0000 (UTC) Date: Thu, 5 Apr 2018 16:24:18 +0200 From: Jens Freimann To: Maxime Coquelin Cc: dev@dpdk.org, tiwei.bie@intel.com, yliu@fridaylinux.org, mst@redhat.com Message-ID: <20180405142418.qjbovnsxspcbr3tl@reserved-198-163.str.redhat.com> References: <20180405101031.26468-1-jfreimann@redhat.com> <20180405101031.26468-3-jfreimann@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180223 X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 05 Apr 2018 14:24:19 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 05 Apr 2018 14:24:19 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'jfreimann@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v3 02/21] 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Apr 2018 14:24:20 -0000 On Thu, Apr 05, 2018 at 04:15:56PM +0200, Maxime Coquelin wrote: > > >On 04/05/2018 12:10 PM, Jens Freimann wrote: >>Add and initialize descriptor data structures. >> >>Signed-off-by: Jens Freimann >>--- >> drivers/net/virtio/virtio_ethdev.c | 22 +++++++++------- >> drivers/net/virtio/virtio_pci.h | 8 ++++++ >> drivers/net/virtio/virtio_ring.h | 53 ++++++++++++++++++++++++++++++++++---- >> drivers/net/virtio/virtqueue.h | 10 +++++++ >> 4 files changed, 78 insertions(+), 15 deletions(-) >> >>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >>index 06fbf7311..cccefafe9 100644 >>--- a/drivers/net/virtio/virtio_ethdev.c >>+++ b/drivers/net/virtio/virtio_ethdev.c >>@@ -298,19 +298,21 @@ 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); >>+ vring_init(vq->hw, vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN); >>+ >> vq->vq_free_cnt = vq->vq_nentries; >> memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries); >>+ vq->vq_used_cons_idx = 0; >>+ vq->vq_avail_idx = 0; >>+ if (vtpci_packed_queue(vq->hw)) { >>+ vring_desc_init_packed(vr, size); >>+ } else { >>+ vq->vq_desc_head_idx = 0; >>+ vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1); >>- vring_desc_init(vr->desc, size); >>+ vring_desc_init(vr->desc, size); >>+ } >> /* >> * Disable device(host) interrupting guest >>@@ -385,7 +387,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx) >> /* >> * 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); >>diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h >>index a28ba8339..528fb46b9 100644 >>--- a/drivers/net/virtio/virtio_pci.h >>+++ b/drivers/net/virtio/virtio_pci.h >>@@ -112,6 +112,8 @@ struct virtnet_ctl; >> #define VIRTIO_F_VERSION_1 32 >> #define VIRTIO_F_IOMMU_PLATFORM 33 >>+#define VIRTIO_F_RING_PACKED 34 >>+#define VIRTIO_F_IN_ORDER 35 >> /* >> * Some VirtIO feature bits (currently bits 28 through 31) are >>@@ -304,6 +306,12 @@ vtpci_with_feature(struct virtio_hw *hw, uint64_t bit) >> return (hw->guest_features & (1ULL << bit)) != 0; >> } >>+static inline int >>+vtpci_packed_queue(struct virtio_hw *hw) >>+{ >>+ return vtpci_with_feature(hw, VIRTIO_F_RING_PACKED); >>+} >>+ >> /* >> * Function declaration from virtio_pci.c >> */ >>diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h >>index 9e3c2a015..1bd7ba98e 100644 >>--- a/drivers/net/virtio/virtio_ring.h >>+++ b/drivers/net/virtio/virtio_ring.h >>@@ -9,6 +9,7 @@ >> #include >>+ >Remove new line > >> /* This marks a buffer as continuing via the next field. */ >> #define VRING_DESC_F_NEXT 1 >> /* This marks a buffer as write-only (otherwise read-only). */ >>@@ -54,11 +55,38 @@ struct vring_used { >> struct vring_used_elem ring[0]; >> }; >>+/* For support of packed virtqueues in Virtio 1.1 the format of descriptors >>+ * looks like this. >>+ */ >>+struct vring_desc_packed { >>+ uint64_t addr; >>+ uint32_t len; >>+ uint16_t index; >>+ uint16_t flags; >>+}; >>+ >>+#define RING_EVENT_FLAGS_ENABLE 0x0 >>+#define RING_EVENT_FLAGS_DISABLE 0x1 >>+#define RING_EVENT_FLAGS_DESC 0x2 >>+struct vring_packed_desc_event { >>+ uint16_t desc_event_off_wrap; >>+ uint16_t desc_event_flags; >>+}; >>+ >> struct vring { >> unsigned int num; >>- struct vring_desc *desc; >>- struct vring_avail *avail; >>- struct vring_used *used; >>+ union { >>+ struct vring_desc_packed *desc_packed; >>+ struct vring_desc *desc; >>+ }; >>+ union { >>+ struct vring_avail *avail; >>+ struct vring_packed_desc_event *driver_event; >>+ }; >>+ union { >>+ struct vring_used *used; >>+ struct vring_packed_desc_event *device_event; >>+ }; > >Not a must, but I wonder if it wouldn't be cleaner to have a unique >union of two structs, one for packed rings, and one for the legacy. >I may be wrong, though. I'm not sure either but this looked best to me. Thanks! regards, Jens > >> }; >> /* The standard layout for the ring is a continuous chunk of memory which >>@@ -95,10 +123,16 @@ 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_desc_packed); >>+ size += 2 * 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); >>@@ -108,10 +142,19 @@ vring_size(unsigned int num, unsigned long align) >> } >> static inline void >>-vring_init(struct vring *vr, unsigned int num, uint8_t *p, >>+vring_init(struct virtio_hw *hw, struct vring *vr, unsigned int num, uint8_t *p, >> unsigned long align) >> { >> vr->num = num; >>+ if (vtpci_packed_queue(hw)) { >>+ vr->desc_packed = (struct vring_desc_packed *)p; >>+ vr->driver_event = (struct vring_packed_desc_event *) (p + >>+ num * sizeof(struct vring_desc_packed)); >>+ vr->device_event = (struct vring_packed_desc_event *) (vr->driver_event + >>+ sizeof(struct vring_packed_desc_event)); >>+ return; >>+ } >>+ >> vr->desc = (struct vring_desc *) p; >> vr->avail = (struct vring_avail *) (p + >> num * sizeof(struct vring_desc)); >>diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h >>index 14364f356..cc2e7c0f6 100644 >>--- a/drivers/net/virtio/virtqueue.h >>+++ b/drivers/net/virtio/virtqueue.h >>@@ -245,6 +245,16 @@ struct virtio_tx_region { >> __attribute__((__aligned__(16))); >> }; >>+static inline void >>+vring_desc_init_packed(struct vring *vr, int n) >>+{ >>+ int i; >>+ for (i = 0; i < n; i++) { >>+ struct vring_desc_packed *desc = &vr->desc_packed[i]; >>+ desc->index = i; >>+ } >>+} >>+ >> /* Chain all the descriptors in the ring with an END */ >> static inline void >> vring_desc_init(struct vring_desc *dp, uint16_t n) >>