Re: [PATCH 14/16] virtio-net: support queue_enable for vhost-user

[Kangjie Xu]

在 2022/7/26 12:25, Jason Wang 写道:
> 在 2022/7/18 19:17, Kangjie Xu 写道:
> > Support queue enable in vhost-user scenario. It will be called when
> > a vq reset operation is performed and the vq will be restared.
> >
> > It should be noted that we can restart the vq when the vhost has
> > already started. When launching a new vhost-user device, the vhost
> > is not started and all vqs are not initalized until
> > VIRTIO_PCI_COMMON_STATUS is written. Thus, we should use vhost_started
> > to differentiate the two cases: vq reset and device start.
> >
> > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   hw/net/virtio-net.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 8396e21a67..2c26e2ef73 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -544,6 +544,25 @@ static void virtio_net_queue_reset(VirtIODevice 
> > *vdev, uint32_t queue_index)
> >       assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
> >   }
> >   +static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t 
> > queue_index)
> > +{
> > +    VirtIONet *n = VIRTIO_NET(vdev);
> > +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> > +    int r;
> > +
> > +    if (!nc->peer || !vdev->vhost_started) {
> > +        return;
> > +    }
> > +
> > +    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > +        r = vhost_virtqueue_restart(vdev, nc, queue_index);
> > +        if (r < 0) {
> > +            error_report("unable to restart vhost net virtqueue: %d, "
> > +                            "when resetting the queue", queue_index);
> > +        }
> > +    }
> > +}
>
>
> So we don't check queue_enable in vhost_dev_start(), does this mean 
> the queue_enable is actually meaningless (since the virtqueue is 
> already started there)?
>
> And another issue is
>
> peet_attach/peer_detach() "abuse" vhost_set_vring_enable(). This 
> probably means we need to invent new type of request instead of 
> re-using VHOST_USER_SET_VRING_ENABLE.
>
> Thanks
1. Yes, we don't need queue_enable in vhost_dev_start(), queue_enable is 
only useful when restarting the queue. I name it as queue_enable() 
simply because it is called when VIRTIO_PCI_COMMON_Q_ENABLE is written. 
Would it look better if we rename it as "queue_reenable"?

2. I think inventing a new type of vhost-protocol message can be a good 
choice. However, I don't know much about the vhost protocol. If we want 
to add a new message in vhost protocol, except the documentation and the 
code in qemu, Do we need to submit patches to other projects, e.g. some 
projects like virtio-spec?

Thanks

> > +
> >   static void virtio_net_reset(VirtIODevice *vdev)
> >   {
> >       VirtIONet *n = VIRTIO_NET(vdev);
> > @@ -3781,6 +3800,7 @@ static void virtio_net_class_init(ObjectClass 
> > *klass, void *data)
> >       vdc->bad_features = virtio_net_bad_features;
> >       vdc->reset = virtio_net_reset;
> >       vdc->queue_reset = virtio_net_queue_reset;
> > +    vdc->queue_enable = virtio_net_queue_enable;
> >       vdc->set_status = virtio_net_set_status;
> >       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
> >       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;