Re: [PATCH v5 06/10] vdpa: Make vhost_vdpa_net_cvq_map_elem accept any out sg

[Jason Wang]

On Thu, Aug 4, 2022 at 3:39 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Thu, Aug 4, 2022 at 6:17 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/8/3 01:57, Eugenio Pérez 写道:
> > > So its generic enough to accept any out sg buffer and we can inject
> > > NIC state messages.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > > v5: Accept out sg instead of dev_buffers[]
> > > ---
> > >   net/vhost-vdpa.c | 13 +++++++------
> > >   1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 33bf3d6409..2421bca347 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -302,16 +302,16 @@ dma_map_err:
> > >   }
> > >
> > >   /**
> > > - * Copy the guest element into a dedicated buffer suitable to be sent to 
> > > NIC
> > > + * Maps out sg and in buffer into dedicated buffers suitable to be sent 
> > > to NIC
> > >    */
> > > -static bool vhost_vdpa_net_cvq_map_elem(VhostVDPAState *s,
> > > -                                        VirtQueueElement *elem,
> > > -                                        size_t *out_len)
> > > +static bool vhost_vdpa_net_cvq_map_sg(VhostVDPAState *s,
> > > +                                      const struct iovec *out, size_t 
> > > out_num,
> > > +                                      size_t *out_len)
> >
> >
> > This still looks not genreal as there's no guarantee that we won't have
> > command-in-specific-data. One example is that Ali is working on the
> > virtio-net statistics fetching from the control virtqueue.
> >
> > So it looks to me we'd better have a general bounce_map here that accepts:
> >
> > 1) out_sg and out_num
> > 2) in_sg and in_num
> >
>
> We don't need to pass in_sg for that: The only useful information is
> its size.

What if we support stats in the future where it extends the ctrl command:

         u8 command;
         u8 command-specific-data[];
         u8 ack;
+        u8 command-specific-data-reply[];

in

https://lists.oasis-open.org/archives/virtio-dev/202203/msg00000.html

> Since the exposed buffer is an in one, it's enough to expose
> the s->cvq_cmd_in_buffer buffer in full. The caller already knows the
> device will write to it, so the only missing piece is to return the
> written length at vhost_vdpa_net_cvq_add.
>
> Is one page the right buffer size for the in buffer?

We can start from this.

> Is it worth
> worrying about it before implementing the stat control command in qemu
> virtio-net?

If it's not complex, it's better to do that from the beginning,
otherwise the user may be surprised and we need extra work. Anyhow, we
should support at least ack which is an in_sg.

>
> > In this level, we'd better not have any special care about the in as the
> > ack.
>
> We need to care about it. If a property has not been updated in the
> vdpa device (it returned VIRTIO_NET_ERR), we must not update the
> device model.

Yes, but what I meant is at the level of bouncing itself. If we met
VIRTIO_NET_ERR, we should propagate it to the guest as well?

Thanks

>
> We can move the processing from vhost_vdpa_net_cvq_add to
> vhost_vdpa_net_load and vhost_vdpa_net_handle_ctrl_avail, but the code
> gets duplicated then.
>
> > And we need do bouncing:
> >
> > 1) for out buffer, during map
> > 2) for in buffer during unmap
> >
>
> We can move the copy of the in_buffer to the unmap for sure.
>
> Thanks!
>
> > Thanks
> >
> >
> > >   {
> > >       size_t in_copied;
> > >       bool ok;
> > >
> > > -    ok = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, elem->out_sg, 
> > > elem->out_num,
> > > +    ok = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, out, out_num,
> > >                                   vhost_vdpa_net_cvq_cmd_len(),
> > >                                   s->cvq_cmd_out_buffer, out_len, false);
> > >       if (unlikely(!ok)) {
> > > @@ -435,7 +435,8 @@ static int 
> > > vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> > >       };
> > >       bool ok;
> > >
> > > -    ok = vhost_vdpa_net_cvq_map_elem(s, elem, &dev_buffers[0].iov_len);
> > > +    ok = vhost_vdpa_net_cvq_map_sg(s, elem->out_sg, elem->out_num,
> > > +                                   &dev_buffers[0].iov_len);
> > >       if (unlikely(!ok)) {
> > >           goto out;
> > >       }
> >
>