On Tue, Jul 26, 2022 at 2:54 PM Kangjie Xu <kangjie...@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...@linux.alibaba.com> > >> Signed-off-by: Xuan Zhuo <xuanz...@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; >