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 348FF1B142 for ; Thu, 10 Jan 2019 09:46:44 +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 7A1D837E8E; Thu, 10 Jan 2019 08:46:43 +0000 (UTC) Received: from [10.36.112.17] (ovpn-112-17.ams2.redhat.com [10.36.112.17]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D0C1D67160; Thu, 10 Jan 2019 08:46:39 +0000 (UTC) To: Jens Freimann , dev@dpdk.org Cc: tiwei.bie@intel.com References: <20190109165941.26622-1-jfreimann@redhat.com> From: Maxime Coquelin Message-ID: Date: Thu, 10 Jan 2019 09:46:37 +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: <20190109165941.26622-1-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]); Thu, 10 Jan 2019 08:46:43 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] 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 08:46:44 -0000 Hi Jens, On 1/9/19 5:59 PM, Jens Freimann wrote: > Add support to virtio-user for control virtqueues > and reverts commit "5e4e7a752 net/virtio-user: fail if > cq used with packed vq". I would prefer you send two patches: 1. add ctrl vq support 2. revert 5e4e7a752 > > Signed-off-by: Jens Freimann > --- > .../net/virtio/virtio_user/virtio_user_dev.c | 111 ++++++++++++++++-- > .../net/virtio/virtio_user/virtio_user_dev.h | 8 +- > drivers/net/virtio/virtio_user_ethdev.c | 49 +++++++- > 3 files changed, 151 insertions(+), 17 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..95f66d5b4 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); > @@ -467,15 +478,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > if (!in_order) > dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER); > > - if (packed_vq) { > - if (cq) { > - PMD_INIT_LOG(ERR, "control vq not supported yet with " > - "packed virtqueues\n"); > - return -1; > - } > - } else { > - dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED); > - } > + if (packed_vq) > + dev->device_features |= (1ull << VIRTIO_F_RING_PACKED); > + else > + dev->device_features &= ~(1ull << VIRTIO_F_RING_PACKED); Shouldn't you set VIRTIO_F_RING_PACKED bit in unsupported features? > > if (dev->mac_specified) > dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC); > @@ -620,6 +626,87 @@ 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 i, idx_data, idx_status; > + uint32_t n_descs = 0; > + > + /* locate desc for header, data, and status */ > + idx_data = idx_hdr + 1; > + n_descs++; > + > + i = idx_data; > + while (vring->desc_packed[i].flags & VRING_DESC_F_NEXT) { > + i++; Why not incrementing idx_status directly? > + n_descs++; > + } > + > + /* locate desc for status */ > + idx_status = i; > + n_descs++; I think it would be clearer to initialize n_decsc to 1, as it could correspond to the header (and add a comment about it). > + > + 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; > + bool wrap_counter = vq->used_wrap_counter; > + > + 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; > + n = vq->vq_used_cons_idx + n_descs - 1; I think it is broken as it needs to handle wrapping. > + while (n >= vq->vq_used_cons_idx) { > + vring->desc_packed[n].flags |= > + VRING_DESC_F_AVAIL(wrap_counter) | > + VRING_DESC_F_USED(wrap_counter); > + n--; > + } > + vq->vq_used_cons_idx += n_descs; > + if (vq->vq_used_cons_idx >= dev->queue_size) { > + vq->vq_used_cons_idx -= dev->queue_size; > + vq->used_wrap_counter ^= 1; > + } > + } > +} > +