| Message ID | 20201120185105.279030-7-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vDPA software assisted live migration | expand |
On Fri, Nov 20, 2020 at 07:50:44PM +0100, Eugenio Pérez wrote: > This function is just used for a few commits, so SW LM is developed > incrementally, and it is deleted after it is useful. > > For a few commits, only the events (irqfd, eventfd) are forwarded. s/eventfd/ioeventfd/ (irqfd is also an eventfd) > +bool virtio_queue_get_used_notify_split(VirtQueue *vq) > +{ > + VRingMemoryRegionCaches *caches; > + hwaddr pa = offsetof(VRingUsed, flags); > + uint16_t flags; > + > + RCU_READ_LOCK_GUARD(); > + > + caches = vring_get_region_caches(vq); > + assert(caches); > + flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); > + return !(VRING_USED_F_NO_NOTIFY & flags); > +} QEMU stores the notification status: void virtio_queue_set_notification(VirtQueue *vq, int enable) { vq->notification = enable; <---- here if (!vq->vring.desc) { return; } if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { virtio_queue_packed_set_notification(vq, enable); } else { virtio_queue_split_set_notification(vq, enable); I'm wondering why it's necessary to fetch from guest RAM instead of using vq->notification? It also works for both split and packed queues so the code would be simpler.
On Mon, Dec 7, 2020 at 5:58 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Fri, Nov 20, 2020 at 07:50:44PM +0100, Eugenio Pérez wrote: > > This function is just used for a few commits, so SW LM is developed > > incrementally, and it is deleted after it is useful. > > > > For a few commits, only the events (irqfd, eventfd) are forwarded. > > s/eventfd/ioeventfd/ (irqfd is also an eventfd) > Oops, will fix, thanks! > > +bool virtio_queue_get_used_notify_split(VirtQueue *vq) > > +{ > > + VRingMemoryRegionCaches *caches; > > + hwaddr pa = offsetof(VRingUsed, flags); > > + uint16_t flags; > > + > > + RCU_READ_LOCK_GUARD(); > > + > > + caches = vring_get_region_caches(vq); > > + assert(caches); > > + flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); > > + return !(VRING_USED_F_NO_NOTIFY & flags); > > +} > > QEMU stores the notification status: > > void virtio_queue_set_notification(VirtQueue *vq, int enable) > { > vq->notification = enable; <---- here > > if (!vq->vring.desc) { > return; > } > > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > virtio_queue_packed_set_notification(vq, enable); > } else { > virtio_queue_split_set_notification(vq, enable); > > I'm wondering why it's necessary to fetch from guest RAM instead of > using vq->notification? It also works for both split and packed > queues so the code would be simpler. To use vq->notification makes sense at the end of the series. However, at this stage (just routing notifications, not descriptors), vhost device is the one updating that flag, not qemu. Since we cannot just migrate used ring memory to qemu without migrating descriptors ring too, qemu needs to check guest's memory looking for vhost device updates on that flag. I can see how that deserves better documentation or even a better name. Also, this function should be in the shadow vq file, not virtio.c. Thanks!
On Tue, Jan 12, 2021 at 07:21:27PM +0100, Eugenio Perez Martin wrote: > On Mon, Dec 7, 2020 at 5:58 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > On Fri, Nov 20, 2020 at 07:50:44PM +0100, Eugenio Pérez wrote: > > > This function is just used for a few commits, so SW LM is developed > > > incrementally, and it is deleted after it is useful. > > > > > > For a few commits, only the events (irqfd, eventfd) are forwarded. > > > > s/eventfd/ioeventfd/ (irqfd is also an eventfd) > > > > Oops, will fix, thanks! > > > > +bool virtio_queue_get_used_notify_split(VirtQueue *vq) > > > +{ > > > + VRingMemoryRegionCaches *caches; > > > + hwaddr pa = offsetof(VRingUsed, flags); > > > + uint16_t flags; > > > + > > > + RCU_READ_LOCK_GUARD(); > > > + > > > + caches = vring_get_region_caches(vq); > > > + assert(caches); > > > + flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); > > > + return !(VRING_USED_F_NO_NOTIFY & flags); > > > +} > > > > QEMU stores the notification status: > > > > void virtio_queue_set_notification(VirtQueue *vq, int enable) > > { > > vq->notification = enable; <---- here > > > > if (!vq->vring.desc) { > > return; > > } > > > > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > > virtio_queue_packed_set_notification(vq, enable); > > } else { > > virtio_queue_split_set_notification(vq, enable); > > > > I'm wondering why it's necessary to fetch from guest RAM instead of > > using vq->notification? It also works for both split and packed > > queues so the code would be simpler. > > To use vq->notification makes sense at the end of the series. > > However, at this stage (just routing notifications, not descriptors), > vhost device is the one updating that flag, not qemu. Since we cannot > just migrate used ring memory to qemu without migrating descriptors > ring too, qemu needs to check guest's memory looking for vhost device > updates on that flag. > > I can see how that deserves better documentation or even a better > name. Also, this function should be in the shadow vq file, not > virtio.c. I can't think of a reason why QEMU needs to know the flag value that the vhost device has set. This flag is a hint to the guest driver indicating whether the device wants to receive notifications. Can you explain why QEMU needs to look at the value of the flag? Stefan
On Tue, Mar 2, 2021 at 12:22 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 07:21:27PM +0100, Eugenio Perez Martin wrote: > > On Mon, Dec 7, 2020 at 5:58 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > > > On Fri, Nov 20, 2020 at 07:50:44PM +0100, Eugenio Pérez wrote: > > > > This function is just used for a few commits, so SW LM is developed > > > > incrementally, and it is deleted after it is useful. > > > > > > > > For a few commits, only the events (irqfd, eventfd) are forwarded. > > > > > > s/eventfd/ioeventfd/ (irqfd is also an eventfd) > > > > > > > Oops, will fix, thanks! > > > > > > +bool virtio_queue_get_used_notify_split(VirtQueue *vq) > > > > +{ > > > > + VRingMemoryRegionCaches *caches; > > > > + hwaddr pa = offsetof(VRingUsed, flags); > > > > + uint16_t flags; > > > > + > > > > + RCU_READ_LOCK_GUARD(); > > > > + > > > > + caches = vring_get_region_caches(vq); > > > > + assert(caches); > > > > + flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); > > > > + return !(VRING_USED_F_NO_NOTIFY & flags); > > > > +} > > > > > > QEMU stores the notification status: > > > > > > void virtio_queue_set_notification(VirtQueue *vq, int enable) > > > { > > > vq->notification = enable; <---- here > > > > > > if (!vq->vring.desc) { > > > return; > > > } > > > > > > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > > > virtio_queue_packed_set_notification(vq, enable); > > > } else { > > > virtio_queue_split_set_notification(vq, enable); > > > > > > I'm wondering why it's necessary to fetch from guest RAM instead of > > > using vq->notification? It also works for both split and packed > > > queues so the code would be simpler. > > > > To use vq->notification makes sense at the end of the series. > > > > However, at this stage (just routing notifications, not descriptors), > > vhost device is the one updating that flag, not qemu. Since we cannot > > just migrate used ring memory to qemu without migrating descriptors > > ring too, qemu needs to check guest's memory looking for vhost device > > updates on that flag. > > > > I can see how that deserves better documentation or even a better > > name. Also, this function should be in the shadow vq file, not > > virtio.c. > > I can't think of a reason why QEMU needs to know the flag value that the > vhost device has set. This flag is a hint to the guest driver indicating > whether the device wants to receive notifications. > > Can you explain why QEMU needs to look at the value of the flag? > > Stefan My bad, "need" is not the right word: SVQ could just forward the notification at this point without checking the flag. Taking into account that it's not used in later series, and it's even removed in patch 14/27 of this series, I can see that it just adds noise to the entire patchset This function just allows svq to re-check the flag after the guest sends the notification. This way svq is able to drop the kick as a (premature?) optimization in case the device sets it just after the guest sends the kick. Until patch 13/27, only notifications are forwarded, not buffers. VM guest's drivers and vhost device still read and write at usual addresses, but ioeventfd and kvmfd are intercepted by qemu. This allows us to test if the notification forwarding part is doing ok. From patch 14 of this series, svq offers a new vring to the device in qemu's VAS, so the former does not need to check the guest's memory anymore, and this function can be dropped. Is it clearer now? Please let me know if I should add something else. Thanks!
On Tue, Mar 02, 2021 at 07:34:20PM +0100, Eugenio Perez Martin wrote: > On Tue, Mar 2, 2021 at 12:22 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > On Tue, Jan 12, 2021 at 07:21:27PM +0100, Eugenio Perez Martin wrote: > > > On Mon, Dec 7, 2020 at 5:58 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > > > > > On Fri, Nov 20, 2020 at 07:50:44PM +0100, Eugenio Pérez wrote: > > > > > This function is just used for a few commits, so SW LM is developed > > > > > incrementally, and it is deleted after it is useful. > > > > > > > > > > For a few commits, only the events (irqfd, eventfd) are forwarded. > > > > > > > > s/eventfd/ioeventfd/ (irqfd is also an eventfd) > > > > > > > > > > Oops, will fix, thanks! > > > > > > > > +bool virtio_queue_get_used_notify_split(VirtQueue *vq) > > > > > +{ > > > > > + VRingMemoryRegionCaches *caches; > > > > > + hwaddr pa = offsetof(VRingUsed, flags); > > > > > + uint16_t flags; > > > > > + > > > > > + RCU_READ_LOCK_GUARD(); > > > > > + > > > > > + caches = vring_get_region_caches(vq); > > > > > + assert(caches); > > > > > + flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); > > > > > + return !(VRING_USED_F_NO_NOTIFY & flags); > > > > > +} > > > > > > > > QEMU stores the notification status: > > > > > > > > void virtio_queue_set_notification(VirtQueue *vq, int enable) > > > > { > > > > vq->notification = enable; <---- here > > > > > > > > if (!vq->vring.desc) { > > > > return; > > > > } > > > > > > > > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > > > > virtio_queue_packed_set_notification(vq, enable); > > > > } else { > > > > virtio_queue_split_set_notification(vq, enable); > > > > > > > > I'm wondering why it's necessary to fetch from guest RAM instead of > > > > using vq->notification? It also works for both split and packed > > > > queues so the code would be simpler. > > > > > > To use vq->notification makes sense at the end of the series. > > > > > > However, at this stage (just routing notifications, not descriptors), > > > vhost device is the one updating that flag, not qemu. Since we cannot > > > just migrate used ring memory to qemu without migrating descriptors > > > ring too, qemu needs to check guest's memory looking for vhost device > > > updates on that flag. > > > > > > I can see how that deserves better documentation or even a better > > > name. Also, this function should be in the shadow vq file, not > > > virtio.c. > > > > I can't think of a reason why QEMU needs to know the flag value that the > > vhost device has set. This flag is a hint to the guest driver indicating > > whether the device wants to receive notifications. > > > > Can you explain why QEMU needs to look at the value of the flag? > > > > Stefan > > My bad, "need" is not the right word: SVQ could just forward the > notification at this point without checking the flag. Taking into > account that it's not used in later series, and it's even removed in > patch 14/27 of this series, I can see that it just adds noise to the > entire patchset > > This function just allows svq to re-check the flag after the guest > sends the notification. This way svq is able to drop the kick as a > (premature?) optimization in case the device sets it just after the > guest sends the kick. > > Until patch 13/27, only notifications are forwarded, not buffers. VM > guest's drivers and vhost device still read and write at usual > addresses, but ioeventfd and kvmfd are intercepted by qemu. This > allows us to test if the notification forwarding part is doing ok. > From patch 14 of this series, svq offers a new vring to the device in > qemu's VAS, so the former does not need to check the guest's memory > anymore, and this function can be dropped. > > Is it clearer now? Please let me know if I should add something else. Thanks for explaining. You could drop it to simplify the code. If you leave it in, please include a comment explaining the purpose. Thanks, Stefan
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index b7ece7a6a8..b9b8497ea0 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -225,6 +225,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id); void virtio_notify_config(VirtIODevice *vdev); +bool virtio_queue_get_used_notify_split(VirtQueue *vq); bool virtio_queue_get_notification(VirtQueue *vq); void virtio_queue_set_notification(VirtQueue *vq, int enable); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index ceb58fda6c..3469946538 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -377,6 +377,20 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val) vq->used_idx = val; } +bool virtio_queue_get_used_notify_split(VirtQueue *vq) +{ + VRingMemoryRegionCaches *caches; + hwaddr pa = offsetof(VRingUsed, flags); + uint16_t flags; + + RCU_READ_LOCK_GUARD(); + + caches = vring_get_region_caches(vq); + assert(caches); + flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); + return !(VRING_USED_F_NO_NOTIFY & flags); +} + /* Called within rcu_read_lock(). */ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask) {
This function is just used for a few commits, so SW LM is developed incrementally, and it is deleted after it is useful. For a few commits, only the events (irqfd, eventfd) are forwarded. This series adds descriptors forwarding on top of that. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- include/hw/virtio/virtio.h | 1 + hw/virtio/virtio.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)