On Tue, Mar 2, 2021 at 12:22 PM Stefan Hajnoczi <stefanha@xxxxxxxxx> 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@xxxxxxxxx> 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!
