Re: [RFC PATCH 06/27] virtio: Add virtio_queue_get_used_notify_split

[Eugenio Perez Martin <eperezma@redhat.com>]

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!