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

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

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