From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AFA35C43217 for ; Thu, 1 Dec 2022 10:18:45 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1p0geH-00031J-B1; Thu, 01 Dec 2022 05:18:33 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0gdw-0002tE-FM for qemu-devel@nongnu.org; Thu, 01 Dec 2022 05:18:25 -0500 Received: from mail-wm1-x334.google.com ([2a00:1450:4864:20::334]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1p0gdg-0006Ep-Uy for qemu-devel@nongnu.org; Thu, 01 Dec 2022 05:18:12 -0500 Received: by mail-wm1-x334.google.com with SMTP id bg10so822705wmb.1 for ; Thu, 01 Dec 2022 02:17:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:mime-version:message-id:in-reply-to:date :subject:cc:to:from:user-agent:references:from:to:cc:subject:date :message-id:reply-to; bh=zQGdXolueJl4+I05fxSRjOKl8hbjw5D0DFTEuJ20ecc=; b=dSs4A0z8dw5RwwHYF99GRQfXl84+Jo0mcHfyDXmDeKciGlzrLc848+DfRoiHPI7Ens aKup7cQhzIctfcQukQqoT310/ttj2yhn8CGqtLln95LrNy5RfYd2nYiDi7fLXOKwWImr kHxJnf5/LW9n+TlURjNBUeymK2hCToIGUV5+V6tY/+QFRoRNAgRAurGurcRF6xwNx0n4 Md+3n6CGci8hjqSs1VF8H2Pq5bYxxqBJQiXOXAxPDZmtAmWxfGCoEuCc+x+4kfPF6MRc ffqnT6nxfOiRzTDBAKtHPOSdJxTGVyY8WkP952jigJ9R+feWahtKggsZpcigBBshnoyr 2L/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:in-reply-to:date :subject:cc:to:from:user-agent:references:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=zQGdXolueJl4+I05fxSRjOKl8hbjw5D0DFTEuJ20ecc=; b=ofIch2CngtmJOFnLPK2T3Ka3NJ9DRAGJpwOzmiX9nyOVaSMjLdqHjhjsPx966sBPrw WvcWLFWSkTMF+ymNqmfmvnEHfshXbCiBaAAas64WMvshLhkeLowURfUjHJG9hXnFwe8r xNHxEiTCfgSmtOMO5z4l1i+RkKyqwzY2SzYy6ErCOWyehYEAvIM78+zJ3eDbPz1qyEnG UXRXm2xg0RNbZsL7U966Zu98qL7cnOJl1kXllAde/145TPWpdV8kU8NmZHxzz4RLvFOA VLOb2f92Nan6Ue7Q0P2spsh+3ovCARj+eWaJfbbbHpimCGLKbCduX2QRrp3/s5t//3cs +ypA== X-Gm-Message-State: ANoB5pkoSiYi7zwO7QPPqCMSJ0r22kcFwt2ACXaBa4Hos4QquMKKuHMj uj3AfqV5SQD1eBeUarw8GRyAoA== X-Google-Smtp-Source: AA0mqf48eYIpZvuhMDpJkvAsukRyMB74tsusnRVLz0eGMy11apqNmoaWc1v58SWmSqZz0EBs88BgAQ== X-Received: by 2002:a05:600c:4148:b0:3cf:5657:3791 with SMTP id h8-20020a05600c414800b003cf56573791mr37063919wmm.34.1669889874153; Thu, 01 Dec 2022 02:17:54 -0800 (PST) Received: from zen.linaroharston ([185.81.254.11]) by smtp.gmail.com with ESMTPSA id bi18-20020a05600c3d9200b003c65c9a36dfsm4650620wmb.48.2022.12.01.02.17.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Dec 2022 02:17:53 -0800 (PST) Received: from zen (localhost [127.0.0.1]) by zen.linaroharston (Postfix) with ESMTP id 1720D1FFB7; Thu, 1 Dec 2022 10:17:53 +0000 (GMT) References: <20221123131630.52020-1-sgarzare@redhat.com> <20221125081243.a3q7ep32o6g4r2vd@sgarzare-redhat> <20221201080250.s3cugnnkf5f4kqvo@sgarzare-redhat> User-agent: mu4e 1.9.3; emacs 29.0.60 From: Alex =?utf-8?Q?Benn=C3=A9e?= To: Stefano Garzarella Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , Kevin Wolf , virtio-fs@redhat.com, Mathieu Poirier , Fam Zheng , Viresh Kumar , German Maglione , Jason Wang , Raphael Norwitz , Hanna Reitz , qemu-block@nongnu.org, "Gonglei (Arei)" , "Michael S. Tsirkin" , Paolo Bonzini Subject: Re: [PATCH for-7.2] vhost: enable vrings in vhost_dev_start() for vhost-user devices Date: Thu, 01 Dec 2022 10:14:39 +0000 In-reply-to: <20221201080250.s3cugnnkf5f4kqvo@sgarzare-redhat> Message-ID: <87k03bctrj.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=2a00:1450:4864:20::334; envelope-from=alex.bennee@linaro.org; helo=mail-wm1-x334.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Stefano Garzarella writes: > On Wed, Nov 30, 2022 at 04:03:28PM -0500, Stefan Hajnoczi wrote: >>On Fri, Nov 25, 2022 at 09:12:43AM +0100, Stefano Garzarella wrote: >>> On Thu, Nov 24, 2022 at 01:36:29PM -0500, Stefan Hajnoczi wrote: >>> > On Wed, Nov 23, 2022 at 02:16:30PM +0100, Stefano Garzarella wrote: >>> > > Commit 02b61f38d3 ("hw/virtio: incorporate backend features in feat= ures") >>> > > properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-u= ser >>> > > backend, but we forgot to enable vrings as specified in >>> > > docs/interop/vhost-user.rst: >>> > > >>> > > If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, = the >>> > > ring starts directly in the enabled state. >>> > > >>> > > If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the = ring is >>> > > initialized in a disabled state and is enabled by >>> > > ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1. >>> > > >>> > > Some vhost-user front-ends already did this by calling >>> > > vhost_ops.vhost_set_vring_enable() directly: >>> > > - backends/cryptodev-vhost.c >>> > > - hw/net/virtio-net.c >>> > > - hw/virtio/vhost-user-gpio.c >>> > > >>> > > But most didn't do that, so we would leave the vrings disabled and = some >>> > > backends would not work. We observed this issue with the rust versi= on of >>> > > virtiofsd [1], which uses the event loop [2] provided by the >>> > > vhost-user-backend crate where requests are not processed if vring = is >>> > > not enabled. >>> > > >>> > > Let's fix this issue by enabling the vrings in vhost_dev_start() for >>> > > vhost-user front-ends that don't already do this directly. Same thi= ng >>> > > also in vhost_dev_stop() where we disable vrings. >>> > > >>> > > [1] https://gitlab.com/virtio-fs/virtiofsd >>> > > [2] > https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backen= d/src/event_loop.rs#L217 >>> > > >>> > > Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in feat= ures") >>> > > Reported-by: German Maglione >>> > > Tested-by: German Maglione >>> > > Signed-off-by: Stefano Garzarella >>> > > --- >>> > > include/hw/virtio/vhost.h | 6 +++-- >>> > > backends/cryptodev-vhost.c | 4 ++-- >>> > > backends/vhost-user.c | 4 ++-- >>> > > hw/block/vhost-user-blk.c | 4 ++-- >>> > > hw/net/vhost_net.c | 8 +++---- >>> > > hw/scsi/vhost-scsi-common.c | 4 ++-- >>> > > hw/virtio/vhost-user-fs.c | 4 ++-- >>> > > hw/virtio/vhost-user-gpio.c | 4 ++-- >>> > > hw/virtio/vhost-user-i2c.c | 4 ++-- >>> > > hw/virtio/vhost-user-rng.c | 4 ++-- >>> > > hw/virtio/vhost-vsock-common.c | 4 ++-- >>> > > hw/virtio/vhost.c | 44 ++++++++++++++++++++++++++++++= ---- >>> > > hw/virtio/trace-events | 4 ++-- >>> > > 13 files changed, 67 insertions(+), 31 deletions(-) >>> > > >>> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h >>> > > index 353252ac3e..67a6807fac 100644 >>> > > --- a/include/hw/virtio/vhost.h >>> > > +++ b/include/hw/virtio/vhost.h >>> > > @@ -184,24 +184,26 @@ static inline bool vhost_dev_is_started(struc= t vhost_dev *hdev) >>> > > * vhost_dev_start() - start the vhost device >>> > > * @hdev: common vhost_dev structure >>> > > * @vdev: the VirtIODevice structure >>> > > + * @vrings: true to have vrings enabled in this call >>> > > * >>> > > * Starts the vhost device. From this point VirtIO feature negotia= tion >>> > > * can start and the device can start processing VirtIO transactio= ns. >>> > > * >>> > > * Return: 0 on success, < 0 on error. >>> > > */ >>> > > -int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev); >>> > > +int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bo= ol vrings); >>> > > >>> > > /** >>> > > * vhost_dev_stop() - stop the vhost device >>> > > * @hdev: common vhost_dev structure >>> > > * @vdev: the VirtIODevice structure >>> > > + * @vrings: true to have vrings disabled in this call >>> > > * >>> > > * Stop the vhost device. After the device is stopped the notifiers >>> > > * can be disabled (@vhost_dev_disable_notifiers) and the device c= an >>> > > * be torn down (@vhost_dev_cleanup). >>> > > */ >>> > > -void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev); >>> > > +void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bo= ol vrings); >>> > > >>> > > /** >>> > > * DOC: vhost device configuration handling >>> > > diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c >>> > > index bc13e466b4..572f87b3be 100644 >>> > > --- a/backends/cryptodev-vhost.c >>> > > +++ b/backends/cryptodev-vhost.c >>> > > @@ -94,7 +94,7 @@ cryptodev_vhost_start_one(CryptoDevBackendVhost *= crypto, >>> > > goto fail_notifiers; >>> > > } >>> > > >>> > > - r =3D vhost_dev_start(&crypto->dev, dev); >>> > > + r =3D vhost_dev_start(&crypto->dev, dev, false); >>> > > if (r < 0) { >>> > > goto fail_start; >>> > > } >>> > > @@ -111,7 +111,7 @@ static void >>> > > cryptodev_vhost_stop_one(CryptoDevBackendVhost *crypto, >>> > > VirtIODevice *dev) >>> > > { >>> > > - vhost_dev_stop(&crypto->dev, dev); >>> > > + vhost_dev_stop(&crypto->dev, dev, false); >>> > > vhost_dev_disable_notifiers(&crypto->dev, dev); >>> > > } >>> > > >>> > > diff --git a/backends/vhost-user.c b/backends/vhost-user.c >>> > > index 5dedb2d987..7bfcaef976 100644 >>> > > --- a/backends/vhost-user.c >>> > > +++ b/backends/vhost-user.c >>> > > @@ -85,7 +85,7 @@ vhost_user_backend_start(VhostUserBackend *b) >>> > > } >>> > > >>> > > b->dev.acked_features =3D b->vdev->guest_features; >>> > > - ret =3D vhost_dev_start(&b->dev, b->vdev); >>> > > + ret =3D vhost_dev_start(&b->dev, b->vdev, true); >>> > > if (ret < 0) { >>> > > error_report("Error start vhost dev"); >>> > > goto err_guest_notifiers; >>> > > @@ -120,7 +120,7 @@ vhost_user_backend_stop(VhostUserBackend *b) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(&b->dev, b->vdev); >>> > > + vhost_dev_stop(&b->dev, b->vdev, true); >>> > > >>> > > if (k->set_guest_notifiers) { >>> > > ret =3D k->set_guest_notifiers(qbus->parent, >>> > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c >>> > > index 0d5190accf..1177064631 100644 >>> > > --- a/hw/block/vhost-user-blk.c >>> > > +++ b/hw/block/vhost-user-blk.c >>> > > @@ -178,7 +178,7 @@ static int vhost_user_blk_start(VirtIODevice *v= dev, Error **errp) >>> > > } >>> > > >>> > > s->dev.vq_index_end =3D s->dev.nvqs; >>> > > - ret =3D vhost_dev_start(&s->dev, vdev); >>> > > + ret =3D vhost_dev_start(&s->dev, vdev, true); >>> > > if (ret < 0) { >>> > > error_setg_errno(errp, -ret, "Error starting vhost"); >>> > > goto err_guest_notifiers; >>> > > @@ -213,7 +213,7 @@ static void vhost_user_blk_stop(VirtIODevice *v= dev) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(&s->dev, vdev); >>> > > + vhost_dev_stop(&s->dev, vdev, true); >>> > > >>> > > ret =3D k->set_guest_notifiers(qbus->parent, s->dev.nvqs, fals= e); >>> > > if (ret < 0) { >>> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >>> > > index 26e4930676..043058ff43 100644 >>> > > --- a/hw/net/vhost_net.c >>> > > +++ b/hw/net/vhost_net.c >>> > > @@ -259,7 +259,7 @@ static int vhost_net_start_one(struct vhost_net= *net, >>> > > goto fail_notifiers; >>> > > } >>> > > >>> > > - r =3D vhost_dev_start(&net->dev, dev); >>> > > + r =3D vhost_dev_start(&net->dev, dev, false); >>> > > if (r < 0) { >>> > > goto fail_start; >>> > > } >>> > > @@ -308,7 +308,7 @@ fail: >>> > > if (net->nc->info->poll) { >>> > > net->nc->info->poll(net->nc, true); >>> > > } >>> > > - vhost_dev_stop(&net->dev, dev); >>> > > + vhost_dev_stop(&net->dev, dev, false); >>> > > fail_start: >>> > > vhost_dev_disable_notifiers(&net->dev, dev); >>> > > fail_notifiers: >>> > > @@ -329,7 +329,7 @@ static void vhost_net_stop_one(struct vhost_net= *net, >>> > > if (net->nc->info->poll) { >>> > > net->nc->info->poll(net->nc, true); >>> > > } >>> > > - vhost_dev_stop(&net->dev, dev); >>> > > + vhost_dev_stop(&net->dev, dev, false); >>> > > if (net->nc->info->stop) { >>> > > net->nc->info->stop(net->nc); >>> > > } >>> > > @@ -606,7 +606,7 @@ err_start: >>> > > assert(r >=3D 0); >>> > > } >>> > > >>> > > - vhost_dev_stop(&net->dev, vdev); >>> > > + vhost_dev_stop(&net->dev, vdev, false); >>> > > >>> > > return r; >>> > > } >>> > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-commo= n.c >>> > > index 767f827e55..18ea5dcfa1 100644 >>> > > --- a/hw/scsi/vhost-scsi-common.c >>> > > +++ b/hw/scsi/vhost-scsi-common.c >>> > > @@ -68,7 +68,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc) >>> > > goto err_guest_notifiers; >>> > > } >>> > > >>> > > - ret =3D vhost_dev_start(&vsc->dev, vdev); >>> > > + ret =3D vhost_dev_start(&vsc->dev, vdev, true); >>> > > if (ret < 0) { >>> > > error_report("Error start vhost dev"); >>> > > goto err_guest_notifiers; >>> > > @@ -101,7 +101,7 @@ void vhost_scsi_common_stop(VHostSCSICommon *vs= c) >>> > > VirtioBusClass *k =3D VIRTIO_BUS_GET_CLASS(qbus); >>> > > int ret =3D 0; >>> > > >>> > > - vhost_dev_stop(&vsc->dev, vdev); >>> > > + vhost_dev_stop(&vsc->dev, vdev, true); >>> > > >>> > > if (k->set_guest_notifiers) { >>> > > ret =3D k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs= , false); >>> > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c >>> > > index dc4014cdef..d97b179e6f 100644 >>> > > --- a/hw/virtio/vhost-user-fs.c >>> > > +++ b/hw/virtio/vhost-user-fs.c >>> > > @@ -76,7 +76,7 @@ static void vuf_start(VirtIODevice *vdev) >>> > > } >>> > > >>> > > fs->vhost_dev.acked_features =3D vdev->guest_features; >>> > > - ret =3D vhost_dev_start(&fs->vhost_dev, vdev); >>> > > + ret =3D vhost_dev_start(&fs->vhost_dev, vdev, true); >>> > > if (ret < 0) { >>> > > error_report("Error starting vhost: %d", -ret); >>> > > goto err_guest_notifiers; >>> > > @@ -110,7 +110,7 @@ static void vuf_stop(VirtIODevice *vdev) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(&fs->vhost_dev, vdev); >>> > > + vhost_dev_stop(&fs->vhost_dev, vdev, true); >>> > > >>> > > ret =3D k->set_guest_notifiers(qbus->parent, fs->vhost_dev.nvq= s, false); >>> > > if (ret < 0) { >>> > > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpi= o.c >>> > > index 5851cb3bc9..0b40ebd15a 100644 >>> > > --- a/hw/virtio/vhost-user-gpio.c >>> > > +++ b/hw/virtio/vhost-user-gpio.c >>> > > @@ -81,7 +81,7 @@ static int vu_gpio_start(VirtIODevice *vdev) >>> > > */ >>> > > vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest= _features); >>> > > >>> > > - ret =3D vhost_dev_start(&gpio->vhost_dev, vdev); >>> > > + ret =3D vhost_dev_start(&gpio->vhost_dev, vdev, false); >>> > > if (ret < 0) { >>> > > error_report("Error starting vhost-user-gpio: %d", ret); >>> > > goto err_guest_notifiers; >>> > > @@ -139,7 +139,7 @@ static void vu_gpio_stop(VirtIODevice *vdev) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(vhost_dev, vdev); >>> > > + vhost_dev_stop(vhost_dev, vdev, false); >>> > > >>> > > ret =3D k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, = false); >>> > > if (ret < 0) { >>> > > diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c >>> > > index 1c9f3d20dc..dc5c828ba6 100644 >>> > > --- a/hw/virtio/vhost-user-i2c.c >>> > > +++ b/hw/virtio/vhost-user-i2c.c >>> > > @@ -46,7 +46,7 @@ static void vu_i2c_start(VirtIODevice *vdev) >>> > > >>> > > i2c->vhost_dev.acked_features =3D vdev->guest_features; >>> > > >>> > > - ret =3D vhost_dev_start(&i2c->vhost_dev, vdev); >>> > > + ret =3D vhost_dev_start(&i2c->vhost_dev, vdev, true); >>> > > if (ret < 0) { >>> > > error_report("Error starting vhost-user-i2c: %d", -ret); >>> > > goto err_guest_notifiers; >>> > > @@ -80,7 +80,7 @@ static void vu_i2c_stop(VirtIODevice *vdev) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(&i2c->vhost_dev, vdev); >>> > > + vhost_dev_stop(&i2c->vhost_dev, vdev, true); >>> > > >>> > > ret =3D k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nv= qs, false); >>> > > if (ret < 0) { >>> > > diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c >>> > > index f9084cde58..201a39e220 100644 >>> > > --- a/hw/virtio/vhost-user-rng.c >>> > > +++ b/hw/virtio/vhost-user-rng.c >>> > > @@ -47,7 +47,7 @@ static void vu_rng_start(VirtIODevice *vdev) >>> > > } >>> > > >>> > > rng->vhost_dev.acked_features =3D vdev->guest_features; >>> > > - ret =3D vhost_dev_start(&rng->vhost_dev, vdev); >>> > > + ret =3D vhost_dev_start(&rng->vhost_dev, vdev, true); >>> > > if (ret < 0) { >>> > > error_report("Error starting vhost-user-rng: %d", -ret); >>> > > goto err_guest_notifiers; >>> > > @@ -81,7 +81,7 @@ static void vu_rng_stop(VirtIODevice *vdev) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(&rng->vhost_dev, vdev); >>> > > + vhost_dev_stop(&rng->vhost_dev, vdev, true); >>> > > >>> > > ret =3D k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nv= qs, false); >>> > > if (ret < 0) { >>> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock= -common.c >>> > > index a67a275de2..d21c72b401 100644 >>> > > --- a/hw/virtio/vhost-vsock-common.c >>> > > +++ b/hw/virtio/vhost-vsock-common.c >>> > > @@ -70,7 +70,7 @@ int vhost_vsock_common_start(VirtIODevice *vdev) >>> > > } >>> > > >>> > > vvc->vhost_dev.acked_features =3D vdev->guest_features; >>> > > - ret =3D vhost_dev_start(&vvc->vhost_dev, vdev); >>> > > + ret =3D vhost_dev_start(&vvc->vhost_dev, vdev, true); >>> > > if (ret < 0) { >>> > > error_report("Error starting vhost: %d", -ret); >>> > > goto err_guest_notifiers; >>> > > @@ -105,7 +105,7 @@ void vhost_vsock_common_stop(VirtIODevice *vdev) >>> > > return; >>> > > } >>> > > >>> > > - vhost_dev_stop(&vvc->vhost_dev, vdev); >>> > > + vhost_dev_stop(&vvc->vhost_dev, vdev, true); >>> > > >>> > > ret =3D k->set_guest_notifiers(qbus->parent, vvc->vhost_dev.nv= qs, false); >>> > > if (ret < 0) { >>> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> > > index d1c4c20b8c..7fb008bc9e 100644 >>> > > --- a/hw/virtio/vhost.c >>> > > +++ b/hw/virtio/vhost.c >>> > > @@ -1777,15 +1777,36 @@ int vhost_dev_get_inflight(struct vhost_dev= *dev, uint16_t queue_size, >>> > > return 0; >>> > > } >>> > > >>> > > +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int = enable) >>> > >>> > There is a similarly-named vhost_set_vring_enable(NetClientState *nc, >>> > int enable) function which is actually part of vhost_net. Please rena= me >>> > it to vhost_net_set_vring_enable(). >>> >>> Should I rename it in this patch? >>> >>> > It should probably call >>> > vhost_dev_set_vring_enable(). >>> >>> Ehm, the idea of this patch was to touch as little as possible to avoid= new >>> regressions. >>> >>> Also, the semantics of vhost_dev_set_vring_enable() was meant to keep >>> vhost_dev_start()/vhost_dev_stop() simple, not to be exposed to fronten= ds. >>> (maybe I should have written it, sorry about that). >>> >>> However I agree that we should clean up vhost-net and also the other >>> frontends as Raphael also suggested, but honestly I'm scared to do that= now >>> in this patch... >>> >>> What I would have wanted to do, would be similar to what we do for >>> vhost-vdpa: call SET_VRING_ENABLE in the vhost_ops->vhost_dev_start() >>> callback of vhost-user.c. >>> Removing all the call to vhost_ops->vhost_set_vring_enable() in the >>> frontends, but I think it's too risky to do that now. >>> >>> > >>> > > +{ >>> > > + if (!hdev->vhost_ops->vhost_set_vring_enable) { >>> > > + return 0; >>> > > + } >>> > > + >>> > > + /* >>> > > + * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES h= as not >>> > > + * been negotiated, the rings start directly in the enabled st= ate, and >>> > > + * .vhost_set_vring_enable callback will fail since >>> > > + * VHOST_USER_SET_VRING_ENABLE is not supported. >>> > > + */ >>> > > + if (hdev->vhost_ops->backend_type =3D=3D VHOST_BACKEND_TYPE_US= ER && >>> > > + !virtio_has_feature(hdev->backend_features, >>> > > + VHOST_USER_F_PROTOCOL_FEATURES)) { >>> > > + return 0; >>> > > + } >>> > >>> > These semantics are the opposite of vhost_user_set_vring_enable(): >>> > >>> > if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURE= S)) { >>> > return -EINVAL; >>> > } >>> > >>> > Please make vhost_user_set_vring_enable() and >>> > vhost_dev_set_vring_enable() consistent. Code gets really confusing w= hen >>> > layers have different semantics for the same operation. >>> >>> It's the opposite precisely because we shouldn't let >>> vhost_dev_start()/vhost_dev_stop() fail if >>> vhost_ops->vhost_set_vring_enable() can't be called because it would fa= il. >>> >>> If I do it this way, then I have to put the check inside >>> vhost_dev_start()/vhost_dev_stop(), and at this point I remove the func= tion >>> that would be useless (just a wrapper of >>> hdev->vhost_ops->vhost_set_vring_enable). >>> Actually this was the first implementation I did, then I added the func= tion >>> just to have vhost_dev_start()/vhost_dev_stop() cleaner and to avoid >>> duplicating the check. >>> >>> > >>> > > + >>> > > + return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable); >>> > > +} >>> > >>> > The return value is hard to understand. An error return is only retur= ned >>> > by vhost-user devices with VHOST_USER_F_PROTOCOL_FEATURES. There are >>> > other cases that seem like they should return an error but return >>> > success instead. For example, when called with enable=3Dfalse on a >>> > non-VHOST_USER_F_PROTOCOL_FEATURES device (e.g. vhost-kernel or legacy >>> > vhost-user) we return success even though the vring wasn't disabled. >>> >>> As I explained above, the idea was not to expose this function outside,= but >>> to use it only in vhost_dev_start()/vhost_dev_stop(). So the return val= ue is >>> 0 both when it has successes and when there is no need/way to enable/di= sable >>> the vrings. >>> >>> Perhaps since it is confusing, I will remove the function and put the c= ode >>> directly into vhost_dev_start()/vhost_dev_stop(). >>> >>> What do you think? >> >>It's late now. We can merge it as-is. >> >>I think this patch makes the vhost code even harder to understand and >>it's important to do the clean ups that have been discussed for 8.0. > > Agree. > >>Will you work on the changes we discussed for 8.0? > > Yep, sure. > I will try to unify all vhost/vhost-user devices. > Now I think it's also a mess because the devices do different things, > we should have everything in the core. Do you think rust-vmm's vhost crates have enough of the state management to manage vhost and vhost-user backends? Maybe it would be a good experiment in replacing a (small well defined) piece of functionality with rust? That said there is a lot of deep magic in the vhost-net stuff which I think is down to the interaction with things like vdpk and other network optimisations that might be missing. For the rest of the devices most of the code is basically boiler plate which has grown variations due to code motion and change. This is the sort of thing that generics solves well. > > Thanks, > Stefano --=20 Alex Benn=C3=A9e