Re: [RFC PATCH v9 08/23] vhost: Decouple vhost_svq_add_split from VirtQueueElement

[Jason Wang]

在 2022/7/11 16:27, Eugenio Perez Martin 写道:
> On Mon, Jul 11, 2022 at 10:00 AM Jason Wang <jasowang@redhat.com> wrote:
> > 在 2022/7/7 02:39, Eugenio Pérez 写道:
> > > VirtQueueElement comes from the guest, but we're heading SVQ to be able
> > > to inject element without the guest's knowledge.
> > >
> > > To do so, make this accept sg buffers directly, instead of using
> > > VirtQueueElement.
> > >
> > > Add vhost_svq_add_element to maintain element convenience
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >    hw/virtio/vhost-shadow-virtqueue.c | 38 +++++++++++++++++++++---------
> > >    1 file changed, 27 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > > b/hw/virtio/vhost-shadow-virtqueue.c
> > > index 2fc5789b73..46d3c1d74f 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -172,30 +172,32 @@ static bool 
> > > vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
> > >    }
> > >
> > >    static bool vhost_svq_add_split(VhostShadowVirtqueue *svq,
> > > -                                VirtQueueElement *elem, unsigned *head)
> > > +                                const struct iovec *out_sg, size_t out_num,
> > > +                                const struct iovec *in_sg, size_t in_num,
> > > +                                unsigned *head)
> > >    {
> > >        unsigned avail_idx;
> > >        vring_avail_t *avail = svq->vring.avail;
> > >        bool ok;
> > > -    g_autofree hwaddr *sgs = g_new(hwaddr, MAX(elem->out_num, elem->in_num));
> > > +    g_autofree hwaddr *sgs = NULL;
> >
> > Is this change a must for this patch? (looks not related to the
> > decoupling anyhow)
> Right, the delay on the variable assignment is an artifact I missed in
> the cleaning. I can revert for the next version if any.
>
> With that reverted, can I add the acked-by tag from you?


Yes.

Thanks


> Thanks!
>
> > Other looks good.
> >
> > Thanks
> >
> >
> > >        *head = svq->free_head;
> > >
> > >        /* We need some descriptors here */
> > > -    if (unlikely(!elem->out_num && !elem->in_num)) {
> > > +    if (unlikely(!out_num && !in_num)) {
> > >            qemu_log_mask(LOG_GUEST_ERROR,
> > >                          "Guest provided element with no descriptors");
> > >            return false;
> > >        }
> > >
> > > -    ok = vhost_svq_vring_write_descs(svq, sgs, elem->out_sg, elem->out_num,
> > > -                                     elem->in_num > 0, false);
> > > +    sgs = g_new(hwaddr, MAX(out_num, in_num));
> > > +    ok = vhost_svq_vring_write_descs(svq, sgs, out_sg, out_num, in_num > 0,
> > > +                                     false);
> > >        if (unlikely(!ok)) {
> > >            return false;
> > >        }
> > >
> > > -    ok = vhost_svq_vring_write_descs(svq, sgs, elem->in_sg, elem->in_num, 
> > > false,
> > > -                                     true);
> > > +    ok = vhost_svq_vring_write_descs(svq, sgs, in_sg, in_num, false, true);
> > >        if (unlikely(!ok)) {
> > >            /* TODO unwind out_sg */
> > >            return false;
> > > @@ -223,10 +225,13 @@ static bool vhost_svq_add_split(VhostShadowVirtqueue *svq,
> > >     * takes ownership of the element: In case of failure, it is free and the SVQ
> > >     * is considered broken.
> > >     */
> > > -static bool vhost_svq_add(VhostShadowVirtqueue *svq, VirtQueueElement *elem)
> > > +static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec 
> > > *out_sg,
> > > +                          size_t out_num, const struct iovec *in_sg,
> > > +                          size_t in_num, VirtQueueElement *elem)
> > >    {
> > >        unsigned qemu_head;
> > > -    bool ok = vhost_svq_add_split(svq, elem, &qemu_head);
> > > +    bool ok = vhost_svq_add_split(svq, out_sg, out_num, in_sg, in_num,
> > > +                                  &qemu_head);
> > >        if (unlikely(!ok)) {
> > >            g_free(elem);
> > >            return false;
> > > @@ -250,6 +255,18 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
> > >        event_notifier_set(&svq->hdev_kick);
> > >    }
> > >
> > > +static bool vhost_svq_add_element(VhostShadowVirtqueue *svq,
> > > +                                  VirtQueueElement *elem)
> > > +{
> > > +    bool ok = vhost_svq_add(svq, elem->out_sg, elem->out_num, elem->in_sg,
> > > +                            elem->in_num, elem);
> > > +    if (ok) {
> > > +        vhost_svq_kick(svq);
> > > +    }
> > > +
> > > +    return ok;
> > > +}
> > > +
> > >    /**
> > >     * Forward available buffers.
> > >     *
> > > @@ -302,12 +319,11 @@ static void vhost_handle_guest_kick(VhostShadowVirtqueue 
> > > *svq)
> > >                    return;
> > >                }
> > >
> > > -            ok = vhost_svq_add(svq, elem);
> > > +            ok = vhost_svq_add_element(svq, g_steal_pointer(&elem));
> > >                if (unlikely(!ok)) {
> > >                    /* VQ is broken, just return and ignore any other kicks */
> > >                    return;
> > >                }
> > > -            vhost_svq_kick(svq);
> > >            }
> > >
> > >            virtio_queue_set_notification(svq->vq, true);