Re: [PATCH 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

On 5/10/24 3:48 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> > Add VIRTIO_F_IN_ORDER feature support for virtqueue_flush operations.
> >
> > The goal of the virtqueue_flush operation when the VIRTIO_F_IN_ORDER
> > feature has been negotiated is to write elements to the used/descriptor
> > ring in-order and then update used_idx.
> >
> > The function iterates through the VirtQueueElement used_elems array
> > in-order starting at vq->used_idx. If the element is valid (filled), the
> > element is written to the used/descriptor ring. This process continues
> > until we find an invalid (not filled) element.
> >
> > If any elements were written, the used_idx is updated.
> >
> > Tested-by: Lei Yang <leiyang@redhat.com>
> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> > ---
> >   hw/virtio/virtio.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 064046b5e2..0efed2c88e 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1006,6 +1006,77 @@ static void virtqueue_packed_flush(VirtQueue *vq, 
> > unsigned int count)
> >       }
> >   }
> >
> > +static void virtqueue_ordered_flush(VirtQueue *vq)
> > +{
> > +    unsigned int i = vq->used_idx;
> > +    unsigned int ndescs = 0;
> > +    uint16_t old = vq->used_idx;
> > +    bool packed;
> > +    VRingUsedElem uelem;
> > +
> > +    packed = virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED);
> > +
> > +    if (packed) {
> > +        if (unlikely(!vq->vring.desc)) {
> > +            return;
> > +        }
> > +    } else if (unlikely(!vq->vring.used)) {
> > +        return;
> > +    }
> > +
> > +    /* First expected in-order element isn't ready, nothing to do */
> > +    if (!vq->used_elems[i].filled) {
> > +        return;
> > +    }
> > +
> > +    /* Write first expected in-order element to used ring (split VQs) */
> > +    if (!packed) {
> > +        uelem.id = vq->used_elems[i].index;
> > +        uelem.len = vq->used_elems[i].len;
> > +        vring_used_write(vq, &uelem, i);
> > +    }
> > +
> > +    ndescs += vq->used_elems[i].ndescs;
> > +    i += ndescs;
> > +    if (i >= vq->vring.num) {
> > +        i -= vq->vring.num;
> > +    }
> > +
> > +    /* Search for more filled elements in-order */
> > +    while (vq->used_elems[i].filled) {
> > +        if (packed) {
> > +            virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
> > +        } else {
> > +            uelem.id = vq->used_elems[i].index;
> > +            uelem.len = vq->used_elems[i].len;
> > +            vring_used_write(vq, &uelem, i);
> > +        }
> > +
> > +        vq->used_elems[i].filled = false;
> > +        ndescs += vq->used_elems[i].ndescs;
> > +        i += ndescs;
> > +        if (i >= vq->vring.num) {
> > +            i -= vq->vring.num;
> > +        }
> > +    }
> > +
>
> I may be missing something, but you have split out the first case as a
> special one, totally out of the while loop. Can't it be contained in
> the loop checking !(packed && i == vq->used_idx)? That would avoid
> code duplication.
>
> A comment can be added in the line of "first entry of packed is
> written the last so the guest does not see invalid descriptors".

Yea this was intentional for the reason you've given above. It was 
either the solution above or, as you suggest, handling this in the while 
loop:

if (!vq->used_elems[i].filled) {
    return;
}

while (vq->used_elems[i].filled) {
    if (packed && i != vq->used_idx) {
        virtqueue_packed_fill_desc(...);
    } else {
        ...
    }
    ...
}

I did consider this option at the time of writing this patch but I 
must've overcomplicated it in my head somehow and thought the current 
solution was the simpler one. However, after looking it over again, your 
suggestion is indeed the cleaner one.

Will adjust this in v2! Thanks for your time reviewing these!

> > +    if (packed) {
> > +        virtqueue_packed_fill_desc(vq, &vq->used_elems[vq->used_idx], 0, true);
> > +        vq->used_idx += ndescs;
> > +        if (vq->used_idx >= vq->vring.num) {
> > +            vq->used_idx -= vq->vring.num;
> > +            vq->used_wrap_counter ^= 1;
> > +            vq->signalled_used_valid = false;
> > +        }
> > +    } else {
> > +        vring_used_idx_set(vq, i);
> > +        if (unlikely((int16_t)(i - vq->signalled_used) < (uint16_t)(i - old))) 
> > {
> > +            vq->signalled_used_valid = false;
> > +        }
> > +    }
> > +    vq->inuse -= ndescs;
> > +}
> > +
> >   void virtqueue_flush(VirtQueue *vq, unsigned int count)
> >   {
> >       if (virtio_device_disabled(vq->vdev)) {
> > @@ -1013,7 +1084,9 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
> >           return;
> >       }
> >
> > -    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> > +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
> > +        virtqueue_ordered_flush(vq);
> > +    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> >           virtqueue_packed_flush(vq, count);
> >       } else {
> >           virtqueue_split_flush(vq, count);
> > --
> > 2.39.3