From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: Jens Freimann <jfreimann@redhat.com>, dev@dpdk.org
Cc: tiwei.bie@intel.com
Subject: Re: [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed
Date: Thu, 10 Jan 2019 09:46:37 +0100 [thread overview]
Message-ID: <d905d2cf-bf4c-1f36-e64f-65c6eda6d319@redhat.com> (raw)
In-Reply-To: <20190109165941.26622-1-jfreimann@redhat.com>
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 <jfreimann@redhat.com>
> ---
> .../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;
> + }
> + }
> +}
> +
next prev parent reply other threads:[~2019-01-10 8:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-09 16:59 Jens Freimann
2019-01-10 8:46 ` Maxime Coquelin [this message]
2019-01-10 9:14 ` Jens Freimann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d905d2cf-bf4c-1f36-e64f-65c6eda6d319@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=dev@dpdk.org \
--cc=jfreimann@redhat.com \
--cc=tiwei.bie@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).