Re: [RFC PATCH v8 01/21] virtio-net: Expose ctrl virtqueue logic

[Eugenio Perez Martin]

On Tue, Jun 7, 2022 at 8:13 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/5/20 03:12, Eugenio Pérez 写道:
> > This allows external vhost-net devices to modify the state of the
> > VirtIO device model once vhost-vdpa device has acknowledge the control
> > commands.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   include/hw/virtio/virtio-net.h |  4 ++
> >   hw/net/virtio-net.c            | 84 ++++++++++++++++++++--------------
> >   2 files changed, 53 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index eb87032627..cd31b7f67d 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -218,6 +218,10 @@ struct VirtIONet {
> >       struct EBPFRSSContext ebpf_rss;
> >   };
> >
> > +unsigned virtio_net_handle_ctrl_iov(VirtIODevice *vdev,
> > +                                    const struct iovec *in_sg, size_t 
> > in_num,
> > +                                    const struct iovec *out_sg,
> > +                                    unsigned out_num);
> >   void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
> >                                      const char *type);
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 7ad948ee7c..0e350154ec 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1434,57 +1434,71 @@ static int virtio_net_handle_mq(VirtIONet *n, 
> > uint8_t cmd,
> >       return VIRTIO_NET_OK;
> >   }
> >
> > -static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > +unsigned virtio_net_handle_ctrl_iov(VirtIODevice *vdev,
>
>
> Should we use size_t here?
>

I think it's a better type, yes. I used "unsigned" because
virtqueue_push uses unsigned for "len", maybe it's a good idea to
replace it there too.

Thanks!

> Thanks
>
>
> > +                                    const struct iovec *in_sg, size_t 
> > in_num,
> > +                                    const struct iovec *out_sg,
> > +                                    unsigned out_num)
> >   {
> >       VirtIONet *n = VIRTIO_NET(vdev);
> >       struct virtio_net_ctrl_hdr ctrl;
> >       virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > -    VirtQueueElement *elem;
> >       size_t s;
> >       struct iovec *iov, *iov2;
> > -    unsigned int iov_cnt;
> > +
> > +    if (iov_size(in_sg, in_num) < sizeof(status) ||
> > +        iov_size(out_sg, out_num) < sizeof(ctrl)) {
> > +        virtio_error(vdev, "virtio-net ctrl missing headers");
> > +        return 0;
> > +    }
> > +
> > +    iov2 = iov = g_memdup2(out_sg, sizeof(struct iovec) * out_num);
> > +    s = iov_to_buf(iov, out_num, 0, &ctrl, sizeof(ctrl));
> > +    iov_discard_front(&iov, &out_num, sizeof(ctrl));
> > +    if (s != sizeof(ctrl)) {
> > +        status = VIRTIO_NET_ERR;
> > +    } else if (ctrl.class == VIRTIO_NET_CTRL_RX) {
> > +        status = virtio_net_handle_rx_mode(n, ctrl.cmd, iov, out_num);
> > +    } else if (ctrl.class == VIRTIO_NET_CTRL_MAC) {
> > +        status = virtio_net_handle_mac(n, ctrl.cmd, iov, out_num);
> > +    } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
> > +        status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, out_num);
> > +    } else if (ctrl.class == VIRTIO_NET_CTRL_ANNOUNCE) {
> > +        status = virtio_net_handle_announce(n, ctrl.cmd, iov, out_num);
> > +    } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
> > +        status = virtio_net_handle_mq(n, ctrl.cmd, iov, out_num);
> > +    } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
> > +        status = virtio_net_handle_offloads(n, ctrl.cmd, iov, out_num);
> > +    }
> > +
> > +    s = iov_from_buf(in_sg, in_num, 0, &status, sizeof(status));
> > +    assert(s == sizeof(status));
> > +
> > +    g_free(iov2);
> > +    return sizeof(status);
> > +}
> > +
> > +static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > +    VirtQueueElement *elem;
> >
> >       for (;;) {
> > +        unsigned written;
> >           elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> >           if (!elem) {
> >               break;
> >           }
> > -        if (iov_size(elem->in_sg, elem->in_num) < sizeof(status) ||
> > -            iov_size(elem->out_sg, elem->out_num) < sizeof(ctrl)) {
> > -            virtio_error(vdev, "virtio-net ctrl missing headers");
> > +
> > +        written = virtio_net_handle_ctrl_iov(vdev, elem->in_sg, 
> > elem->in_num,
> > +                                             elem->out_sg, elem->out_num);
> > +        if (written > 0) {
> > +            virtqueue_push(vq, elem, written);
> > +            virtio_notify(vdev, vq);
> > +            g_free(elem);
> > +        } else {
> >               virtqueue_detach_element(vq, elem, 0);
> >               g_free(elem);
> >               break;
> >           }
> > -
> > -        iov_cnt = elem->out_num;
> > -        iov2 = iov = g_memdup2(elem->out_sg,
> > -                               sizeof(struct iovec) * elem->out_num);
> > -        s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
> > -        iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
> > -        if (s != sizeof(ctrl)) {
> > -            status = VIRTIO_NET_ERR;
> > -        } else if (ctrl.class == VIRTIO_NET_CTRL_RX) {
> > -            status = virtio_net_handle_rx_mode(n, ctrl.cmd, iov, iov_cnt);
> > -        } else if (ctrl.class == VIRTIO_NET_CTRL_MAC) {
> > -            status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
> > -        } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
> > -            status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, 
> > iov_cnt);
> > -        } else if (ctrl.class == VIRTIO_NET_CTRL_ANNOUNCE) {
> > -            status = virtio_net_handle_announce(n, ctrl.cmd, iov, iov_cnt);
> > -        } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
> > -            status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt);
> > -        } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
> > -            status = virtio_net_handle_offloads(n, ctrl.cmd, iov, iov_cnt);
> > -        }
> > -
> > -        s = iov_from_buf(elem->in_sg, elem->in_num, 0, &status, 
> > sizeof(status));
> > -        assert(s == sizeof(status));
> > -
> > -        virtqueue_push(vq, elem, sizeof(status));
> > -        virtio_notify(vdev, vq);
> > -        g_free(iov2);
> > -        g_free(elem);
> >       }
> >   }
> >
>