Re: [RFC PATCH v7 16/25] vdpa: control virtqueue support on shadow virtqueue

[Jason Wang]

On Thu, Apr 14, 2022 at 12:32 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Introduce the control virtqueue support for vDPA shadow virtqueue. This
> is needed for advanced networking features like multiqueue.
>
> To demonstrate command handling, VIRTIO_NET_F_CTRL_MACADDR and
> VIRTIO_NET_CTRL_MQ are implemented. If vDPA device is started with SVQ
> support and virtio-net driver changes MAC or the number of queues
> virtio-net device model will be updated with the new one.
>
> Others cvq commands could be added here straightforwardly but they have
> been not tested.

If I understand the code correctly, the cvq can still see all the
guest mappings. I wonder if it's simpler to:

1) find a way to reuse the ctrl handler in virtio-net.c
2) do not expose all the guest memory to shadow cvq.

>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  net/vhost-vdpa.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index a8dde49198..38e6912255 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -11,6 +11,7 @@
>
>  #include "qemu/osdep.h"
>  #include "clients.h"
> +#include "hw/virtio/virtio-net.h"
>  #include "net/vhost_net.h"
>  #include "net/vhost-vdpa.h"
>  #include "hw/virtio/vhost-vdpa.h"
> @@ -69,6 +70,30 @@ const int vdpa_feature_bits[] = {
>      VHOST_INVALID_FEATURE_BIT
>  };
>
> +/** Supported device specific feature bits with SVQ */
> +static const uint64_t vdpa_svq_device_features =
> +    BIT_ULL(VIRTIO_NET_F_CSUM) |
> +    BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) |
> +    BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) |
> +    BIT_ULL(VIRTIO_NET_F_MTU) |
> +    BIT_ULL(VIRTIO_NET_F_MAC) |
> +    BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) |
> +    BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) |
> +    BIT_ULL(VIRTIO_NET_F_GUEST_ECN) |
> +    BIT_ULL(VIRTIO_NET_F_GUEST_UFO) |
> +    BIT_ULL(VIRTIO_NET_F_HOST_TSO4) |
> +    BIT_ULL(VIRTIO_NET_F_HOST_TSO6) |
> +    BIT_ULL(VIRTIO_NET_F_HOST_ECN) |
> +    BIT_ULL(VIRTIO_NET_F_HOST_UFO) |
> +    BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |
> +    BIT_ULL(VIRTIO_NET_F_STATUS) |
> +    BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |
> +    BIT_ULL(VIRTIO_NET_F_MQ) |
> +    BIT_ULL(VIRTIO_F_ANY_LAYOUT) |
> +    BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
> +    BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> +    BIT_ULL(VIRTIO_NET_F_STANDBY);

I wonder what's the reason for having a dedicated feature whitelist for SVQ?

> +
>  VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
>  {
>      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -196,6 +221,46 @@ static int vhost_vdpa_get_iova_range(int fd,
>      return ret < 0 ? -errno : 0;
>  }
>
> +static void vhost_vdpa_net_handle_ctrl(VirtIODevice *vdev,
> +                                       const VirtQueueElement *elem)
> +{
> +    struct virtio_net_ctrl_hdr ctrl;
> +    virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +    size_t s;
> +    struct iovec in = {
> +        .iov_base = &status,
> +        .iov_len = sizeof(status),
> +    };
> +
> +    s = iov_to_buf(elem->out_sg, elem->out_num, 0, &ctrl, 
> sizeof(ctrl.class));
> +    if (s != sizeof(ctrl.class)) {
> +        return;
> +    }
> +
> +    switch (ctrl.class) {
> +    case VIRTIO_NET_CTRL_MAC_ADDR_SET:
> +    case VIRTIO_NET_CTRL_MQ:
> +        break;
> +    default:
> +        return;
> +    };

Any reason that we only support those two commands?

> +
> +    s = iov_to_buf(elem->in_sg, elem->in_num, 0, &status, sizeof(status));
> +    if (s != sizeof(status) || status != VIRTIO_NET_OK) {
> +        return;
> +    }
> +
> +    status = VIRTIO_NET_ERR;
> +    virtio_net_handle_ctrl_iov(vdev, &in, 1, elem->out_sg, elem->out_num);
> +    if (status != VIRTIO_NET_OK) {

status is guaranteed to be VIRTIO_NET_ERROR, so we hit the error for sure?

Thanks

> +        error_report("Bad CVQ processing in model");
> +    }
> +}
> +
> +static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
> +    .used_elem_handler = vhost_vdpa_net_handle_ctrl,
> +};
> +
>  static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>                                         const char *device,
>                                         const char *name,
> @@ -225,6 +290,9 @@ static NetClientState *net_vhost_vdpa_init(NetClientState 
> *peer,
>      s->vhost_vdpa.shadow_vqs_enabled = svq;
>      s->vhost_vdpa.iova_tree = iova_tree ? vhost_iova_tree_acquire(iova_tree) 
> :
>                                NULL;
> +    if (!is_datapath) {
> +        s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops;
> +    }
>      ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa, queue_pair_index, nvqs);
>      if (ret) {
>          if (iova_tree) {
> @@ -315,9 +383,15 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
> *name,
>      }
>      if (opts->x_svq) {
>          struct vhost_vdpa_iova_range iova_range;
> -
> -        if (has_cvq) {
> -            error_setg(errp, "vdpa svq does not work with cvq");
> +        uint64_t invalid_dev_features =
> +            features & ~vdpa_svq_device_features &
> +            /* Transport are all accepted at this point */
> +            ~MAKE_64BIT_MASK(VIRTIO_TRANSPORT_F_START,
> +                             VIRTIO_TRANSPORT_F_END - 
> VIRTIO_TRANSPORT_F_START);
> +
> +        if (invalid_dev_features) {
> +            error_setg(errp, "vdpa svq does not work with features 0x%" 
> PRIx64,
> +                       invalid_dev_features);
>              goto err_svq;
>          }
>          vhost_vdpa_get_iova_range(vdpa_device_fd, &iova_range);
> --
> 2.27.0
>