[v7,1/8] net/virtio: vring init for packed queues
Checks
Commit Message
Add and initialize descriptor data structures.
To allow out of order processing a .next field was added to
struct vq_desc_extra because there is none in the packed virtqueue
descriptor itself. This is used to chain descriptors and process them
similiar to how it is handled for split virtqueues.
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
drivers/net/virtio/virtio_pci.h | 8 +++++
drivers/net/virtio/virtio_ring.h | 55 +++++++++++++++++++++++++++---
drivers/net/virtio/virtqueue.h | 13 ++++++-
4 files changed, 88 insertions(+), 16 deletions(-)
Comments
On 10/03/2018 03:11 PM, Jens Freimann wrote:
> Add and initialize descriptor data structures.
>
> To allow out of order processing a .next field was added to
> struct vq_desc_extra because there is none in the packed virtqueue
> descriptor itself. This is used to chain descriptors and process them
> similiar to how it is handled for split virtqueues.
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
> drivers/net/virtio/virtio_pci.h | 8 +++++
> drivers/net/virtio/virtio_ring.h | 55 +++++++++++++++++++++++++++---
> drivers/net/virtio/virtqueue.h | 13 ++++++-
> 4 files changed, 88 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index b81df0a99..d6a1613dd 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -299,19 +299,27 @@ virtio_init_vring(struct virtqueue *vq)
>
> PMD_INIT_FUNC_TRACE();
>
> - /*
> - * Reinitialise since virtio port might have been stopped and restarted
> - */
> memset(ring_mem, 0, vq->vq_ring_size);
> - vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
> + vring_init(vq->hw, vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
> +
> + vq->vq_free_cnt = vq->vq_nentries;
> + memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
> vq->vq_used_cons_idx = 0;
> + vq->vq_avail_idx = 0;
> vq->vq_desc_head_idx = 0;
> - vq->vq_avail_idx = 0;
> vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
> - vq->vq_free_cnt = vq->vq_nentries;
> - memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
> + if (vtpci_packed_queue(vq->hw)) {
> + uint16_t i;
> + for(i = 0; i < size - 1; i++) {
> + vq->vq_descx[i].next = i + 1;
I would move it in a dedicated loop, and do it only if IN_ORDER hasn't
been negotiated. Not for performance reason of course, but just to
highlight that this extra stuff isn't needed with in-order.
> + vq->vq_ring.desc_packed[i].index = i;
I would use the vring_desc_init_packed function declared below instead.
> + }
Trailing space.
> + vq->vq_ring.desc_packed[i].index = i;
> + vq->vq_descx[i].next = VQ_RING_DESC_CHAIN_END;
> + } else {
>
> - vring_desc_init(vr->desc, size);
> + vring_desc_init_split(vr->desc, size);
> + }
>
> /*
> * Disable device(host) interrupting guest
> @@ -386,7 +394,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
> /*
> * Reserve a memzone for vring elements
> */
> - size = vring_size(vq_size, VIRTIO_PCI_VRING_ALIGN);
> + size = vring_size(hw, vq_size, VIRTIO_PCI_VRING_ALIGN);
> vq->vq_ring_size = RTE_ALIGN_CEIL(size, VIRTIO_PCI_VRING_ALIGN);
> PMD_INIT_LOG(DEBUG, "vring_size: %d, rounded_vring_size: %d",
> size, vq->vq_ring_size);
> @@ -489,7 +497,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
> for (i = 0; i < vq_size; i++) {
> struct vring_desc *start_dp = txr[i].tx_indir;
>
> - vring_desc_init(start_dp, RTE_DIM(txr[i].tx_indir));
> + vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir));
>
> /* first indirect descriptor is always the tx header */
> start_dp->addr = txvq->virtio_net_hdr_mem
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index 58fdd3d45..90204d281 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -113,6 +113,8 @@ struct virtnet_ctl;
>
> #define VIRTIO_F_VERSION_1 32
> #define VIRTIO_F_IOMMU_PLATFORM 33
> +#define VIRTIO_F_RING_PACKED 34
> +#define VIRTIO_F_IN_ORDER 35
Isn't that feature already declared?
>
> /*
> * Some VirtIO feature bits (currently bits 28 through 31) are
> @@ -314,6 +316,12 @@ vtpci_with_feature(struct virtio_hw *hw, uint64_t bit)
> return (hw->guest_features & (1ULL << bit)) != 0;
> }
>
> +static inline int
> +vtpci_packed_queue(struct virtio_hw *hw)
> +{
> + return vtpci_with_feature(hw, VIRTIO_F_RING_PACKED);
> +}
> +
> /*
> * Function declaration from virtio_pci.c
> */
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index 9e3c2a015..309069fdb 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -54,11 +54,38 @@ struct vring_used {
> struct vring_used_elem ring[0];
> };
>
> +/* For support of packed virtqueues in Virtio 1.1 the format of descriptors
> + * looks like this.
> + */
> +struct vring_desc_packed {
> + uint64_t addr;
> + uint32_t len;
> + uint16_t index;
> + uint16_t flags;
> +} __attribute__ ((aligned(16)));
> +
> +#define RING_EVENT_FLAGS_ENABLE 0x0
> +#define RING_EVENT_FLAGS_DISABLE 0x1
> +#define RING_EVENT_FLAGS_DESC 0x2
> +struct vring_packed_desc_event {
> + uint16_t desc_event_off_wrap;
> + uint16_t desc_event_flags;
> +};
> +
> struct vring {
> unsigned int num;
> - struct vring_desc *desc;
> - struct vring_avail *avail;
> - struct vring_used *used;
> + union {
> + struct vring_desc_packed *desc_packed;
> + struct vring_desc *desc;
> + };
> + union {
> + struct vring_avail *avail;
> + struct vring_packed_desc_event *driver_event;
> + };
> + union {
> + struct vring_used *used;
> + struct vring_packed_desc_event *device_event;
> + };
> };
>
> /* The standard layout for the ring is a continuous chunk of memory which
> @@ -95,10 +122,18 @@ struct vring {
> #define vring_avail_event(vr) (*(uint16_t *)&(vr)->used->ring[(vr)->num])
>
> static inline size_t
> -vring_size(unsigned int num, unsigned long align)
> +vring_size(struct virtio_hw *hw, unsigned int num, unsigned long align)
> {
> size_t size;
>
> + if (vtpci_packed_queue(hw)) {
> + size = num * sizeof(struct vring_desc_packed);
> + size += sizeof(struct vring_packed_desc_event);
> + size = RTE_ALIGN_CEIL(size, align);
> + size += sizeof(struct vring_packed_desc_event);
> + return size;
> + }
> +
> size = num * sizeof(struct vring_desc);
> size += sizeof(struct vring_avail) + (num * sizeof(uint16_t));
> size = RTE_ALIGN_CEIL(size, align);
> @@ -108,10 +143,20 @@ vring_size(unsigned int num, unsigned long align)
> }
>
> static inline void
> -vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> +vring_init(struct virtio_hw *hw, struct vring *vr, unsigned int num, uint8_t *p,
> unsigned long align)
> {
> vr->num = num;
> + if (vtpci_packed_queue(hw)) {
> + vr->desc_packed = (struct vring_desc_packed *)p;
> + vr->driver_event = (struct vring_packed_desc_event *)(p +
> + num * sizeof(struct vring_desc_packed));
> + vr->device_event = (struct vring_packed_desc_event *)
> + RTE_ALIGN_CEIL((uintptr_t)(vr->driver_event +
> + sizeof(struct vring_packed_desc_event)), align);
> + return;
> + }
> +
As a general comment, I would find it cleaner to have dedicated
functions for split and packed variants, like vring_init_split,
vring_init_packed, etc...
> vr->desc = (struct vring_desc *) p;
> vr->avail = (struct vring_avail *) (p +
> num * sizeof(struct vring_desc));
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 26518ed98..6a4f92b79 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
> struct vq_desc_extra {
> void *cookie;
> uint16_t ndescs;
> + uint16_t next;
> };
>
> struct virtqueue {
> @@ -245,9 +246,19 @@ struct virtio_tx_region {
> __attribute__((__aligned__(16)));
> };
>
> +static inline void
> +vring_desc_init_packed(struct vring *vr, int n)
> +{
> + int i;
> + for (i = 0; i < n; i++) {
> + struct vring_desc_packed *desc = &vr->desc_packed[i];
> + desc->index = i;
> + }
> +}
I see the split variant is also called to init the indirect tables.
Do you confirm this isn't needed in the case of packed ring?
> +
> /* Chain all the descriptors in the ring with an END */
> static inline void
> -vring_desc_init(struct vring_desc *dp, uint16_t n)
> +vring_desc_init_split(struct vring_desc *dp, uint16_t n)
> {
> uint16_t i;
>
>
On Thu, Oct 04, 2018 at 01:54:00PM +0200, Maxime Coquelin wrote:
>On 10/03/2018 03:11 PM, Jens Freimann wrote:
>>Add and initialize descriptor data structures.
>>
>>To allow out of order processing a .next field was added to
>>struct vq_desc_extra because there is none in the packed virtqueue
>>descriptor itself. This is used to chain descriptors and process them
>>similiar to how it is handled for split virtqueues.
>>
>>Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>---
>> drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
>> drivers/net/virtio/virtio_pci.h | 8 +++++
>> drivers/net/virtio/virtio_ring.h | 55 +++++++++++++++++++++++++++---
>> drivers/net/virtio/virtqueue.h | 13 ++++++-
>> 4 files changed, 88 insertions(+), 16 deletions(-)
>>
>>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>index b81df0a99..d6a1613dd 100644
>>--- a/drivers/net/virtio/virtio_ethdev.c
>>+++ b/drivers/net/virtio/virtio_ethdev.c
>>@@ -299,19 +299,27 @@ virtio_init_vring(struct virtqueue *vq)
>> PMD_INIT_FUNC_TRACE();
>>- /*
>>- * Reinitialise since virtio port might have been stopped and restarted
>>- */
>> memset(ring_mem, 0, vq->vq_ring_size);
>>- vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
>>+ vring_init(vq->hw, vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
>>+
>>+ vq->vq_free_cnt = vq->vq_nentries;
>>+ memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
>> vq->vq_used_cons_idx = 0;
>>+ vq->vq_avail_idx = 0;
>> vq->vq_desc_head_idx = 0;
>>- vq->vq_avail_idx = 0;
>> vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
>>- vq->vq_free_cnt = vq->vq_nentries;
>>- memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
>>+ if (vtpci_packed_queue(vq->hw)) {
>>+ uint16_t i;
>>+ for(i = 0; i < size - 1; i++) {
>>+ vq->vq_descx[i].next = i + 1;
>
>I would move it in a dedicated loop, and do it only if IN_ORDER hasn't
>been negotiated. Not for performance reason of course, but just to
>highlight that this extra stuff isn't needed with in-order.
makes sense, I'll change it.
>
>>+ vq->vq_ring.desc_packed[i].index = i;
>
>I would use the vring_desc_init_packed function declared below instead.
ok
>
>>+ }
>
>Trailing space.
[...]
>>diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
>>index 58fdd3d45..90204d281 100644
>>--- a/drivers/net/virtio/virtio_pci.h
>>+++ b/drivers/net/virtio/virtio_pci.h
>>@@ -113,6 +113,8 @@ struct virtnet_ctl;
>> #define VIRTIO_F_VERSION_1 32
>> #define VIRTIO_F_IOMMU_PLATFORM 33
>>+#define VIRTIO_F_RING_PACKED 34
>>+#define VIRTIO_F_IN_ORDER 35
>Isn't that feature already declared?
Yes, it is. I'll remove it here.
[...].
>> static inline void
>>-vring_init(struct vring *vr, unsigned int num, uint8_t *p,
>>+vring_init(struct virtio_hw *hw, struct vring *vr, unsigned int num, uint8_t *p,
>> unsigned long align)
>> {
>> vr->num = num;
>>+ if (vtpci_packed_queue(hw)) {
>>+ vr->desc_packed = (struct vring_desc_packed *)p;
>>+ vr->driver_event = (struct vring_packed_desc_event *)(p +
>>+ num * sizeof(struct vring_desc_packed));
>>+ vr->device_event = (struct vring_packed_desc_event *)
>>+ RTE_ALIGN_CEIL((uintptr_t)(vr->driver_event +
>>+ sizeof(struct vring_packed_desc_event)), align);
>>+ return;
>>+ }
>>+
>
>As a general comment, I would find it cleaner to have dedicated
>functions for split and packed variants, like vring_init_split,
>vring_init_packed, etc...
ok, no problem.
>> vr->desc = (struct vring_desc *) p;
>> vr->avail = (struct vring_avail *) (p +
>> num * sizeof(struct vring_desc));
>>diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>>index 26518ed98..6a4f92b79 100644
>>--- a/drivers/net/virtio/virtqueue.h
>>+++ b/drivers/net/virtio/virtqueue.h
>>@@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
>> struct vq_desc_extra {
>> void *cookie;
>> uint16_t ndescs;
>>+ uint16_t next;
>> };
>> struct virtqueue {
>>@@ -245,9 +246,19 @@ struct virtio_tx_region {
>> __attribute__((__aligned__(16)));
>> };
>>+static inline void
>>+vring_desc_init_packed(struct vring *vr, int n)
>>+{
>>+ int i;
>>+ for (i = 0; i < n; i++) {
>>+ struct vring_desc_packed *desc = &vr->desc_packed[i];
>>+ desc->index = i;
>>+ }
>>+}
>
>I see the split variant is also called to init the indirect tables.
>Do you confirm this isn't needed in the case of packed ring?
Yes, the split variant just chains descriptors in the indirect table. For
packed virtqueues we don't need to do this according to the spec.
Thanks for the review!
regards,
Jens
@@ -299,19 +299,27 @@ virtio_init_vring(struct virtqueue *vq)
PMD_INIT_FUNC_TRACE();
- /*
- * Reinitialise since virtio port might have been stopped and restarted
- */
memset(ring_mem, 0, vq->vq_ring_size);
- vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
+ vring_init(vq->hw, vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
+
+ vq->vq_free_cnt = vq->vq_nentries;
+ memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
vq->vq_used_cons_idx = 0;
+ vq->vq_avail_idx = 0;
vq->vq_desc_head_idx = 0;
- vq->vq_avail_idx = 0;
vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
- vq->vq_free_cnt = vq->vq_nentries;
- memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
+ if (vtpci_packed_queue(vq->hw)) {
+ uint16_t i;
+ for(i = 0; i < size - 1; i++) {
+ vq->vq_descx[i].next = i + 1;
+ vq->vq_ring.desc_packed[i].index = i;
+ }
+ vq->vq_ring.desc_packed[i].index = i;
+ vq->vq_descx[i].next = VQ_RING_DESC_CHAIN_END;
+ } else {
- vring_desc_init(vr->desc, size);
+ vring_desc_init_split(vr->desc, size);
+ }
/*
* Disable device(host) interrupting guest
@@ -386,7 +394,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
/*
* Reserve a memzone for vring elements
*/
- size = vring_size(vq_size, VIRTIO_PCI_VRING_ALIGN);
+ size = vring_size(hw, vq_size, VIRTIO_PCI_VRING_ALIGN);
vq->vq_ring_size = RTE_ALIGN_CEIL(size, VIRTIO_PCI_VRING_ALIGN);
PMD_INIT_LOG(DEBUG, "vring_size: %d, rounded_vring_size: %d",
size, vq->vq_ring_size);
@@ -489,7 +497,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
for (i = 0; i < vq_size; i++) {
struct vring_desc *start_dp = txr[i].tx_indir;
- vring_desc_init(start_dp, RTE_DIM(txr[i].tx_indir));
+ vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir));
/* first indirect descriptor is always the tx header */
start_dp->addr = txvq->virtio_net_hdr_mem
@@ -113,6 +113,8 @@ struct virtnet_ctl;
#define VIRTIO_F_VERSION_1 32
#define VIRTIO_F_IOMMU_PLATFORM 33
+#define VIRTIO_F_RING_PACKED 34
+#define VIRTIO_F_IN_ORDER 35
/*
* Some VirtIO feature bits (currently bits 28 through 31) are
@@ -314,6 +316,12 @@ vtpci_with_feature(struct virtio_hw *hw, uint64_t bit)
return (hw->guest_features & (1ULL << bit)) != 0;
}
+static inline int
+vtpci_packed_queue(struct virtio_hw *hw)
+{
+ return vtpci_with_feature(hw, VIRTIO_F_RING_PACKED);
+}
+
/*
* Function declaration from virtio_pci.c
*/
@@ -54,11 +54,38 @@ struct vring_used {
struct vring_used_elem ring[0];
};
+/* For support of packed virtqueues in Virtio 1.1 the format of descriptors
+ * looks like this.
+ */
+struct vring_desc_packed {
+ uint64_t addr;
+ uint32_t len;
+ uint16_t index;
+ uint16_t flags;
+} __attribute__ ((aligned(16)));
+
+#define RING_EVENT_FLAGS_ENABLE 0x0
+#define RING_EVENT_FLAGS_DISABLE 0x1
+#define RING_EVENT_FLAGS_DESC 0x2
+struct vring_packed_desc_event {
+ uint16_t desc_event_off_wrap;
+ uint16_t desc_event_flags;
+};
+
struct vring {
unsigned int num;
- struct vring_desc *desc;
- struct vring_avail *avail;
- struct vring_used *used;
+ union {
+ struct vring_desc_packed *desc_packed;
+ struct vring_desc *desc;
+ };
+ union {
+ struct vring_avail *avail;
+ struct vring_packed_desc_event *driver_event;
+ };
+ union {
+ struct vring_used *used;
+ struct vring_packed_desc_event *device_event;
+ };
};
/* The standard layout for the ring is a continuous chunk of memory which
@@ -95,10 +122,18 @@ struct vring {
#define vring_avail_event(vr) (*(uint16_t *)&(vr)->used->ring[(vr)->num])
static inline size_t
-vring_size(unsigned int num, unsigned long align)
+vring_size(struct virtio_hw *hw, unsigned int num, unsigned long align)
{
size_t size;
+ if (vtpci_packed_queue(hw)) {
+ size = num * sizeof(struct vring_desc_packed);
+ size += sizeof(struct vring_packed_desc_event);
+ size = RTE_ALIGN_CEIL(size, align);
+ size += sizeof(struct vring_packed_desc_event);
+ return size;
+ }
+
size = num * sizeof(struct vring_desc);
size += sizeof(struct vring_avail) + (num * sizeof(uint16_t));
size = RTE_ALIGN_CEIL(size, align);
@@ -108,10 +143,20 @@ vring_size(unsigned int num, unsigned long align)
}
static inline void
-vring_init(struct vring *vr, unsigned int num, uint8_t *p,
+vring_init(struct virtio_hw *hw, struct vring *vr, unsigned int num, uint8_t *p,
unsigned long align)
{
vr->num = num;
+ if (vtpci_packed_queue(hw)) {
+ vr->desc_packed = (struct vring_desc_packed *)p;
+ vr->driver_event = (struct vring_packed_desc_event *)(p +
+ num * sizeof(struct vring_desc_packed));
+ vr->device_event = (struct vring_packed_desc_event *)
+ RTE_ALIGN_CEIL((uintptr_t)(vr->driver_event +
+ sizeof(struct vring_packed_desc_event)), align);
+ return;
+ }
+
vr->desc = (struct vring_desc *) p;
vr->avail = (struct vring_avail *) (p +
num * sizeof(struct vring_desc));
@@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
struct vq_desc_extra {
void *cookie;
uint16_t ndescs;
+ uint16_t next;
};
struct virtqueue {
@@ -245,9 +246,19 @@ struct virtio_tx_region {
__attribute__((__aligned__(16)));
};
+static inline void
+vring_desc_init_packed(struct vring *vr, int n)
+{
+ int i;
+ for (i = 0; i < n; i++) {
+ struct vring_desc_packed *desc = &vr->desc_packed[i];
+ desc->index = i;
+ }
+}
+
/* Chain all the descriptors in the ring with an END */
static inline void
-vring_desc_init(struct vring_desc *dp, uint16_t n)
+vring_desc_init_split(struct vring_desc *dp, uint16_t n)
{
uint16_t i;