| Message ID | 1528225683-11413-7-git-send-email-wexu@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,1/8] virtio: feature bit, data structure, init for 1.1 | expand |
On 2018年06月06日 03:08, wexu@redhat.com wrote: > From: Wei Xu <wexu@redhat.com> > > Signed-off-by: Wei Xu <wexu@redhat.com> > Signed-off-by: Wei Xu <wexu@redhat.com> > --- > hw/virtio/virtio.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 96 insertions(+), 13 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 0160d03..6f2da83 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -371,6 +371,21 @@ static void vring_packed_desc_read(VirtIODevice *vdev, VRingDescPacked *desc, > virtio_tswap16s(vdev, &desc->flags); > } > > +static void vring_packed_desc_write(VirtIODevice *vdev, VRingDescPacked *desc, > + MemoryRegionCache *cache, int i) > +{ > + virtio_tswap64s(vdev, &desc->addr); > + virtio_tswap32s(vdev, &desc->len); > + virtio_tswap16s(vdev, &desc->id); > + virtio_tswap16s(vdev, &desc->flags); > + address_space_write_cached(cache, > + sizeof(VRingDescPacked) * i, desc, > + sizeof(VRingDescPacked)); > + address_space_cache_invalidate(cache, > + sizeof(VRingDescPacked) * i, > + sizeof(VRingDescPacked)); Is there a guarantee that flags was wrote before all other fields? > +} > + > static inline bool is_desc_avail(struct VRingDescPacked *desc) > { > return !!(desc->flags & AVAIL_DESC_PACKED(1)) != > @@ -526,19 +541,11 @@ bool virtqueue_rewind(VirtQueue *vq, unsigned int num) > } > > /* Called within rcu_read_lock(). */ > -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > +static void virtqueue_split_fill(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len, unsigned int idx) > { > VRingUsedElem uelem; > > - trace_virtqueue_fill(vq, elem, len, idx); > - > - virtqueue_unmap_sg(vq, elem, len); > - > - if (unlikely(vq->vdev->broken)) { > - return; > - } > - > if (unlikely(!vq->vring.used)) { > return; > } > @@ -550,16 +557,64 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > vring_used_write(vq, &uelem, idx); > } > > -/* Called within rcu_read_lock(). */ > -void virtqueue_flush(VirtQueue *vq, unsigned int count) > +static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem, > + unsigned int len, unsigned int idx) > { > - uint16_t old, new; > + uint16_t w, head; > + VRingMemoryRegionCaches *caches; > + VRingDescPacked desc = { > + .addr = 0, > + .flags = 0, > + }; > + > + if (unlikely(!vq->vring.desc)) { > + return; > + } > + > + caches = vring_get_region_caches(vq); > + head = vq->used_idx + idx; > + head = head >= vq->vring.num ? (head - vq->vring.num) : head; > + vring_packed_desc_read(vq->vdev, &desc, &caches->desc, head); > + > + w = (desc.flags & AVAIL_DESC_PACKED(1)) >> 7; > + desc.flags &= ~(AVAIL_DESC_PACKED(1) | USED_DESC_PACKED(1)); > + desc.flags |= AVAIL_DESC_PACKED(w) | USED_DESC_PACKED(w); Why need read descriptor here? We should set the used counter by our own (e.g we have used_wrap_counter). > + if (!(desc.flags & VRING_DESC_F_INDIRECT)) { > + if (!(desc.flags & VRING_DESC_F_WRITE)) { > + desc.len = 0; > + } else { > + desc.len = len; > + } > + } > + vring_packed_desc_write(vq->vdev, &desc, &caches->desc, head); > + > + /* Make sure flags has been updated */ > + smp_mb(); This looks wrong, and if you think it's correct, you need a better comment to explain, e.g how it was paired with other. Thanks > +} > + > +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > + unsigned int len, unsigned int idx) > +{ > + trace_virtqueue_fill(vq, elem, len, idx); > + > + virtqueue_unmap_sg(vq, elem, len); > > if (unlikely(vq->vdev->broken)) { > - vq->inuse -= count; > return; > } > > + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > + virtqueue_packed_fill(vq, elem, len, idx); > + } else { > + virtqueue_split_fill(vq, elem, len, idx); > + } > +} > + > +/* Called within rcu_read_lock(). */ > +static void virtqueue_split_flush(VirtQueue *vq, unsigned int count) > +{ > + uint16_t old, new; > + > if (unlikely(!vq->vring.used)) { > return; > } > @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count) > vq->signalled_used_valid = false; > } > > +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) > +{ > + if (unlikely(!vq->vring.desc)) { > + return; > + } > + > + vq->inuse -= count; > + vq->used_idx += count; > + if (vq->used_idx >= vq->vring.num) { > + vq->used_idx -= vq->vring.num; > + vq->used_wrap_counter = !vq->used_wrap_counter; > + } > +} > + > +void virtqueue_flush(VirtQueue *vq, unsigned int count) > +{ > + if (unlikely(vq->vdev->broken)) { > + vq->inuse -= count; > + return; > + } > + > + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > + virtqueue_packed_flush(vq, count); > + } else { > + virtqueue_split_flush(vq, count); > + } > +} > + > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len) > {
On 06/05/2018 09:08 PM, wexu@redhat.com wrote: > From: Wei Xu <wexu@redhat.com> > > Signed-off-by: Wei Xu <wexu@redhat.com> > Signed-off-by: Wei Xu <wexu@redhat.com> > --- > hw/virtio/virtio.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 96 insertions(+), 13 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 0160d03..6f2da83 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -371,6 +371,21 @@ static void vring_packed_desc_read(VirtIODevice *vdev, VRingDescPacked *desc, > virtio_tswap16s(vdev, &desc->flags); > } > > +static void vring_packed_desc_write(VirtIODevice *vdev, VRingDescPacked *desc, > + MemoryRegionCache *cache, int i) > +{ > + virtio_tswap64s(vdev, &desc->addr); > + virtio_tswap32s(vdev, &desc->len); > + virtio_tswap16s(vdev, &desc->id); > + virtio_tswap16s(vdev, &desc->flags); > + address_space_write_cached(cache, > + sizeof(VRingDescPacked) * i, desc, > + sizeof(VRingDescPacked)); > + address_space_cache_invalidate(cache, > + sizeof(VRingDescPacked) * i, > + sizeof(VRingDescPacked)); > +} > + > static inline bool is_desc_avail(struct VRingDescPacked *desc) > { > return !!(desc->flags & AVAIL_DESC_PACKED(1)) != > @@ -526,19 +541,11 @@ bool virtqueue_rewind(VirtQueue *vq, unsigned int num) > } > > /* Called within rcu_read_lock(). */ > -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > +static void virtqueue_split_fill(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len, unsigned int idx) > { > VRingUsedElem uelem; > > - trace_virtqueue_fill(vq, elem, len, idx); > - > - virtqueue_unmap_sg(vq, elem, len); > - > - if (unlikely(vq->vdev->broken)) { > - return; > - } > - > if (unlikely(!vq->vring.used)) { > return; > } > @@ -550,16 +557,64 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > vring_used_write(vq, &uelem, idx); > } > > -/* Called within rcu_read_lock(). */ > -void virtqueue_flush(VirtQueue *vq, unsigned int count) > +static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem, > + unsigned int len, unsigned int idx) > { > - uint16_t old, new; > + uint16_t w, head; > + VRingMemoryRegionCaches *caches; > + VRingDescPacked desc = { > + .addr = 0, > + .flags = 0, > + }; > + > + if (unlikely(!vq->vring.desc)) { > + return; > + } > + > + caches = vring_get_region_caches(vq); > + head = vq->used_idx + idx; > + head = head >= vq->vring.num ? (head - vq->vring.num) : head; > + vring_packed_desc_read(vq->vdev, &desc, &caches->desc, head); > + > + w = (desc.flags & AVAIL_DESC_PACKED(1)) >> 7; > + desc.flags &= ~(AVAIL_DESC_PACKED(1) | USED_DESC_PACKED(1)); > + desc.flags |= AVAIL_DESC_PACKED(w) | USED_DESC_PACKED(w); > + if (!(desc.flags & VRING_DESC_F_INDIRECT)) { > + if (!(desc.flags & VRING_DESC_F_WRITE)) { > + desc.len = 0; > + } else { > + desc.len = len; > + } > + } > + vring_packed_desc_write(vq->vdev, &desc, &caches->desc, head); > + > + /* Make sure flags has been updated */ > + smp_mb(); > +} > + > +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > + unsigned int len, unsigned int idx) > +{ > + trace_virtqueue_fill(vq, elem, len, idx); > + > + virtqueue_unmap_sg(vq, elem, len); > > if (unlikely(vq->vdev->broken)) { > - vq->inuse -= count; > return; > } > > + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > + virtqueue_packed_fill(vq, elem, len, idx); > + } else { > + virtqueue_split_fill(vq, elem, len, idx); > + } > +} > + > +/* Called within rcu_read_lock(). */ > +static void virtqueue_split_flush(VirtQueue *vq, unsigned int count) > +{ > + uint16_t old, new; > + > if (unlikely(!vq->vring.used)) { > return; > } > @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count) > vq->signalled_used_valid = false; > } > > +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) > +{ > + if (unlikely(!vq->vring.desc)) { > + return; > + } > + > + vq->inuse -= count; > + vq->used_idx += count; I think it is wrong, because count seems to be representing the number of descriptor chains. But in case of packed ring layout, used index must be incremented by the number of descriptors. For example, I'm having trouble with the ctrl queue, where the commands sent by the guest use 3 descriptors, but count is 1. > + if (vq->used_idx >= vq->vring.num) { > + vq->used_idx -= vq->vring.num; > + vq->used_wrap_counter = !vq->used_wrap_counter; > + } > +} > + > +void virtqueue_flush(VirtQueue *vq, unsigned int count) > +{ > + if (unlikely(vq->vdev->broken)) { > + vq->inuse -= count; > + return; > + } > + > + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > + virtqueue_packed_flush(vq, count); > + } else { > + virtqueue_split_flush(vq, count); > + } > +} > + > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len) > { >
Hi Wei, On 9/20/18 4:13 PM, Maxime Coquelin wrote: >> } >> @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int >> count) >> vq->signalled_used_valid = false; >> } >> +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) >> +{ >> + if (unlikely(!vq->vring.desc)) { >> + return; >> + } >> + >> + vq->inuse -= count; >> + vq->used_idx += count; > > I think it is wrong, because count seems to be representing the number > of descriptor chains. > > But in case of packed ring layout, used index must be incremented by the > number of descriptors. > > For example, I'm having trouble with the ctrl queue, where the commands > sent by the guest use 3 descriptors, but count is 1. It seems you didn't fixed this issue in your last revision. I guess it works for virtio-net Kernel driver (with the fix you proposed) in guest because it is using indirect descs. But Virtio PMD does not use indirect descs for the control vq. Thanks, Maxime
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 0160d03..6f2da83 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -371,6 +371,21 @@ static void vring_packed_desc_read(VirtIODevice *vdev, VRingDescPacked *desc, virtio_tswap16s(vdev, &desc->flags); } +static void vring_packed_desc_write(VirtIODevice *vdev, VRingDescPacked *desc, + MemoryRegionCache *cache, int i) +{ + virtio_tswap64s(vdev, &desc->addr); + virtio_tswap32s(vdev, &desc->len); + virtio_tswap16s(vdev, &desc->id); + virtio_tswap16s(vdev, &desc->flags); + address_space_write_cached(cache, + sizeof(VRingDescPacked) * i, desc, + sizeof(VRingDescPacked)); + address_space_cache_invalidate(cache, + sizeof(VRingDescPacked) * i, + sizeof(VRingDescPacked)); +} + static inline bool is_desc_avail(struct VRingDescPacked *desc) { return !!(desc->flags & AVAIL_DESC_PACKED(1)) != @@ -526,19 +541,11 @@ bool virtqueue_rewind(VirtQueue *vq, unsigned int num) } /* Called within rcu_read_lock(). */ -void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, +static void virtqueue_split_fill(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len, unsigned int idx) { VRingUsedElem uelem; - trace_virtqueue_fill(vq, elem, len, idx); - - virtqueue_unmap_sg(vq, elem, len); - - if (unlikely(vq->vdev->broken)) { - return; - } - if (unlikely(!vq->vring.used)) { return; } @@ -550,16 +557,64 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, vring_used_write(vq, &uelem, idx); } -/* Called within rcu_read_lock(). */ -void virtqueue_flush(VirtQueue *vq, unsigned int count) +static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem, + unsigned int len, unsigned int idx) { - uint16_t old, new; + uint16_t w, head; + VRingMemoryRegionCaches *caches; + VRingDescPacked desc = { + .addr = 0, + .flags = 0, + }; + + if (unlikely(!vq->vring.desc)) { + return; + } + + caches = vring_get_region_caches(vq); + head = vq->used_idx + idx; + head = head >= vq->vring.num ? (head - vq->vring.num) : head; + vring_packed_desc_read(vq->vdev, &desc, &caches->desc, head); + + w = (desc.flags & AVAIL_DESC_PACKED(1)) >> 7; + desc.flags &= ~(AVAIL_DESC_PACKED(1) | USED_DESC_PACKED(1)); + desc.flags |= AVAIL_DESC_PACKED(w) | USED_DESC_PACKED(w); + if (!(desc.flags & VRING_DESC_F_INDIRECT)) { + if (!(desc.flags & VRING_DESC_F_WRITE)) { + desc.len = 0; + } else { + desc.len = len; + } + } + vring_packed_desc_write(vq->vdev, &desc, &caches->desc, head); + + /* Make sure flags has been updated */ + smp_mb(); +} + +void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, + unsigned int len, unsigned int idx) +{ + trace_virtqueue_fill(vq, elem, len, idx); + + virtqueue_unmap_sg(vq, elem, len); if (unlikely(vq->vdev->broken)) { - vq->inuse -= count; return; } + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { + virtqueue_packed_fill(vq, elem, len, idx); + } else { + virtqueue_split_fill(vq, elem, len, idx); + } +} + +/* Called within rcu_read_lock(). */ +static void virtqueue_split_flush(VirtQueue *vq, unsigned int count) +{ + uint16_t old, new; + if (unlikely(!vq->vring.used)) { return; } @@ -575,6 +630,34 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count) vq->signalled_used_valid = false; } +static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) +{ + if (unlikely(!vq->vring.desc)) { + return; + } + + vq->inuse -= count; + vq->used_idx += count; + if (vq->used_idx >= vq->vring.num) { + vq->used_idx -= vq->vring.num; + vq->used_wrap_counter = !vq->used_wrap_counter; + } +} + +void virtqueue_flush(VirtQueue *vq, unsigned int count) +{ + if (unlikely(vq->vdev->broken)) { + vq->inuse -= count; + return; + } + + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { + virtqueue_packed_flush(vq, count); + } else { + virtqueue_split_flush(vq, count); + } +} + void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len) {