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 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 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