On Thu, Apr 14, 2022 at 11:10 AM Jason Wang <jasow...@redhat.com> wrote:
>
> On Thu, Apr 14, 2022 at 12:32 AM Eugenio Pérez <epere...@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
It's reused, that's why virtio_net_handle_ctrl_iov is extracted from
virtio_net_handle_ctrl.
> 2) do not expose all the guest memory to shadow cvq.
>
It can be done that way actually, but it would include a map and unmap
for each control command call. I'll explore that approach, thanks!
> >
> > Signed-off-by: Eugenio Pérez <epere...@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?
>
We cannot be sure that future commands do not require modifications to
qemu. Same as with the switch, I can dedicate time to test all of the
currently supported cvq commands and delete this.
> > +
> > 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?
>
Lack of testing, basically. I can try to test all of them for the next
patch series.
> > +
> > + 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?
>
Status is modified through "in" iovec virtio_net_handle_ctrl_iov
parameter, but it is not immediate just seeing this piece of code in
isolation. I can try to make it clearer.
Thanks!
> 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
> >
>
