On Thu, Jan 10, 2019 at 09:46:37AM +0100, Maxime Coquelin wrote:
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
ok
Signed-off-by: Jens Freimann <jfreim...@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?
I'll fix it
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?
I'll do that
+ 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).
ok
+
+ 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.
I think you're right. I'll fix it.
thanks for the review!
regards,
Jens
