Date: Tue, 19 Jan 2021 05:12:03 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Bryan Venteicher <bryanv@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: fbe0c4f4c7f4 - main - virtio: Add modern (v1) virtqueue support Message-ID: <850BBBD1-3068-4D1B-B828-201E48EA460A@freebsd.org> In-Reply-To: <202101190508.10J581lo085577@gitrepo.freebsd.org> References: <202101190508.10J581lo085577@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Bryan, Neither of my review comments from 2.5 weeks ago were addressed. Jess On 19 Jan 2021, at 05:08, Bryan Venteicher <bryanv@freebsd.org> wrote: >=20 > The branch main has been updated by bryanv: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3Dfbe0c4f4c7f4474def7b041c29c1e2fa= cf2af08b >=20 > commit fbe0c4f4c7f4474def7b041c29c1e2facf2af08b > Author: Bryan Venteicher <bryanv@FreeBSD.org> > AuthorDate: 2021-01-19 04:55:23 +0000 > Commit: Bryan Venteicher <bryanv@FreeBSD.org> > CommitDate: 2021-01-19 04:55:23 +0000 >=20 > virtio: Add modern (v1) virtqueue support >=20 > This only supports the legacy virtqueue format that is now called > "Split Virtqueues". Support for the new "Packed Virtqueues" = described > in v1.1 is left for a later date. >=20 > Reviewed by: grehan (mentor) > Differential Revision: https://reviews.freebsd.org/D27857 > --- > sys/dev/virtio/virtqueue.c | 141 = ++++++++++++++++++++++++--------------------- > sys/dev/virtio/virtqueue.h | 2 - > 2 files changed, 75 insertions(+), 68 deletions(-) >=20 > diff --git a/sys/dev/virtio/virtqueue.c b/sys/dev/virtio/virtqueue.c > index da6e7bf89fd5..14c2088b70a8 100644 > --- a/sys/dev/virtio/virtqueue.c > +++ b/sys/dev/virtio/virtqueue.c > @@ -57,19 +57,15 @@ __FBSDID("$FreeBSD$"); >=20 > struct virtqueue { > device_t vq_dev; > - char vq_name[VIRTQUEUE_MAX_NAME_SZ]; > uint16_t vq_queue_index; > uint16_t vq_nentries; > uint32_t vq_flags; > -#define VIRTQUEUE_FLAG_INDIRECT 0x0001 > -#define VIRTQUEUE_FLAG_EVENT_IDX 0x0002 > +#define VIRTQUEUE_FLAG_MODERN 0x0001 > +#define VIRTQUEUE_FLAG_INDIRECT 0x0002 > +#define VIRTQUEUE_FLAG_EVENT_IDX 0x0004 >=20 > - bus_size_t vq_notify_offset; > - int vq_alignment; > - int vq_ring_size; > - void *vq_ring_mem; > int vq_max_indirect_size; > - int vq_indirect_mem_size; > + bus_size_t vq_notify_offset; > virtqueue_intr_t *vq_intrhand; > void *vq_intrhand_arg; >=20 > @@ -88,6 +84,12 @@ struct virtqueue { > */ > uint16_t vq_used_cons_idx; >=20 > + void *vq_ring_mem; > + int vq_indirect_mem_size; > + int vq_alignment; > + int vq_ring_size; > + char vq_name[VIRTQUEUE_MAX_NAME_SZ]; > + > struct vq_desc_extra { > void *cookie; > struct vring_desc *indirect; > @@ -135,17 +137,13 @@ static int vq_ring_must_notify_host(struct = virtqueue *); > static void vq_ring_notify_host(struct virtqueue *); > static void vq_ring_free_chain(struct virtqueue *, uint16_t); >=20 > -uint64_t > -virtqueue_filter_features(uint64_t features) > -{ > - uint64_t mask; > - > - mask =3D (1 << VIRTIO_TRANSPORT_F_START) - 1; > - mask |=3D VIRTIO_RING_F_INDIRECT_DESC; > - mask |=3D VIRTIO_RING_F_EVENT_IDX; > - > - return (features & mask); > -} > +#define vq_modern(_vq) (((_vq)->vq_flags & = VIRTQUEUE_FLAG_MODERN) !=3D 0) > +#define vq_htog16(_vq, _val) virtio_htog16(vq_modern(_vq), = _val) > +#define vq_htog32(_vq, _val) virtio_htog32(vq_modern(_vq), = _val) > +#define vq_htog64(_vq, _val) virtio_htog64(vq_modern(_vq), = _val) > +#define vq_gtoh16(_vq, _val) virtio_gtoh16(vq_modern(_vq), = _val) > +#define vq_gtoh32(_vq, _val) virtio_gtoh32(vq_modern(_vq), = _val) > +#define vq_gtoh64(_vq, _val) virtio_gtoh64(vq_modern(_vq), = _val) >=20 > int > virtqueue_alloc(device_t dev, uint16_t queue, uint16_t size, > @@ -193,6 +191,8 @@ virtqueue_alloc(device_t dev, uint16_t queue, = uint16_t size, > vq->vq_intrhand =3D info->vqai_intr; > vq->vq_intrhand_arg =3D info->vqai_intr_arg; >=20 > + if (VIRTIO_BUS_WITH_FEATURE(dev, VIRTIO_F_VERSION_1) !=3D 0) > + vq->vq_flags |=3D VIRTQUEUE_FLAG_MODERN; > if (VIRTIO_BUS_WITH_FEATURE(dev, VIRTIO_RING_F_EVENT_IDX) !=3D = 0) > vq->vq_flags |=3D VIRTQUEUE_FLAG_EVENT_IDX; >=20 > @@ -297,8 +297,8 @@ virtqueue_init_indirect_list(struct virtqueue *vq, > bzero(indirect, vq->vq_indirect_mem_size); >=20 > for (i =3D 0; i < vq->vq_max_indirect_size - 1; i++) > - indirect[i].next =3D i + 1; > - indirect[i].next =3D VQ_RING_DESC_CHAIN_END; > + indirect[i].next =3D vq_gtoh16(vq, i + 1); > + indirect[i].next =3D vq_gtoh16(vq, VQ_RING_DESC_CHAIN_END); > } >=20 > int > @@ -396,6 +396,7 @@ virtqueue_used_paddr(struct virtqueue *vq) > uint16_t > virtqueue_index(struct virtqueue *vq) > { > + > return (vq->vq_queue_index); > } >=20 > @@ -444,7 +445,7 @@ virtqueue_nused(struct virtqueue *vq) > { > uint16_t used_idx, nused; >=20 > - used_idx =3D vq->vq_ring.used->idx; > + used_idx =3D vq_htog16(vq, vq->vq_ring.used->idx); >=20 > nused =3D (uint16_t)(used_idx - vq->vq_used_cons_idx); > VQASSERT(vq, nused <=3D vq->vq_nentries, "used more than = available"); > @@ -456,7 +457,7 @@ int > virtqueue_intr_filter(struct virtqueue *vq) > { >=20 > - if (vq->vq_used_cons_idx =3D=3D vq->vq_ring.used->idx) > + if (vq->vq_used_cons_idx =3D=3D vq_htog16(vq, = vq->vq_ring.used->idx)) > return (0); >=20 > virtqueue_disable_intr(vq); > @@ -483,7 +484,7 @@ virtqueue_postpone_intr(struct virtqueue *vq, = vq_postpone_t hint) > { > uint16_t ndesc, avail_idx; >=20 > - avail_idx =3D vq->vq_ring.avail->idx; > + avail_idx =3D vq_htog16(vq, vq->vq_ring.avail->idx); > ndesc =3D (uint16_t)(avail_idx - vq->vq_used_cons_idx); >=20 > switch (hint) { > @@ -508,10 +509,12 @@ virtqueue_disable_intr(struct virtqueue *vq) > { >=20 > if (vq->vq_flags & VIRTQUEUE_FLAG_EVENT_IDX) { > - vring_used_event(&vq->vq_ring) =3D vq->vq_used_cons_idx = - > - vq->vq_nentries - 1; > - } else > - vq->vq_ring.avail->flags |=3D = VRING_AVAIL_F_NO_INTERRUPT; > + vring_used_event(&vq->vq_ring) =3D vq_gtoh16(vq, > + vq->vq_used_cons_idx - vq->vq_nentries - 1); > + return; > + } > + > + vq->vq_ring.avail->flags |=3D vq_gtoh16(vq, = VRING_AVAIL_F_NO_INTERRUPT); > } >=20 > int > @@ -574,16 +577,16 @@ virtqueue_dequeue(struct virtqueue *vq, uint32_t = *len) > void *cookie; > uint16_t used_idx, desc_idx; >=20 > - if (vq->vq_used_cons_idx =3D=3D vq->vq_ring.used->idx) > + if (vq->vq_used_cons_idx =3D=3D vq_htog16(vq, = vq->vq_ring.used->idx)) > return (NULL); >=20 > used_idx =3D vq->vq_used_cons_idx++ & (vq->vq_nentries - 1); > uep =3D &vq->vq_ring.used->ring[used_idx]; >=20 > rmb(); > - desc_idx =3D (uint16_t) uep->id; > + desc_idx =3D (uint16_t) vq_htog32(vq, uep->id); > if (len !=3D NULL) > - *len =3D uep->len; > + *len =3D vq_htog32(vq, uep->len); >=20 > vq_ring_free_chain(vq, desc_idx); >=20 > @@ -641,13 +644,13 @@ virtqueue_dump(struct virtqueue *vq) > printf("VQ: %s - size=3D%d; free=3D%d; used=3D%d; queued=3D%d; " > "desc_head_idx=3D%d; avail.idx=3D%d; used_cons_idx=3D%d; " > "used.idx=3D%d; used_event_idx=3D%d; avail.flags=3D0x%x; = used.flags=3D0x%x\n", > - vq->vq_name, vq->vq_nentries, vq->vq_free_cnt, > - virtqueue_nused(vq), vq->vq_queued_cnt, = vq->vq_desc_head_idx, > - vq->vq_ring.avail->idx, vq->vq_used_cons_idx, > - vq->vq_ring.used->idx, > - vring_used_event(&vq->vq_ring), > - vq->vq_ring.avail->flags, > - vq->vq_ring.used->flags); > + vq->vq_name, vq->vq_nentries, vq->vq_free_cnt, = virtqueue_nused(vq), > + vq->vq_queued_cnt, vq->vq_desc_head_idx, > + vq_htog16(vq, vq->vq_ring.avail->idx), vq->vq_used_cons_idx, > + vq_htog16(vq, vq->vq_ring.used->idx), > + vq_htog16(vq, vring_used_event(&vq->vq_ring)), > + vq_htog16(vq, vq->vq_ring.avail->flags), > + vq_htog16(vq, vq->vq_ring.used->flags)); > } >=20 > static void > @@ -664,14 +667,14 @@ vq_ring_init(struct virtqueue *vq) > vring_init(vr, size, ring_mem, vq->vq_alignment); >=20 > for (i =3D 0; i < size - 1; i++) > - vr->desc[i].next =3D i + 1; > - vr->desc[i].next =3D VQ_RING_DESC_CHAIN_END; > + vr->desc[i].next =3D vq_gtoh16(vq, i + 1); > + vr->desc[i].next =3D vq_gtoh16(vq, VQ_RING_DESC_CHAIN_END); > } >=20 > static void > vq_ring_update_avail(struct virtqueue *vq, uint16_t desc_idx) > { > - uint16_t avail_idx; > + uint16_t avail_idx, avail_ring_idx; >=20 > /* > * Place the head of the descriptor chain into the next slot and = make > @@ -680,11 +683,12 @@ vq_ring_update_avail(struct virtqueue *vq, = uint16_t desc_idx) > * currently running on another CPU, we can keep it processing = the new > * descriptor. > */ > - avail_idx =3D vq->vq_ring.avail->idx & (vq->vq_nentries - 1); > - vq->vq_ring.avail->ring[avail_idx] =3D desc_idx; > + avail_idx =3D vq_htog16(vq, vq->vq_ring.avail->idx); > + avail_ring_idx =3D avail_idx & (vq->vq_nentries - 1); > + vq->vq_ring.avail->ring[avail_ring_idx] =3D vq_gtoh16(vq, = desc_idx); >=20 > wmb(); > - vq->vq_ring.avail->idx++; > + vq->vq_ring.avail->idx =3D vq_gtoh16(vq, avail_idx + 1); >=20 > /* Keep pending count until virtqueue_notify(). */ > vq->vq_queued_cnt++; > @@ -703,19 +707,19 @@ vq_ring_enqueue_segments(struct virtqueue *vq, = struct vring_desc *desc, >=20 > for (i =3D 0, idx =3D head_idx, seg =3D sg->sg_segs; > i < needed; > - i++, idx =3D dp->next, seg++) { > + i++, idx =3D vq_htog16(vq, dp->next), seg++) { > VQASSERT(vq, idx !=3D VQ_RING_DESC_CHAIN_END, > "premature end of free desc chain"); >=20 > dp =3D &desc[idx]; > - dp->addr =3D seg->ss_paddr; > - dp->len =3D seg->ss_len; > + dp->addr =3D vq_gtoh64(vq, seg->ss_paddr); > + dp->len =3D vq_gtoh32(vq, seg->ss_len); > dp->flags =3D 0; >=20 > if (i < needed - 1) > - dp->flags |=3D VRING_DESC_F_NEXT; > + dp->flags |=3D vq_gtoh16(vq, VRING_DESC_F_NEXT); > if (i >=3D readable) > - dp->flags |=3D VRING_DESC_F_WRITE; > + dp->flags |=3D vq_gtoh16(vq, = VRING_DESC_F_WRITE); > } >=20 > return (idx); > @@ -760,14 +764,14 @@ vq_ring_enqueue_indirect(struct virtqueue *vq, = void *cookie, > dxp->cookie =3D cookie; > dxp->ndescs =3D 1; >=20 > - dp->addr =3D dxp->indirect_paddr; > - dp->len =3D needed * sizeof(struct vring_desc); > - dp->flags =3D VRING_DESC_F_INDIRECT; > + dp->addr =3D vq_gtoh64(vq, dxp->indirect_paddr); > + dp->len =3D vq_gtoh32(vq, needed * sizeof(struct vring_desc)); > + dp->flags =3D vq_gtoh16(vq, VRING_DESC_F_INDIRECT); >=20 > vq_ring_enqueue_segments(vq, dxp->indirect, 0, > sg, readable, writable); >=20 > - vq->vq_desc_head_idx =3D dp->next; > + vq->vq_desc_head_idx =3D vq_htog16(vq, dp->next); > vq->vq_free_cnt--; > if (vq->vq_free_cnt =3D=3D 0) > VQ_RING_ASSERT_CHAIN_TERM(vq); > @@ -785,10 +789,13 @@ vq_ring_enable_interrupt(struct virtqueue *vq, = uint16_t ndesc) > * Enable interrupts, making sure we get the latest index of > * what's already been consumed. > */ > - if (vq->vq_flags & VIRTQUEUE_FLAG_EVENT_IDX) > - vring_used_event(&vq->vq_ring) =3D vq->vq_used_cons_idx = + ndesc; > - else > - vq->vq_ring.avail->flags &=3D = ~VRING_AVAIL_F_NO_INTERRUPT; > + if (vq->vq_flags & VIRTQUEUE_FLAG_EVENT_IDX) { > + vring_used_event(&vq->vq_ring) =3D > + vq_gtoh16(vq, vq->vq_used_cons_idx + ndesc); > + } else { > + vq->vq_ring.avail->flags &=3D > + vq_gtoh16(vq, ~VRING_AVAIL_F_NO_INTERRUPT); > + } >=20 > mb(); >=20 > @@ -806,17 +813,18 @@ vq_ring_enable_interrupt(struct virtqueue *vq, = uint16_t ndesc) > static int > vq_ring_must_notify_host(struct virtqueue *vq) > { > - uint16_t new_idx, prev_idx, event_idx; > + uint16_t new_idx, prev_idx, event_idx, flags; >=20 > if (vq->vq_flags & VIRTQUEUE_FLAG_EVENT_IDX) { > - new_idx =3D vq->vq_ring.avail->idx; > + new_idx =3D vq_htog16(vq, vq->vq_ring.avail->idx); > prev_idx =3D new_idx - vq->vq_queued_cnt; > - event_idx =3D vring_avail_event(&vq->vq_ring); > + event_idx =3D vq_htog16(vq, = vring_avail_event(&vq->vq_ring)); >=20 > return (vring_need_event(event_idx, new_idx, prev_idx) = !=3D 0); > } >=20 > - return ((vq->vq_ring.used->flags & VRING_USED_F_NO_NOTIFY) =3D=3D = 0); > + flags =3D vq->vq_ring.used->flags; > + return ((flags & vq_gtoh16(vq, VRING_USED_F_NO_NOTIFY)) =3D=3D = 0); > } >=20 > static void > @@ -843,10 +851,11 @@ vq_ring_free_chain(struct virtqueue *vq, = uint16_t desc_idx) > vq->vq_free_cnt +=3D dxp->ndescs; > dxp->ndescs--; >=20 > - if ((dp->flags & VRING_DESC_F_INDIRECT) =3D=3D 0) { > - while (dp->flags & VRING_DESC_F_NEXT) { > - VQ_RING_ASSERT_VALID_IDX(vq, dp->next); > - dp =3D &vq->vq_ring.desc[dp->next]; > + if ((dp->flags & vq_gtoh16(vq, VRING_DESC_F_INDIRECT)) =3D=3D 0) = { > + while (dp->flags & vq_gtoh16(vq, VRING_DESC_F_NEXT)) { > + uint16_t next_idx =3D vq_htog16(vq, dp->next); > + VQ_RING_ASSERT_VALID_IDX(vq, next_idx); > + dp =3D &vq->vq_ring.desc[next_idx]; > dxp->ndescs--; > } > } > @@ -859,6 +868,6 @@ vq_ring_free_chain(struct virtqueue *vq, uint16_t = desc_idx) > * newly freed chain. If the virtqueue was completely used, then > * head would be VQ_RING_DESC_CHAIN_END (ASSERTed above). > */ > - dp->next =3D vq->vq_desc_head_idx; > + dp->next =3D vq_gtoh16(vq, vq->vq_desc_head_idx); > vq->vq_desc_head_idx =3D desc_idx; > } > diff --git a/sys/dev/virtio/virtqueue.h b/sys/dev/virtio/virtqueue.h > index 6ac5899cf0c3..4bde4e32edc6 100644 > --- a/sys/dev/virtio/virtqueue.h > +++ b/sys/dev/virtio/virtqueue.h > @@ -67,8 +67,6 @@ struct vq_alloc_info { > (_i)->vqai_vq =3D (_vqp); = \ > } while (0) >=20 > -uint64_t virtqueue_filter_features(uint64_t features); > - > int virtqueue_alloc(device_t dev, uint16_t queue, uint16_t size, > bus_size_t notify_offset, int align, vm_paddr_t highaddr, > struct vq_alloc_info *info, struct virtqueue **vqp); Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?850BBBD1-3068-4D1B-B828-201E48EA460A>