| Message ID | 1550118402-4057-11-git-send-email-wexu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | packed ring virtio-net backends support | expand |
On 2019/2/14 下午12:26, wexu@redhat.com wrote: > From: Wei Xu <wexu@redhat.com> > > Both userspace and vhost-net/user are supported with this patch. > > A new subsection is introduced for packed ring, only 'last_avail_idx' > and 'last_avail_wrap_counter' are saved/loaded presumably based on > all the others relevant data(inuse, used/avail index and wrap count > should be the same. This is probably only true for net device, see comment in virtio_load(): /* * Some devices migrate VirtQueueElements that have been popped * from the avail ring but not yet returned to the used ring. * Since max ring size < UINT16_MAX it's safe to use modulo * UINT16_MAX + 1 subtraction. */ vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx - vdev->vq[i].used_idx); So you need to migrate used_idx and used_wrap_counter since we don't have used idx. > > Signed-off-by: Wei Xu <wexu@redhat.com> > --- > hw/virtio/virtio.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 66 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 8cfc7b6..7c5de07 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2349,6 +2349,13 @@ static bool virtio_virtqueue_needed(void *opaque) > return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1); > } > > +static bool virtio_packed_virtqueue_needed(void *opaque) > +{ > + VirtIODevice *vdev = opaque; > + > + return virtio_host_has_feature(vdev, VIRTIO_F_RING_PACKED); > +} > + > static bool virtio_ringsize_needed(void *opaque) > { > VirtIODevice *vdev = opaque; > @@ -2390,6 +2397,17 @@ static const VMStateDescription vmstate_virtqueue = { > } > }; > > +static const VMStateDescription vmstate_packed_virtqueue = { > + .name = "packed_virtqueue_state", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT16(last_avail_idx, struct VirtQueue), > + VMSTATE_BOOL(last_avail_wrap_counter, struct VirtQueue), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_virtio_virtqueues = { > .name = "virtio/virtqueues", > .version_id = 1, > @@ -2402,6 +2420,18 @@ static const VMStateDescription vmstate_virtio_virtqueues = { > } > }; > > +static const VMStateDescription vmstate_virtio_packed_virtqueues = { > + .name = "virtio/packed_virtqueues", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = &virtio_packed_virtqueue_needed, > + .fields = (VMStateField[]) { > + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, > + VIRTIO_QUEUE_MAX, 0, vmstate_packed_virtqueue, VirtQueue), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_ringsize = { > .name = "ringsize_state", > .version_id = 1, > @@ -2522,6 +2552,7 @@ static const VMStateDescription vmstate_virtio = { > &vmstate_virtio_ringsize, > &vmstate_virtio_broken, > &vmstate_virtio_extra_state, > + &vmstate_virtio_packed_virtqueues, > NULL > } > }; > @@ -2794,6 +2825,17 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > virtio_queue_update_rings(vdev, i); > } > > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + vdev->vq[i].shadow_avail_idx = vdev->vq[i].last_avail_idx; > + vdev->vq[i].avail_wrap_counter = > + vdev->vq[i].last_avail_wrap_counter; > + > + vdev->vq[i].used_idx = vdev->vq[i].last_avail_idx; > + vdev->vq[i].used_wrap_counter = > + vdev->vq[i].last_avail_wrap_counter; > + continue; > + } > + > nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; > /* Check it isn't doing strange things with descriptor numbers. */ > if (nheads > vdev->vq[i].vring.num) { > @@ -2955,17 +2997,34 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) > > uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) > { > - return vdev->vq[n].last_avail_idx; > + uint16_t idx; > + > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + idx = vdev->vq[n].last_avail_idx; > + idx |= ((int)vdev->vq[n].avail_wrap_counter) << 15; > + } else { > + idx = (int)vdev->vq[n].last_avail_idx; > + } > + return idx; > } > > void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx) > { > - vdev->vq[n].last_avail_idx = idx; > - vdev->vq[n].shadow_avail_idx = idx; > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + vdev->vq[n].last_avail_idx = idx & 0x7fff; > + vdev->vq[n].avail_wrap_counter = !!(idx & 0x8000); Let's define some macros for those magic number. > + } else { > + vdev->vq[n].last_avail_idx = idx; > + vdev->vq[n].shadow_avail_idx = idx; > + } > } > > void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) > { > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + return; > + } Why doesn't packed ring care about this? > + > rcu_read_lock(); > if (vdev->vq[n].vring.desc) { > vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]); > @@ -2976,6 +3035,10 @@ void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) > > void virtio_queue_update_used_idx(VirtIODevice *vdev, int n) > { > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > + return; > + } And this? Thanks > + > rcu_read_lock(); > if (vdev->vq[n].vring.desc) { > vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]);
On Tue, Feb 19, 2019 at 03:30:41PM +0800, Jason Wang wrote: > > On 2019/2/14 下午12:26, wexu@redhat.com wrote: > >From: Wei Xu <wexu@redhat.com> > > > >Both userspace and vhost-net/user are supported with this patch. > > > >A new subsection is introduced for packed ring, only 'last_avail_idx' > >and 'last_avail_wrap_counter' are saved/loaded presumably based on > >all the others relevant data(inuse, used/avail index and wrap count > >should be the same. > > > This is probably only true for net device, see comment in virtio_load(): > > /* > * Some devices migrate VirtQueueElements that have been popped > * from the avail ring but not yet returned to the used ring. > * Since max ring size < UINT16_MAX it's safe to use modulo > * UINT16_MAX + 1 subtraction. > */ > vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx - > vdev->vq[i].used_idx); > > > So you need to migrate used_idx and used_wrap_counter since we don't have > used idx. This is trying to align with vhost-net/user as we discussed, since all we have done is to support virtio-net device for packed ring, maybe we can consider supporting other devices after we have got it verified. > > > > > >Signed-off-by: Wei Xu <wexu@redhat.com> > >--- > > hw/virtio/virtio.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 66 insertions(+), 3 deletions(-) > > > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >index 8cfc7b6..7c5de07 100644 > >--- a/hw/virtio/virtio.c > >+++ b/hw/virtio/virtio.c > >@@ -2349,6 +2349,13 @@ static bool virtio_virtqueue_needed(void *opaque) > > return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1); > > } > >+static bool virtio_packed_virtqueue_needed(void *opaque) > >+{ > >+ VirtIODevice *vdev = opaque; > >+ > >+ return virtio_host_has_feature(vdev, VIRTIO_F_RING_PACKED); > >+} > >+ > > static bool virtio_ringsize_needed(void *opaque) > > { > > VirtIODevice *vdev = opaque; > >@@ -2390,6 +2397,17 @@ static const VMStateDescription vmstate_virtqueue = { > > } > > }; > >+static const VMStateDescription vmstate_packed_virtqueue = { > >+ .name = "packed_virtqueue_state", > >+ .version_id = 1, > >+ .minimum_version_id = 1, > >+ .fields = (VMStateField[]) { > >+ VMSTATE_UINT16(last_avail_idx, struct VirtQueue), > >+ VMSTATE_BOOL(last_avail_wrap_counter, struct VirtQueue), > >+ VMSTATE_END_OF_LIST() > >+ } > >+}; > >+ > > static const VMStateDescription vmstate_virtio_virtqueues = { > > .name = "virtio/virtqueues", > > .version_id = 1, > >@@ -2402,6 +2420,18 @@ static const VMStateDescription vmstate_virtio_virtqueues = { > > } > > }; > >+static const VMStateDescription vmstate_virtio_packed_virtqueues = { > >+ .name = "virtio/packed_virtqueues", > >+ .version_id = 1, > >+ .minimum_version_id = 1, > >+ .needed = &virtio_packed_virtqueue_needed, > >+ .fields = (VMStateField[]) { > >+ VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, > >+ VIRTIO_QUEUE_MAX, 0, vmstate_packed_virtqueue, VirtQueue), > >+ VMSTATE_END_OF_LIST() > >+ } > >+}; > >+ > > static const VMStateDescription vmstate_ringsize = { > > .name = "ringsize_state", > > .version_id = 1, > >@@ -2522,6 +2552,7 @@ static const VMStateDescription vmstate_virtio = { > > &vmstate_virtio_ringsize, > > &vmstate_virtio_broken, > > &vmstate_virtio_extra_state, > >+ &vmstate_virtio_packed_virtqueues, > > NULL > > } > > }; > >@@ -2794,6 +2825,17 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > > virtio_queue_update_rings(vdev, i); > > } > >+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > >+ vdev->vq[i].shadow_avail_idx = vdev->vq[i].last_avail_idx; > >+ vdev->vq[i].avail_wrap_counter = > >+ vdev->vq[i].last_avail_wrap_counter; > >+ > >+ vdev->vq[i].used_idx = vdev->vq[i].last_avail_idx; > >+ vdev->vq[i].used_wrap_counter = > >+ vdev->vq[i].last_avail_wrap_counter; > >+ continue; > >+ } > >+ > > nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; > > /* Check it isn't doing strange things with descriptor numbers. */ > > if (nheads > vdev->vq[i].vring.num) { > >@@ -2955,17 +2997,34 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) > > uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) > > { > >- return vdev->vq[n].last_avail_idx; > >+ uint16_t idx; > >+ > >+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > >+ idx = vdev->vq[n].last_avail_idx; > >+ idx |= ((int)vdev->vq[n].avail_wrap_counter) << 15; > >+ } else { > >+ idx = (int)vdev->vq[n].last_avail_idx; > >+ } > >+ return idx; > > } > > void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx) > > { > >- vdev->vq[n].last_avail_idx = idx; > >- vdev->vq[n].shadow_avail_idx = idx; > >+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > >+ vdev->vq[n].last_avail_idx = idx & 0x7fff; > >+ vdev->vq[n].avail_wrap_counter = !!(idx & 0x8000); > > > Let's define some macros for those magic number. OK. > > > >+ } else { > >+ vdev->vq[n].last_avail_idx = idx; > >+ vdev->vq[n].shadow_avail_idx = idx; > >+ } > > } > > void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) > > { > >+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > >+ return; > >+ } > > > Why doesn't packed ring care about this? As elaborated above, used idx/wrap_counter are supposed to be the same with avail ones for vhost-net/user. > > > >+ > > rcu_read_lock(); > > if (vdev->vq[n].vring.desc) { > > vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]); > >@@ -2976,6 +3035,10 @@ void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) > > void virtio_queue_update_used_idx(VirtIODevice *vdev, int n) > > { > >+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { > >+ return; > >+ } > > > And this? Same as above. Wei > > Thanks > > > >+ > > rcu_read_lock(); > > if (vdev->vq[n].vring.desc) { > > vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]);
On 2019/2/19 下午7:00, Wei Xu wrote: > On Tue, Feb 19, 2019 at 03:30:41PM +0800, Jason Wang wrote: >> On 2019/2/14 下午12:26,wexu@redhat.com wrote: >>> From: Wei Xu<wexu@redhat.com> >>> >>> Both userspace and vhost-net/user are supported with this patch. >>> >>> A new subsection is introduced for packed ring, only 'last_avail_idx' >>> and 'last_avail_wrap_counter' are saved/loaded presumably based on >>> all the others relevant data(inuse, used/avail index and wrap count >>> should be the same. >> This is probably only true for net device, see comment in virtio_load(): >> >> /* >> * Some devices migrate VirtQueueElements that have been popped >> * from the avail ring but not yet returned to the used ring. >> * Since max ring size < UINT16_MAX it's safe to use modulo >> * UINT16_MAX + 1 subtraction. >> */ >> vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx - >> vdev->vq[i].used_idx); >> >> >> So you need to migrate used_idx and used_wrap_counter since we don't have >> used idx. > This is trying to align with vhost-net/user as we discussed, since all we > have done is to support virtio-net device for packed ring, Well, what you did probably work for virtio-net. But again, virtio-net is not the only device we want to support. > maybe we can > consider supporting other devices after we have got it verified. > Please don't and fix the issue, packed virtqueue is a generic layout changes that should work for all kinds of virtio devices. It should be specially optimized for net, but it doesn't mean it was only designed for net. Thanks
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 8cfc7b6..7c5de07 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2349,6 +2349,13 @@ static bool virtio_virtqueue_needed(void *opaque) return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1); } +static bool virtio_packed_virtqueue_needed(void *opaque) +{ + VirtIODevice *vdev = opaque; + + return virtio_host_has_feature(vdev, VIRTIO_F_RING_PACKED); +} + static bool virtio_ringsize_needed(void *opaque) { VirtIODevice *vdev = opaque; @@ -2390,6 +2397,17 @@ static const VMStateDescription vmstate_virtqueue = { } }; +static const VMStateDescription vmstate_packed_virtqueue = { + .name = "packed_virtqueue_state", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT16(last_avail_idx, struct VirtQueue), + VMSTATE_BOOL(last_avail_wrap_counter, struct VirtQueue), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_virtio_virtqueues = { .name = "virtio/virtqueues", .version_id = 1, @@ -2402,6 +2420,18 @@ static const VMStateDescription vmstate_virtio_virtqueues = { } }; +static const VMStateDescription vmstate_virtio_packed_virtqueues = { + .name = "virtio/packed_virtqueues", + .version_id = 1, + .minimum_version_id = 1, + .needed = &virtio_packed_virtqueue_needed, + .fields = (VMStateField[]) { + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, + VIRTIO_QUEUE_MAX, 0, vmstate_packed_virtqueue, VirtQueue), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_ringsize = { .name = "ringsize_state", .version_id = 1, @@ -2522,6 +2552,7 @@ static const VMStateDescription vmstate_virtio = { &vmstate_virtio_ringsize, &vmstate_virtio_broken, &vmstate_virtio_extra_state, + &vmstate_virtio_packed_virtqueues, NULL } }; @@ -2794,6 +2825,17 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) virtio_queue_update_rings(vdev, i); } + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { + vdev->vq[i].shadow_avail_idx = vdev->vq[i].last_avail_idx; + vdev->vq[i].avail_wrap_counter = + vdev->vq[i].last_avail_wrap_counter; + + vdev->vq[i].used_idx = vdev->vq[i].last_avail_idx; + vdev->vq[i].used_wrap_counter = + vdev->vq[i].last_avail_wrap_counter; + continue; + } + nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; /* Check it isn't doing strange things with descriptor numbers. */ if (nheads > vdev->vq[i].vring.num) { @@ -2955,17 +2997,34 @@ hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n) uint16_t virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n) { - return vdev->vq[n].last_avail_idx; + uint16_t idx; + + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { + idx = vdev->vq[n].last_avail_idx; + idx |= ((int)vdev->vq[n].avail_wrap_counter) << 15; + } else { + idx = (int)vdev->vq[n].last_avail_idx; + } + return idx; } void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx) { - vdev->vq[n].last_avail_idx = idx; - vdev->vq[n].shadow_avail_idx = idx; + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { + vdev->vq[n].last_avail_idx = idx & 0x7fff; + vdev->vq[n].avail_wrap_counter = !!(idx & 0x8000); + } else { + vdev->vq[n].last_avail_idx = idx; + vdev->vq[n].shadow_avail_idx = idx; + } } void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) { + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { + return; + } + rcu_read_lock(); if (vdev->vq[n].vring.desc) { vdev->vq[n].last_avail_idx = vring_used_idx(&vdev->vq[n]); @@ -2976,6 +3035,10 @@ void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n) void virtio_queue_update_used_idx(VirtIODevice *vdev, int n) { + if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) { + return; + } + rcu_read_lock(); if (vdev->vq[n].vring.desc) { vdev->vq[n].used_idx = vring_used_idx(&vdev->vq[n]);