* [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed
@ 2019-01-09 16:59 Jens Freimann
2019-01-10 8:46 ` Maxime Coquelin
0 siblings, 1 reply; 3+ messages in thread
From: Jens Freimann @ 2019-01-09 16:59 UTC (permalink / raw)
To: dev; +Cc: tiwei.bie, maxime.coquelin
Add support to virtio-user for control virtqueues
and reverts commit "5e4e7a752 net/virtio-user: fail if
cq used with packed vq".
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);
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++;
+ n_descs++;
+ }
+
+ /* locate desc for status */
+ idx_status = i;
+ 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;
+ 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;
+ 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;
+ }
+ }
+}
+
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;
}
--
2.17.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed
2019-01-09 16:59 [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed Jens Freimann
@ 2019-01-10 8:46 ` Maxime Coquelin
2019-01-10 9:14 ` Jens Freimann
0 siblings, 1 reply; 3+ messages in thread
From: Maxime Coquelin @ 2019-01-10 8:46 UTC (permalink / raw)
To: Jens Freimann, dev; +Cc: tiwei.bie
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;
> + }
> + }
> +}
> +
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed
2019-01-10 8:46 ` Maxime Coquelin
@ 2019-01-10 9:14 ` Jens Freimann
0 siblings, 0 replies; 3+ messages in thread
From: Jens Freimann @ 2019-01-10 9:14 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, tiwei.bie
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 <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?
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-01-10 9:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09 16:59 [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed Jens Freimann
2019-01-10 8:46 ` Maxime Coquelin
2019-01-10 9:14 ` Jens Freimann
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).