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);
+        }
+    }
+}
+
 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;
-- 
2.32.0

在 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


> +
>   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;

在 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;

On Tue, Jul 26, 2022 at 2:54 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 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"?

I think the right approach is probably:

1) when VERSION_1 is negotiated, only start the virtqueue when queue_enable is 1
2) when VERSION_1 is not negotiated, start virtqueue when DRIVER_OK
(vhost_dev_start())

?

>
> 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?

Probably not since vhost-user doesn't belong to the spec currently.
The doc in qemu should be sufficient.

Thanks

>
> 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;
>