From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id A57B31B87F for ; Thu, 10 Jan 2019 16:04:39 +0100 (CET) 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 mx1.redhat.com (Postfix) with ESMTPS id C222CC0BEAA2; Thu, 10 Jan 2019 15:04:38 +0000 (UTC) Received: from localhost (unknown [10.36.118.36]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1134D282C3; Thu, 10 Jan 2019 15:04:32 +0000 (UTC) Date: Thu, 10 Jan 2019 16:04:31 +0100 From: Jens Freimann To: Maxime Coquelin Cc: dev@dpdk.org, tiwei.bie@intel.com Message-ID: <20190110150431.55xetbxl54mt6k6m@jenstp.localdomain> References: <20190110131751.32670-1-jfreimann@redhat.com> <20190110131751.32670-2-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/20180716 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 10 Jan 2019 15:04:39 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/virtio-user: ctrl vq support for packed 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, 10 Jan 2019 15:04:40 -0000 On Thu, Jan 10, 2019 at 03:23:29PM +0100, Maxime Coquelin wrote: > > >On 1/10/19 2:17 PM, Jens Freimann wrote: >>Add support to virtio-user for control virtqueues. >> >>Signed-off-by: Jens Freimann >>--- >> .../net/virtio/virtio_user/virtio_user_dev.c | 92 ++++++++++++++++++- >> .../net/virtio/virtio_user/virtio_user_dev.h | 8 +- >> drivers/net/virtio/virtio_user_ethdev.c | 49 +++++++++- >> 3 files changed, 141 insertions(+), 8 deletions(-) >> >>diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>index b9044faff..49fcf48b9 100644 >>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>@@ -43,15 +43,26 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel) >> struct vhost_vring_file file; >> struct vhost_vring_state state; >> struct vring *vring = &dev->vrings[queue_sel]; >>+ struct vring_packed *pq_vring = &dev->packed_vrings[queue_sel]; >> struct vhost_vring_addr addr = { >> .index = queue_sel, >>- .desc_user_addr = (uint64_t)(uintptr_t)vring->desc, >>- .avail_user_addr = (uint64_t)(uintptr_t)vring->avail, >>- .used_user_addr = (uint64_t)(uintptr_t)vring->used, >> .log_guest_addr = 0, >> .flags = 0, /* disable log */ >> }; >>+ if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) { >>+ addr.desc_user_addr = >>+ (uint64_t)(uintptr_t)pq_vring->desc_packed; >>+ addr.avail_user_addr = >>+ (uint64_t)(uintptr_t)pq_vring->driver_event; >>+ addr.used_user_addr = >>+ (uint64_t)(uintptr_t)pq_vring->device_event; >>+ } else { >>+ addr.desc_user_addr = (uint64_t)(uintptr_t)vring->desc; >>+ addr.avail_user_addr = (uint64_t)(uintptr_t)vring->avail; >>+ addr.used_user_addr = (uint64_t)(uintptr_t)vring->used; >>+ } >>+ >> state.index = queue_sel; >> state.num = vring->num; >> dev->ops->send_request(dev, VHOST_USER_SET_VRING_NUM, &state); >>@@ -620,6 +631,81 @@ virtio_user_handle_ctrl_msg(struct virtio_user_dev *dev, struct vring *vring, >> return n_descs; >> } >>+static inline int >>+desc_is_avail(struct vring_packed_desc *desc, bool wrap_counter) >>+{ >>+ return wrap_counter == !!(desc->flags & VRING_DESC_F_AVAIL(1)) && >>+ wrap_counter != !!(desc->flags & VRING_DESC_F_USED(1)); >>+} >>+ >>+static uint32_t >>+virtio_user_handle_ctrl_msg_pq(struct virtio_user_dev *dev, >>+ struct vring_packed *vring, >>+ uint16_t idx_hdr) >>+{ >>+ struct virtio_net_ctrl_hdr *hdr; >>+ virtio_net_ctrl_ack status = ~0; >>+ uint16_t idx_data, idx_status; >>+ /* initialize to one, header is first */ >>+ uint32_t n_descs = 1; >>+ >>+ /* locate desc for header, data, and status */ >>+ idx_data = idx_hdr + 1; >>+ n_descs++; >>+ >>+ idx_status = idx_data; >>+ while (vring->desc_packed[idx_status].flags & VRING_DESC_F_NEXT) { >>+ idx_status++; >>+ n_descs++; >>+ } >>+ >>+ hdr = (void *)(uintptr_t)vring->desc_packed[idx_hdr].addr; >>+ if (hdr->class == VIRTIO_NET_CTRL_MQ && >>+ hdr->cmd == VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET) { >>+ uint16_t queues; >>+ >>+ queues = *(uint16_t *)(uintptr_t) >>+ vring->desc_packed[idx_data].addr; >>+ status = virtio_user_handle_mq(dev, queues); >>+ } >>+ >>+ /* Update status */ >>+ *(virtio_net_ctrl_ack *)(uintptr_t) >>+ vring->desc_packed[idx_status].addr = status; >>+ >>+ return n_descs; >>+} >>+ >>+void >>+virtio_user_handle_cq_packed(struct virtio_user_dev *dev, uint16_t queue_idx, >>+ struct virtqueue *vq) >>+{ >>+ struct vring_packed *vring = &dev->packed_vrings[queue_idx]; >>+ uint16_t id, n_descs; >>+ int16_t n; >>+ >>+ while (desc_is_avail(&vring->desc_packed[vq->vq_used_cons_idx], >>+ vq->avail_wrap_counter)) { >>+ id = vring->desc_packed[vq->vq_used_cons_idx].id; >>+ >>+ n_descs = virtio_user_handle_ctrl_msg_pq(dev, vring, id); >>+ >>+ vring->desc_packed[vq->vq_used_cons_idx].len = n_descs; >>+ vring->desc_packed[vq->vq_used_cons_idx].id = >>+ vq->vq_used_cons_idx; >Are the above assignments necessary? I don't think so. The device could use this to skip n descriptors, but only if we do in-order and we don't use the id field. So, yes I agree it's not necessary. >>+ n = vq->vq_used_cons_idx + n_descs - 1; >>+ do { >>+ vring->desc_packed[vq->vq_used_cons_idx].flags |= >>+ VRING_DESC_F_AVAIL(vq->used_wrap_counter) | >>+ VRING_DESC_F_USED(vq->used_wrap_counter); >>+ if (vq->vq_used_cons_idx >= dev->queue_size) { >>+ vq->vq_used_cons_idx -= dev->queue_size; >>+ vq->used_wrap_counter ^= 1; >>+ } >>+ } while (++vq->vq_used_cons_idx <= n); > >I think it still does not work. >In case of wrap, n will be greater than queue size. >But as vq->vq_used_cons_idx wraps, I guess there will be an endless >loop. Now I get what you meant :) yes, I'll fix it. regards, Jens > >>+ } >>+} >>+ >> void >> virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx) >> { >>diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h >>index 672a8161a..8be8ca622 100644 >>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h >>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h >>@@ -39,7 +39,11 @@ struct virtio_user_dev { >> uint16_t port_id; >> uint8_t mac_addr[ETHER_ADDR_LEN]; >> char path[PATH_MAX]; >>- struct vring vrings[VIRTIO_MAX_VIRTQUEUES]; >>+ union { >>+ struct vring vrings[VIRTIO_MAX_VIRTQUEUES]; >>+ struct vring_packed packed_vrings[VIRTIO_MAX_VIRTQUEUES]; >>+ }; >>+ >> struct virtio_user_backend_ops *ops; >> pthread_mutex_t mutex; >> bool started; >>@@ -53,5 +57,7 @@ int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, >> int mrg_rxbuf, int in_order, int packed_vq); >> void virtio_user_dev_uninit(struct virtio_user_dev *dev); >> void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx); >>+void virtio_user_handle_cq_packed(struct virtio_user_dev *dev, >>+ uint16_t queue_idx, struct virtqueue *vq); >> uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs); >> #endif >>diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c >>index 2df6eb695..efc375cd7 100644 >>--- a/drivers/net/virtio/virtio_user_ethdev.c >>+++ b/drivers/net/virtio/virtio_user_ethdev.c >>@@ -271,10 +271,36 @@ virtio_user_get_queue_num(struct virtio_hw *hw, uint16_t queue_id __rte_unused) >> return dev->queue_size; >> } >>-static int >>-virtio_user_setup_queue(struct virtio_hw *hw, struct virtqueue *vq) >>+static void >>+virtio_user_setup_queue_packed(struct virtqueue *vq, >>+ struct virtio_user_dev *dev) >>+ >>+{ >>+ uint16_t queue_idx = vq->vq_queue_index; >>+ uint64_t desc_addr; >>+ uint64_t avail_addr; >>+ uint64_t used_addr; >>+ >>+ desc_addr = (uintptr_t)vq->vq_ring_virt_mem; >>+ avail_addr = desc_addr + vq->vq_nentries * >>+ sizeof(struct vring_packed_desc); >>+ used_addr = RTE_ALIGN_CEIL(avail_addr + >>+ sizeof(struct vring_packed_desc_event), >>+ VIRTIO_PCI_VRING_ALIGN); >>+ dev->packed_vrings[queue_idx].num = vq->vq_nentries; >>+ dev->packed_vrings[queue_idx].desc_packed = >>+ (void *)(uintptr_t)desc_addr; >>+ dev->packed_vrings[queue_idx].driver_event = >>+ (void *)(uintptr_t)avail_addr; >>+ dev->packed_vrings[queue_idx].device_event = >>+ (void *)(uintptr_t)used_addr; >>+ vq->avail_wrap_counter = true; >>+ vq->used_wrap_counter = true; >>+} >>+ >>+static void >>+virtio_user_setup_queue_split(struct virtqueue *vq, struct virtio_user_dev *dev) >> { >>- struct virtio_user_dev *dev = virtio_user_get_dev(hw); >> uint16_t queue_idx = vq->vq_queue_index; >> uint64_t desc_addr, avail_addr, used_addr; >>@@ -288,6 +314,17 @@ virtio_user_setup_queue(struct virtio_hw *hw, struct virtqueue *vq) >> dev->vrings[queue_idx].desc = (void *)(uintptr_t)desc_addr; >> dev->vrings[queue_idx].avail = (void *)(uintptr_t)avail_addr; >> dev->vrings[queue_idx].used = (void *)(uintptr_t)used_addr; >>+} >>+ >>+static int >>+virtio_user_setup_queue(struct virtio_hw *hw, struct virtqueue *vq) >>+{ >>+ struct virtio_user_dev *dev = virtio_user_get_dev(hw); >>+ >>+ if (vtpci_packed_queue(hw)) >>+ virtio_user_setup_queue_packed(vq, dev); >>+ else >>+ virtio_user_setup_queue_split(vq, dev); >> return 0; >> } >>@@ -317,7 +354,11 @@ virtio_user_notify_queue(struct virtio_hw *hw, struct virtqueue *vq) >> struct virtio_user_dev *dev = virtio_user_get_dev(hw); >> if (hw->cvq && (hw->cvq->vq == vq)) { >>- virtio_user_handle_cq(dev, vq->vq_queue_index); >>+ if (vtpci_packed_queue(vq->hw)) >>+ virtio_user_handle_cq_packed(dev, vq->vq_queue_index, >>+ vq); >>+ else >>+ virtio_user_handle_cq(dev, vq->vq_queue_index); >> return; >> } >>