* [PATCH v3 01/23] vhost-user: rework enabling vrings
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 02/23] vhost: drop backend_features field Vladimir Sementsov-Ogievskiy
` (22 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz, Gonglei (Arei), Zhenwei Pi, Jason Wang
We call the handler almost the same way in three places:
- cryptodev-vhost.c
- vhost_net.c
- vhost.c
The only difference, is that in vhost.c we don't try to call the handler
for old vhost-user (when VHOST_USER_F_PROTOCOL_FEATURES is not supported).
cryptodev-vhost and vhost_net code will just fail in this case. Probably
they were developed only for newer vhost-user. Anyway, it doesn't seem
correct to rely on this error path, if these devices want to check,
that they don't communicate to old vhost-user protocol, they should
do that earlier.
Let's create the common helper, to call .vhost_set_vring_enable and
use in all three places. For vhost-user let's just always skip
enable/disable if it's unsupported.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
backends/cryptodev-vhost.c | 8 +-------
hw/net/vhost_net.c | 7 +------
hw/virtio/vhost-user.c | 7 ++++++-
hw/virtio/vhost.c | 21 ---------------------
include/hw/virtio/vhost.h | 9 +++++++++
5 files changed, 17 insertions(+), 35 deletions(-)
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 943680a23a..abdfce33af 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -152,7 +152,6 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
{
CryptoDevBackendVhost *crypto =
cryptodev_get_vhost(cc, b, queue);
- const VhostOps *vhost_ops;
cc->vring_enable = enable;
@@ -160,12 +159,7 @@ vhost_set_vring_enable(CryptoDevBackendClient *cc,
return 0;
}
- vhost_ops = crypto->dev.vhost_ops;
- if (vhost_ops->vhost_set_vring_enable) {
- return vhost_ops->vhost_set_vring_enable(&crypto->dev, enable);
- }
-
- return 0;
+ return vhost_dev_set_vring_enable(&crypto->dev, enable);
}
int cryptodev_vhost_start(VirtIODevice *dev, int total_queues)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index a8ee18a912..25e9f1fd24 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -587,7 +587,6 @@ VHostNetState *get_vhost_net(NetClientState *nc)
int vhost_net_set_vring_enable(NetClientState *nc, int enable)
{
VHostNetState *net = get_vhost_net(nc);
- const VhostOps *vhost_ops = net->dev.vhost_ops;
/*
* vhost-vdpa network devices need to enable dataplane virtqueues after
@@ -601,11 +600,7 @@ int vhost_net_set_vring_enable(NetClientState *nc, int enable)
nc->vring_enable = enable;
- if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
- return vhost_ops->vhost_set_vring_enable(&net->dev, enable);
- }
-
- return 0;
+ return vhost_dev_set_vring_enable(&net->dev, enable);
}
int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 36c9c2e04d..f367ce06ce 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1235,7 +1235,12 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
int i;
if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
- return -EINVAL;
+ /*
+ * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
+ * been negotiated, the rings start directly in the enabled state,
+ * and can't be disabled.
+ */
+ return 0;
}
for (i = 0; i < dev->nvqs; ++i) {
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 266a11514a..414a48a218 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2013,27 +2013,6 @@ 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)
-{
- if (!hdev->vhost_ops->vhost_set_vring_enable) {
- return 0;
- }
-
- /*
- * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
- * been negotiated, the rings start directly in the enabled state, and
- * .vhost_set_vring_enable callback will fail since
- * VHOST_USER_SET_VRING_ENABLE is not supported.
- */
- if (hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER &&
- !virtio_has_feature(hdev->backend_features,
- VHOST_USER_F_PROTOCOL_FEATURES)) {
- return 0;
- }
-
- return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
-}
-
/*
* Host notifiers must be enabled at this point.
*
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 08bbb4dfe9..1ee639dd7e 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -215,6 +215,15 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)
return hdev->started;
}
+static inline int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
+{
+ if (!hdev->vhost_ops->vhost_set_vring_enable) {
+ return 0;
+ }
+
+ return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
+}
+
/**
* vhost_dev_start() - start the vhost device
* @hdev: common vhost_dev structure
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 02/23] vhost: drop backend_features field
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 01/23] vhost-user: rework enabling vrings Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 03/23] vhost-user: introduce vhost_user_has_protocol_feature() helper Vladimir Sementsov-Ogievskiy
` (21 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
devel, Markus Armbruster, Raphael Norwitz, Kevin Wolf,
Hanna Reitz, Jason Wang, Paolo Bonzini, Fam Zheng, Eric Blake,
open list:Block layer core
This field is mostly unused and sometimes confusing. We even have
a TODO-like comment to drop it, the comment is removed in this commit.
The field is used to held VHOST_USER_F_PROTOCOL_FEATURES for vhost-user
and/or VHOST_NET_F_VIRTIO_NET_HDR for vhost-net (which may be
vhost-user-net). But we can simply recalculate these two flags in place
from hdev->features, and from net-client for VHOST_NET_F_VIRTIO_NET_HDR.
Note: removing field from x-query-virtio-status result is incompatible
change. We can do it because the command is unstable.
Cc: devel@lists.libvirt.org
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Acked-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Acked-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/block/vhost-user-blk.c | 1 -
hw/net/vhost_net.c | 26 ++++++++++++++------------
hw/scsi/vhost-scsi.c | 1 -
hw/scsi/vhost-user-scsi.c | 1 -
hw/virtio/vdpa-dev.c | 1 -
hw/virtio/vhost-user.c | 17 ++++++++---------
hw/virtio/virtio-hmp-cmds.c | 2 --
hw/virtio/virtio-qmp.c | 4 ----
include/hw/virtio/vhost.h | 7 -------
qapi/virtio.json | 3 ---
10 files changed, 22 insertions(+), 41 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index c0cc5f6942..de7a810c93 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -348,7 +348,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
s->dev.nvqs = s->num_queues;
s->dev.vqs = s->vhost_vqs;
s->dev.vq_index = 0;
- s->dev.backend_features = 0;
vhost_dev_set_config_notifier(&s->dev, &blk_ops);
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 25e9f1fd24..fda90e231e 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -52,8 +52,14 @@ int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
void vhost_net_ack_features_ex(struct vhost_net *net, const uint64_t *features)
{
- virtio_features_copy(net->dev.acked_features_ex,
- net->dev.backend_features_ex);
+ virtio_features_clear(net->dev.acked_features_ex);
+ if (net->backend == -1) {
+ net->dev.acked_features =
+ net->dev.features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+ } else if (!qemu_has_vnet_hdr(net->nc)) {
+ net->dev.acked_features = 1ULL << VHOST_NET_F_VIRTIO_NET_HDR;
+ }
+
vhost_ack_features_ex(&net->dev, net->feature_bits, features);
}
@@ -258,12 +264,9 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
if (r < 0) {
goto fail;
}
- net->dev.backend_features = qemu_has_vnet_hdr(options->net_backend)
- ? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
net->backend = r;
net->dev.protocol_features = 0;
} else {
- virtio_features_clear(net->dev.backend_features_ex);
net->dev.protocol_features = 0;
net->backend = -1;
@@ -284,13 +287,12 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
net->dev.features &= ~(1ULL << VIRTIO_NET_F_MRG_RXBUF);
}
- if (virtio_features_andnot(missing_features,
- net->dev.backend_features_ex,
- net->dev.features_ex)) {
- fprintf(stderr, "vhost lacks feature mask 0x" VIRTIO_FEATURES_FMT
- " for backend\n", VIRTIO_FEATURES_PR(missing_features));
- goto fail;
- }
+ if (!qemu_has_vnet_hdr(options->net_backend) &&
+ (~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR))) {
+ fprintf(stderr, "vhost lacks feature mask 0x%llx for backend\n",
+ ~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR));
+ goto fail;
+ }
}
/* Set sane init value. Override when guest acks. */
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index cdf405b0f8..d694a25fe2 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -276,7 +276,6 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs);
vsc->dev.vqs = vqs;
vsc->dev.vq_index = 0;
- vsc->dev.backend_features = 0;
ret = vhost_dev_init(&vsc->dev, (void *)(uintptr_t)vhostfd,
VHOST_BACKEND_TYPE_KERNEL, 0, errp);
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 25f2d894e7..0c80a271d8 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -159,7 +159,6 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
vsc->dev.vqs = s->vhost_vqs;
vsc->dev.vq_index = 0;
- vsc->dev.backend_features = 0;
ret = vhost_dev_init(&vsc->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
errp);
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 4a7b970976..efd9f68420 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -104,7 +104,6 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
v->dev.vqs = vqs;
v->dev.vq_index = 0;
v->dev.vq_index_end = v->dev.nvqs;
- v->dev.backend_features = 0;
v->started = false;
ret = vhost_vdpa_get_iova_range(v->vhostfd, &iova_range);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f367ce06ce..b80174f489 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1448,14 +1448,15 @@ static int vhost_user_set_features(struct vhost_dev *dev,
int ret;
/*
- * We need to include any extra backend only feature bits that
- * might be needed by our device. Currently this includes the
- * VHOST_USER_F_PROTOCOL_FEATURES bit for enabling protocol
- * features.
+ * Don't lose VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user
+ * specific.
*/
- ret = vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
- features | dev->backend_features,
- log_enabled);
+ if (virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
+ features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+ }
+
+ ret = vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+ log_enabled);
if (virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_STATUS)) {
@@ -2189,8 +2190,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
uint64_t protocol_features;
- dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
-
err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
&protocol_features);
if (err < 0) {
diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
index 1daae482d3..4bf9a3109d 100644
--- a/hw/virtio/virtio-hmp-cmds.c
+++ b/hw/virtio/virtio-hmp-cmds.c
@@ -176,8 +176,6 @@ void hmp_virtio_status(Monitor *mon, const QDict *qdict)
hmp_virtio_dump_features(mon, s->vhost_dev->features);
monitor_printf(mon, " Acked features:\n");
hmp_virtio_dump_features(mon, s->vhost_dev->acked_features);
- monitor_printf(mon, " Backend features:\n");
- hmp_virtio_dump_features(mon, s->vhost_dev->backend_features);
monitor_printf(mon, " Protocol features:\n");
hmp_virtio_dump_protocols(mon, s->vhost_dev->protocol_features);
}
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index b338344c6c..dd1a38e792 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -780,8 +780,6 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
vdev->guest_features_ex);
status->host_features = qmp_decode_features(vdev->device_id,
vdev->host_features_ex);
- status->backend_features = qmp_decode_features(vdev->device_id,
- vdev->backend_features_ex);
switch (vdev->device_endian) {
case VIRTIO_DEVICE_ENDIAN_LITTLE:
@@ -822,8 +820,6 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
qmp_decode_features(vdev->device_id, hdev->features_ex);
status->vhost_dev->acked_features =
qmp_decode_features(vdev->device_id, hdev->acked_features_ex);
- status->vhost_dev->backend_features =
- qmp_decode_features(vdev->device_id, hdev->backend_features_ex);
status->vhost_dev->protocol_features =
qmp_decode_protocols(hdev->protocol_features);
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 1ee639dd7e..3e69e47833 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -100,16 +100,9 @@ struct vhost_dev {
*
* @features: available features provided by the backend
* @acked_features: final negotiated features with front-end driver
- *
- * @backend_features: this is used in a couple of places to either
- * store VHOST_USER_F_PROTOCOL_FEATURES to apply to
- * VHOST_USER_SET_FEATURES or VHOST_NET_F_VIRTIO_NET_HDR. Its
- * future use should be discouraged and the variable retired as
- * its easy to confuse with the VirtIO backend_features.
*/
VIRTIO_DECLARE_FEATURES(features);
VIRTIO_DECLARE_FEATURES(acked_features);
- VIRTIO_DECLARE_FEATURES(backend_features);
/**
* @protocol_features: is the vhost-user only feature set by
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 05295ab665..b995a5bb6d 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -85,8 +85,6 @@
#
# @acked-features: vhost_dev acked_features
#
-# @backend-features: vhost_dev backend_features
-#
# @protocol-features: vhost_dev protocol_features
#
# @max-queues: vhost_dev max_queues
@@ -106,7 +104,6 @@
'vq-index': 'int',
'features': 'VirtioDeviceFeatures',
'acked-features': 'VirtioDeviceFeatures',
- 'backend-features': 'VirtioDeviceFeatures',
'protocol-features': 'VhostDeviceProtocols',
'max-queues': 'uint64',
'backend-cap': 'uint64',
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 03/23] vhost-user: introduce vhost_user_has_protocol_feature() helper
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 01/23] vhost-user: rework enabling vrings Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 02/23] vhost: drop backend_features field Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 04/23] vhost: move protocol_features to vhost_user Vladimir Sementsov-Ogievskiy
` (20 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Make all protocol feature checks in the same way.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/virtio/vhost-user.c | 120 +++++++++++++++++++++--------------------
1 file changed, 62 insertions(+), 58 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b80174f489..b8231dcbcf 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -272,6 +272,12 @@ struct scrub_regions {
int fd_idx;
};
+static bool vhost_user_has_protocol_feature(struct vhost_dev *dev,
+ uint64_t feature)
+{
+ return virtio_has_feature(dev->protocol_features, feature);
+}
+
static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
{
struct vhost_user *u = dev->opaque;
@@ -435,8 +441,8 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
{
int fds[VHOST_USER_MAX_RAM_SLOTS];
size_t fd_num = 0;
- bool shmfd = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_LOG_SHMFD);
+ bool shmfd =
+ vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_LOG_SHMFD);
int ret;
VhostUserMsg msg = {
.hdr.request = VHOST_USER_SET_LOG_BASE,
@@ -1006,11 +1012,11 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
size_t fd_num = 0;
bool do_postcopy = u->postcopy_listen && u->postcopy_fd.handler;
- bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
+ bool reply_supported =
+ vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK);
bool config_mem_slots =
- virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS);
+ vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS);
int ret;
if (do_postcopy) {
@@ -1058,8 +1064,9 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
static int vhost_user_set_vring_endian(struct vhost_dev *dev,
struct vhost_vring_state *ring)
{
- bool cross_endian = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CROSS_ENDIAN);
+ bool cross_endian =
+ vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_CROSS_ENDIAN);
VhostUserMsg msg = {
.hdr.request = VHOST_USER_SET_VRING_ENDIAN,
.hdr.flags = VHOST_USER_VERSION,
@@ -1129,8 +1136,9 @@ static int vhost_user_write_sync(struct vhost_dev *dev, VhostUserMsg *msg,
int ret;
if (wait_for_reply) {
- bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
+ bool reply_supported =
+ vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_REPLY_ACK);
if (reply_supported) {
msg->hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
}
@@ -1458,8 +1466,7 @@ static int vhost_user_set_features(struct vhost_dev *dev,
ret = vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
log_enabled);
- if (virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_STATUS)) {
+ if (vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_STATUS)) {
if (!ret) {
return vhost_user_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
}
@@ -1513,8 +1520,8 @@ static int vhost_user_reset_device(struct vhost_dev *dev)
* Historically, reset was not implemented so only reset devices
* that are expecting it.
*/
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
return -ENOSYS;
}
@@ -1571,8 +1578,8 @@ static int vhost_user_backend_handle_vring_host_notifier(struct vhost_dev *dev,
void *addr;
char *name;
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_HOST_NOTIFIER) ||
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_HOST_NOTIFIER) ||
vdev == NULL || queue_idx >= virtio_get_num_queues(vdev)) {
return -EINVAL;
}
@@ -1884,13 +1891,13 @@ static int vhost_setup_backend_channel(struct vhost_dev *dev)
};
struct vhost_user *u = dev->opaque;
int sv[2], ret = 0;
- bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
+ bool reply_supported =
+ vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK);
Error *local_err = NULL;
QIOChannel *ioc;
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_BACKEND_REQ)) {
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ)) {
return 0;
}
@@ -2138,8 +2145,8 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
switch (pnd->reason) {
case POSTCOPY_NOTIFY_PROBE:
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_PAGEFAULT)) {
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_PAGEFAULT)) {
/* TODO: Get the device name into this error somehow */
error_setg(errp,
"vhost-user backend not capable of postcopy");
@@ -2230,7 +2237,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
}
/* query the max queues we support if backend supports Multiple Queue */
- if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)) {
+ if (vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_MQ)) {
err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM,
&dev->max_queues);
if (err < 0) {
@@ -2248,18 +2255,18 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
}
if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) &&
- !(virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_BACKEND_REQ) &&
- virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
+ !(vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ) &&
+ vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
error_setg(errp, "IOMMU support requires reply-ack and "
"backend-req protocol features.");
return -EINVAL;
}
/* get max memory regions if backend supports configurable RAM slots */
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS)) {
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS)) {
u->user->memory_slots = VHOST_MEMORY_BASELINE_NREGIONS;
} else {
err = vhost_user_get_max_memslots(dev, &ram_slots);
@@ -2281,8 +2288,8 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
}
if (dev->migration_blocker == NULL &&
- !virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_LOG_SHMFD)) {
+ !vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_LOG_SHMFD)) {
error_setg(&dev->migration_blocker,
"Migration disabled: vhost-user backend lacks "
"VHOST_USER_PROTOCOL_F_LOG_SHMFD feature.");
@@ -2351,8 +2358,8 @@ static bool vhost_user_requires_shm_log(struct vhost_dev *dev)
{
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
- return virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_LOG_SHMFD);
+ return vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_LOG_SHMFD);
}
static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
@@ -2367,8 +2374,7 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
}
/* if backend supports VHOST_USER_PROTOCOL_F_RARP ask it to send the RARP */
- if (virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_RARP)) {
+ if (vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_RARP)) {
msg.hdr.request = VHOST_USER_SEND_RARP;
msg.hdr.flags = VHOST_USER_VERSION;
memcpy((char *)&msg.payload.u64, mac_addr, 6);
@@ -2382,11 +2388,11 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
{
VhostUserMsg msg;
- bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
+ bool reply_supported =
+ vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK);
int ret;
- if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU))) {
+ if (!vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_NET_MTU)) {
return 0;
}
@@ -2446,8 +2452,7 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
.hdr.size = VHOST_USER_CONFIG_HDR_SIZE + config_len,
};
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIG)) {
+ if (!vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_CONFIG)) {
error_setg(errp, "VHOST_USER_PROTOCOL_F_CONFIG not supported");
return -EINVAL;
}
@@ -2490,8 +2495,8 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
{
int ret;
uint8_t *p;
- bool reply_supported = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_REPLY_ACK);
+ bool reply_supported =
+ vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK);
VhostUserMsg msg = {
.hdr.request = VHOST_USER_SET_CONFIG,
@@ -2499,8 +2504,7 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
.hdr.size = VHOST_USER_CONFIG_HDR_SIZE + size,
};
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIG)) {
+ if (!vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_CONFIG)) {
return -ENOTSUP;
}
@@ -2535,8 +2539,9 @@ static int vhost_user_crypto_create_session(struct vhost_dev *dev,
uint64_t *session_id)
{
int ret;
- bool crypto_session = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CRYPTO_SESSION);
+ bool crypto_session =
+ vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION);
CryptoDevBackendSessionInfo *backend_info = session_info;
VhostUserMsg msg = {
.hdr.request = VHOST_USER_CREATE_CRYPTO_SESSION,
@@ -2637,8 +2642,9 @@ static int
vhost_user_crypto_close_session(struct vhost_dev *dev, uint64_t session_id)
{
int ret;
- bool crypto_session = virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_CRYPTO_SESSION);
+ bool crypto_session =
+ vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_CRYPTO_SESSION);
VhostUserMsg msg = {
.hdr.request = VHOST_USER_CLOSE_CRYPTO_SESSION,
.hdr.flags = VHOST_USER_VERSION,
@@ -2683,8 +2689,8 @@ static int vhost_user_get_inflight_fd(struct vhost_dev *dev,
.hdr.size = sizeof(msg.payload.inflight),
};
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
return 0;
}
@@ -2751,8 +2757,8 @@ static int vhost_user_set_inflight_fd(struct vhost_dev *dev,
.hdr.size = sizeof(msg.payload.inflight),
};
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+ if (!vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
return 0;
}
@@ -2851,8 +2857,7 @@ void vhost_user_async_close(DeviceState *d,
static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
{
- if (!virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_STATUS)) {
+ if (!vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_STATUS)) {
return 0;
}
@@ -2877,16 +2882,15 @@ static void vhost_user_reset_status(struct vhost_dev *dev)
return;
}
- if (virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_STATUS)) {
+ if (vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_STATUS)) {
vhost_user_set_status(dev, 0);
}
}
static bool vhost_user_supports_device_state(struct vhost_dev *dev)
{
- return virtio_has_feature(dev->protocol_features,
- VHOST_USER_PROTOCOL_F_DEVICE_STATE);
+ return vhost_user_has_protocol_feature(
+ dev, VHOST_USER_PROTOCOL_F_DEVICE_STATE);
}
static int vhost_user_set_device_state_fd(struct vhost_dev *dev,
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 04/23] vhost: move protocol_features to vhost_user
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 03/23] vhost-user: introduce vhost_user_has_protocol_feature() helper Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 05/23] vhost-user-gpu: drop code duplication Vladimir Sementsov-Ogievskiy
` (19 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz, Gonglei (Arei), Zhenwei Pi, Jason Wang
As comment says: it's only for vhost-user. So, let's move it
to corresponding vhost backend realization.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
backends/cryptodev-vhost.c | 1 -
hw/net/vhost_net.c | 2 --
hw/virtio/vhost-user.c | 26 +++++++++++++++++++++++---
hw/virtio/virtio-qmp.c | 6 ++++--
include/hw/virtio/vhost-user.h | 3 +++
include/hw/virtio/vhost.h | 8 --------
6 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index abdfce33af..c6069f4e5b 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -60,7 +60,6 @@ cryptodev_vhost_init(
crypto->cc = options->cc;
- crypto->dev.protocol_features = 0;
crypto->backend = -1;
/* vhost-user needs vq_index to initiate a specific queue pair */
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index fda90e231e..ca19983126 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -265,9 +265,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
goto fail;
}
net->backend = r;
- net->dev.protocol_features = 0;
} else {
- net->dev.protocol_features = 0;
net->backend = -1;
/* vhost-user needs vq_index to initiate a specific queue pair */
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b8231dcbcf..3fd11a3b57 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -11,6 +11,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/virtio/virtio-dmabuf.h"
+#include "hw/virtio/virtio-qmp.h"
#include "hw/virtio/vhost.h"
#include "hw/virtio/virtio-crypto.h"
#include "hw/virtio/vhost-user.h"
@@ -264,6 +265,14 @@ struct vhost_user {
/* Our current regions */
int num_shadow_regions;
struct vhost_memory_region shadow_regions[VHOST_USER_MAX_RAM_SLOTS];
+
+ /**
+ * @protocol_features: the vhost-user protocol feature set by
+ * VHOST_USER_SET_PROTOCOL_FEATURES. Protocol features are only
+ * negotiated if VHOST_USER_F_PROTOCOL_FEATURES has been offered
+ * by the backend (see @features).
+ */
+ uint64_t protocol_features;
};
struct scrub_regions {
@@ -275,7 +284,8 @@ struct scrub_regions {
static bool vhost_user_has_protocol_feature(struct vhost_dev *dev,
uint64_t feature)
{
- return virtio_has_feature(dev->protocol_features, feature);
+ struct vhost_user *u = dev->opaque;
+ return virtio_has_feature(u->protocol_features, feature);
}
static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
@@ -2229,8 +2239,8 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
}
/* final set of protocol features */
- dev->protocol_features = protocol_features;
- err = vhost_user_set_protocol_features(dev, dev->protocol_features);
+ u->protocol_features = protocol_features;
+ err = vhost_user_set_protocol_features(dev, u->protocol_features);
if (err < 0) {
error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
return -EPROTO;
@@ -3021,6 +3031,16 @@ static int vhost_user_check_device_state(struct vhost_dev *dev, Error **errp)
return 0;
}
+void vhost_user_qmp_status(struct vhost_dev *dev, VirtioStatus *status)
+{
+ struct vhost_user *u = dev->opaque;
+
+ assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
+
+ status->vhost_dev->protocol_features =
+ qmp_decode_protocols(u->protocol_features);
+}
+
const VhostOps user_ops = {
.backend_type = VHOST_BACKEND_TYPE_USER,
.vhost_backend_init = vhost_user_backend_init,
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index dd1a38e792..82acb6d232 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -821,12 +821,14 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
status->vhost_dev->acked_features =
qmp_decode_features(vdev->device_id, hdev->acked_features_ex);
- status->vhost_dev->protocol_features =
- qmp_decode_protocols(hdev->protocol_features);
status->vhost_dev->max_queues = hdev->max_queues;
status->vhost_dev->backend_cap = hdev->backend_cap;
status->vhost_dev->log_enabled = hdev->log_enabled;
status->vhost_dev->log_size = hdev->log_size;
+
+ if (hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER) {
+ vhost_user_qmp_status(hdev, status);
+ }
}
return status;
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 9a3f238b43..36d96296a3 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -10,6 +10,7 @@
#include "chardev/char-fe.h"
#include "hw/virtio/virtio.h"
+#include "qapi/qapi-types-virtio.h"
enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_MQ = 0,
@@ -111,4 +112,6 @@ void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
vu_async_close_fn cb);
+void vhost_user_qmp_status(struct vhost_dev *dev, VirtioStatus *status);
+
#endif
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 3e69e47833..e308bc4556 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -104,14 +104,6 @@ struct vhost_dev {
VIRTIO_DECLARE_FEATURES(features);
VIRTIO_DECLARE_FEATURES(acked_features);
- /**
- * @protocol_features: is the vhost-user only feature set by
- * VHOST_USER_SET_PROTOCOL_FEATURES. Protocol features are only
- * negotiated if VHOST_USER_F_PROTOCOL_FEATURES has been offered
- * by the backend (see @features).
- */
- uint64_t protocol_features;
-
uint64_t max_queues;
uint64_t backend_cap;
/* @started: is the vhost device started? */
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 05/23] vhost-user-gpu: drop code duplication
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 04/23] vhost: move protocol_features to vhost_user Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 06/23] vhost: make vhost_dev.features private Vladimir Sementsov-Ogievskiy
` (18 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Philippe Mathieu-Daudé, Raphael Norwitz,
Marc-André Lureau
Obviously, this duplicated fragment doesn't make any sense.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/display/vhost-user-gpu.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 9fc6bbcd2c..79ea64b12c 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -644,10 +644,6 @@ vhost_user_gpu_device_realize(DeviceState *qdev, Error **errp)
VIRTIO_GPU_F_RESOURCE_UUID)) {
g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED;
}
- if (virtio_has_feature(g->vhost->dev.features,
- VIRTIO_GPU_F_RESOURCE_UUID)) {
- g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED;
- }
if (!virtio_gpu_base_device_realize(qdev, NULL, NULL, errp)) {
return;
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 06/23] vhost: make vhost_dev.features private
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 05/23] vhost-user-gpu: drop code duplication Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-17 10:32 ` Daniil Tatianin
2025-10-15 14:57 ` [PATCH v3 07/23] virtio: move common part of _set_guest_notifier to generic code Vladimir Sementsov-Ogievskiy
` (17 subsequent siblings)
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Marc-André Lureau, Jason Wang, Alex Bennée
It's hard to control where and how do we use this field. Let's
cover all usages by getters/setters, and keep direct access to the
field only in vhost.c. It will help to control migration of this
field in further commits.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
hw/display/vhost-user-gpu.c | 7 +++----
hw/net/vhost_net.c | 18 ++++++++---------
hw/virtio/vdpa-dev.c | 2 +-
hw/virtio/vhost-user-base.c | 8 ++++++--
hw/virtio/vhost-user.c | 4 ++--
hw/virtio/vhost.c | 6 +++---
hw/virtio/virtio-qmp.c | 2 +-
include/hw/virtio/vhost.h | 39 +++++++++++++++++++++++++++++++++++--
net/vhost-vdpa.c | 7 +++----
9 files changed, 65 insertions(+), 28 deletions(-)
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 79ea64b12c..146620e0a3 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -631,17 +631,16 @@ vhost_user_gpu_device_realize(DeviceState *qdev, Error **errp)
/* existing backend may send DMABUF, so let's add that requirement */
g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_DMABUF_ENABLED;
- if (virtio_has_feature(g->vhost->dev.features, VIRTIO_GPU_F_VIRGL)) {
+ if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_VIRGL)) {
g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED;
}
- if (virtio_has_feature(g->vhost->dev.features, VIRTIO_GPU_F_EDID)) {
+ if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_EDID)) {
g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_EDID_ENABLED;
} else {
error_report("EDID requested but the backend doesn't support it.");
g->parent_obj.conf.flags &= ~(1 << VIRTIO_GPU_FLAG_EDID_ENABLED);
}
- if (virtio_has_feature(g->vhost->dev.features,
- VIRTIO_GPU_F_RESOURCE_UUID)) {
+ if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_RESOURCE_UUID)) {
g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED;
}
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index ca19983126..323d117735 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -54,8 +54,8 @@ void vhost_net_ack_features_ex(struct vhost_net *net, const uint64_t *features)
{
virtio_features_clear(net->dev.acked_features_ex);
if (net->backend == -1) {
- net->dev.acked_features =
- net->dev.features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+ net->dev.acked_features = (vhost_dev_features(&net->dev) &
+ (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
} else if (!qemu_has_vnet_hdr(net->nc)) {
net->dev.acked_features = 1ULL << VHOST_NET_F_VIRTIO_NET_HDR;
}
@@ -282,15 +282,15 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
if (backend_kernel) {
if (!qemu_has_vnet_hdr_len(options->net_backend,
sizeof(struct virtio_net_hdr_mrg_rxbuf))) {
- net->dev.features &= ~(1ULL << VIRTIO_NET_F_MRG_RXBUF);
+ vhost_dev_clear_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF);
}
if (!qemu_has_vnet_hdr(options->net_backend) &&
- (~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR))) {
- fprintf(stderr, "vhost lacks feature mask 0x%llx for backend\n",
- ~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR));
- goto fail;
- }
+ !vhost_dev_has_feature(&net->dev, VHOST_NET_F_VIRTIO_NET_HDR)) {
+ fprintf(stderr, "vhost lacks VHOST_NET_F_VIRTIO_NET_HDR "
+ "feature for backend\n");
+ goto fail;
+ }
}
/* Set sane init value. Override when guest acks. */
@@ -298,7 +298,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
virtio_features_from_u64(features,
options->get_acked_features(net->nc));
if (virtio_features_andnot(missing_features, features,
- net->dev.features_ex)) {
+ vhost_dev_features_ex(&net->dev))) {
fprintf(stderr, "vhost lacks feature mask 0x" VIRTIO_FEATURES_FMT
" for backend\n", VIRTIO_FEATURES_PR(missing_features));
goto fail;
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index efd9f68420..e1a2ff433d 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -224,7 +224,7 @@ static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
Error **errp)
{
VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
- uint64_t backend_features = s->dev.features;
+ uint64_t backend_features = vhost_dev_features(&s->dev);
if (!virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM)) {
virtio_clear_feature(&backend_features, VIRTIO_F_IOMMU_PLATFORM);
diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
index ff67a020b4..cf311c3bfc 100644
--- a/hw/virtio/vhost-user-base.c
+++ b/hw/virtio/vhost-user-base.c
@@ -118,9 +118,13 @@ static uint64_t vub_get_features(VirtIODevice *vdev,
uint64_t requested_features, Error **errp)
{
VHostUserBase *vub = VHOST_USER_BASE(vdev);
+ uint64_t backend_features = vhost_dev_features(&vub->vhost_dev);
+
/* This should be set when the vhost connection initialises */
- g_assert(vub->vhost_dev.features);
- return vub->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+ g_assert(backend_features);
+ virtio_clear_feature(&backend_features, VHOST_USER_F_PROTOCOL_FEATURES);
+
+ return backend_features;
}
/*
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 3fd11a3b57..9f26515fd4 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1252,7 +1252,7 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
{
int i;
- if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
+ if (!vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
/*
* For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
* been negotiated, the rings start directly in the enabled state,
@@ -1469,7 +1469,7 @@ static int vhost_user_set_features(struct vhost_dev *dev,
* Don't lose VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user
* specific.
*/
- if (virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
+ if (vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
}
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 414a48a218..94efa409aa 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1603,7 +1603,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
}
- virtio_features_copy(hdev->features_ex, features);
+ virtio_features_copy(hdev->_features_ex, features);
hdev->memory_listener = (MemoryListener) {
.name = "vhost",
@@ -1626,7 +1626,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
};
if (hdev->migration_blocker == NULL) {
- if (!virtio_has_feature_ex(hdev->features_ex, VHOST_F_LOG_ALL)) {
+ if (!vhost_dev_has_feature_ex(hdev, VHOST_F_LOG_ALL)) {
error_setg(&hdev->migration_blocker,
"Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");
} else if (vhost_dev_log_is_shared(hdev) && !qemu_memfd_alloc_check()) {
@@ -1900,7 +1900,7 @@ void vhost_get_features_ex(struct vhost_dev *hdev,
const int *bit = feature_bits;
while (*bit != VHOST_INVALID_FEATURE_BIT) {
- if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
+ if (!vhost_dev_has_feature_ex(hdev, *bit)) {
virtio_clear_feature_ex(features, *bit);
}
bit++;
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 82acb6d232..33d32720e1 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -817,7 +817,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
status->vhost_dev->nvqs = hdev->nvqs;
status->vhost_dev->vq_index = hdev->vq_index;
status->vhost_dev->features =
- qmp_decode_features(vdev->device_id, hdev->features_ex);
+ qmp_decode_features(vdev->device_id, vhost_dev_features_ex(hdev));
status->vhost_dev->acked_features =
qmp_decode_features(vdev->device_id, hdev->acked_features_ex);
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index e308bc4556..1ba1af1d86 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -98,10 +98,11 @@ struct vhost_dev {
* offered by a backend which may be a subset of the total
* features eventually offered to the guest.
*
- * @features: available features provided by the backend
+ * @_features: available features provided by the backend, private,
+ * direct access only in vhost.h/vhost.c
* @acked_features: final negotiated features with front-end driver
*/
- VIRTIO_DECLARE_FEATURES(features);
+ VIRTIO_DECLARE_FEATURES(_features);
VIRTIO_DECLARE_FEATURES(acked_features);
uint64_t max_queues;
@@ -403,6 +404,40 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
struct vhost_inflight *inflight);
bool vhost_dev_has_iommu(struct vhost_dev *dev);
+static inline bool vhost_dev_has_feature(struct vhost_dev *dev,
+ uint64_t feature)
+{
+ return virtio_has_feature(dev->_features, feature);
+}
+
+static inline bool vhost_dev_has_feature_ex(struct vhost_dev *dev,
+ uint64_t feature)
+{
+ return virtio_has_feature_ex(dev->_features_ex, feature);
+}
+
+static inline uint64_t vhost_dev_features(struct vhost_dev *dev)
+{
+ return dev->_features;
+}
+
+static inline const uint64_t *vhost_dev_features_ex(struct vhost_dev *dev)
+{
+ return dev->_features_ex;
+}
+
+static inline void vhost_dev_clear_feature(struct vhost_dev *dev,
+ uint64_t feature)
+{
+ virtio_clear_feature(&dev->_features, feature);
+}
+
+static inline void vhost_dev_clear_feature_ex(struct vhost_dev *dev,
+ uint64_t feature)
+{
+ virtio_clear_feature_ex(dev->_features_ex, feature);
+}
+
#ifdef CONFIG_VHOST
int vhost_reset_device(struct vhost_dev *hdev);
#else
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 74d26a9497..0af0d3bdd3 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -256,15 +256,14 @@ static bool vhost_vdpa_get_vnet_hash_supported_types(NetClientState *nc,
{
assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
- uint64_t features = s->vhost_vdpa.dev->features;
int fd = s->vhost_vdpa.shared->device_fd;
struct {
struct vhost_vdpa_config hdr;
uint32_t supported_hash_types;
} config;
- if (!virtio_has_feature(features, VIRTIO_NET_F_HASH_REPORT) &&
- !virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
+ if (!vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_HASH_REPORT) &&
+ !vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_RSS)) {
return false;
}
@@ -585,7 +584,7 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
* If we early return in these cases SVQ will not be enabled. The migration
* will be blocked as long as vhost-vdpa backends will not offer _F_LOG.
*/
- if (!vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
+ if (!vhost_vdpa_net_valid_svq_features(vhost_dev_features(v->dev), NULL)) {
return 0;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 06/23] vhost: make vhost_dev.features private
2025-10-15 14:57 ` [PATCH v3 06/23] vhost: make vhost_dev.features private Vladimir Sementsov-Ogievskiy
@ 2025-10-17 10:32 ` Daniil Tatianin
2025-10-17 17:02 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 36+ messages in thread
From: Daniil Tatianin @ 2025-10-17 10:32 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, mst
Cc: sgarzare, raphael, qemu-devel, yc-core, Marc-André Lureau,
Jason Wang, Alex Bennée
On 10/15/25 5:57 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's hard to control where and how do we use this field. Let's
> cover all usages by getters/setters, and keep direct access to the
> field only in vhost.c. It will help to control migration of this
> field in further commits.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> hw/display/vhost-user-gpu.c | 7 +++----
> hw/net/vhost_net.c | 18 ++++++++---------
> hw/virtio/vdpa-dev.c | 2 +-
> hw/virtio/vhost-user-base.c | 8 ++++++--
> hw/virtio/vhost-user.c | 4 ++--
> hw/virtio/vhost.c | 6 +++---
> hw/virtio/virtio-qmp.c | 2 +-
> include/hw/virtio/vhost.h | 39 +++++++++++++++++++++++++++++++++++--
> net/vhost-vdpa.c | 7 +++----
> 9 files changed, 65 insertions(+), 28 deletions(-)
>
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 79ea64b12c..146620e0a3 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -631,17 +631,16 @@ vhost_user_gpu_device_realize(DeviceState *qdev, Error **errp)
>
> /* existing backend may send DMABUF, so let's add that requirement */
> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_DMABUF_ENABLED;
> - if (virtio_has_feature(g->vhost->dev.features, VIRTIO_GPU_F_VIRGL)) {
> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_VIRGL)) {
> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED;
> }
> - if (virtio_has_feature(g->vhost->dev.features, VIRTIO_GPU_F_EDID)) {
> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_EDID)) {
> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_EDID_ENABLED;
> } else {
> error_report("EDID requested but the backend doesn't support it.");
> g->parent_obj.conf.flags &= ~(1 << VIRTIO_GPU_FLAG_EDID_ENABLED);
> }
> - if (virtio_has_feature(g->vhost->dev.features,
> - VIRTIO_GPU_F_RESOURCE_UUID)) {
> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_RESOURCE_UUID)) {
> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED;
> }
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index ca19983126..323d117735 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -54,8 +54,8 @@ void vhost_net_ack_features_ex(struct vhost_net *net, const uint64_t *features)
> {
> virtio_features_clear(net->dev.acked_features_ex);
> if (net->backend == -1) {
> - net->dev.acked_features =
> - net->dev.features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
> + net->dev.acked_features = (vhost_dev_features(&net->dev) &
> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
> } else if (!qemu_has_vnet_hdr(net->nc)) {
> net->dev.acked_features = 1ULL << VHOST_NET_F_VIRTIO_NET_HDR;
> }
> @@ -282,15 +282,15 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> if (backend_kernel) {
> if (!qemu_has_vnet_hdr_len(options->net_backend,
> sizeof(struct virtio_net_hdr_mrg_rxbuf))) {
> - net->dev.features &= ~(1ULL << VIRTIO_NET_F_MRG_RXBUF);
> + vhost_dev_clear_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF);
> }
>
> if (!qemu_has_vnet_hdr(options->net_backend) &&
> - (~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR))) {
> - fprintf(stderr, "vhost lacks feature mask 0x%llx for backend\n",
> - ~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR));
> - goto fail;
> - }
> + !vhost_dev_has_feature(&net->dev, VHOST_NET_F_VIRTIO_NET_HDR)) {
> + fprintf(stderr, "vhost lacks VHOST_NET_F_VIRTIO_NET_HDR "
> + "feature for backend\n");
> + goto fail;
> + }
> }
>
> /* Set sane init value. Override when guest acks. */
> @@ -298,7 +298,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> virtio_features_from_u64(features,
> options->get_acked_features(net->nc));
> if (virtio_features_andnot(missing_features, features,
> - net->dev.features_ex)) {
> + vhost_dev_features_ex(&net->dev))) {
> fprintf(stderr, "vhost lacks feature mask 0x" VIRTIO_FEATURES_FMT
> " for backend\n", VIRTIO_FEATURES_PR(missing_features));
> goto fail;
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index efd9f68420..e1a2ff433d 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -224,7 +224,7 @@ static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
> Error **errp)
> {
> VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
> - uint64_t backend_features = s->dev.features;
> + uint64_t backend_features = vhost_dev_features(&s->dev);
>
> if (!virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM)) {
> virtio_clear_feature(&backend_features, VIRTIO_F_IOMMU_PLATFORM);
> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> index ff67a020b4..cf311c3bfc 100644
> --- a/hw/virtio/vhost-user-base.c
> +++ b/hw/virtio/vhost-user-base.c
> @@ -118,9 +118,13 @@ static uint64_t vub_get_features(VirtIODevice *vdev,
> uint64_t requested_features, Error **errp)
> {
> VHostUserBase *vub = VHOST_USER_BASE(vdev);
> + uint64_t backend_features = vhost_dev_features(&vub->vhost_dev);
> +
> /* This should be set when the vhost connection initialises */
> - g_assert(vub->vhost_dev.features);
> - return vub->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
> + g_assert(backend_features);
> + virtio_clear_feature(&backend_features, VHOST_USER_F_PROTOCOL_FEATURES);
> +
> + return backend_features;
> }
>
> /*
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 3fd11a3b57..9f26515fd4 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1252,7 +1252,7 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
> {
> int i;
>
> - if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> + if (!vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
> /*
> * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
> * been negotiated, the rings start directly in the enabled state,
> @@ -1469,7 +1469,7 @@ static int vhost_user_set_features(struct vhost_dev *dev,
> * Don't lose VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user
> * specific.
> */
> - if (virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> + if (vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
> features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> }
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 414a48a218..94efa409aa 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1603,7 +1603,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> }
> }
>
> - virtio_features_copy(hdev->features_ex, features);
> + virtio_features_copy(hdev->_features_ex, features);
This should probably use the vhost_dev_features_ex getter
Otherwise, LGTM:
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>
> hdev->memory_listener = (MemoryListener) {
> .name = "vhost",
> @@ -1626,7 +1626,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> };
>
> if (hdev->migration_blocker == NULL) {
> - if (!virtio_has_feature_ex(hdev->features_ex, VHOST_F_LOG_ALL)) {
> + if (!vhost_dev_has_feature_ex(hdev, VHOST_F_LOG_ALL)) {
> error_setg(&hdev->migration_blocker,
> "Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");
> } else if (vhost_dev_log_is_shared(hdev) && !qemu_memfd_alloc_check()) {
> @@ -1900,7 +1900,7 @@ void vhost_get_features_ex(struct vhost_dev *hdev,
> const int *bit = feature_bits;
>
> while (*bit != VHOST_INVALID_FEATURE_BIT) {
> - if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
> + if (!vhost_dev_has_feature_ex(hdev, *bit)) {
> virtio_clear_feature_ex(features, *bit);
> }
> bit++;
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index 82acb6d232..33d32720e1 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -817,7 +817,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> status->vhost_dev->nvqs = hdev->nvqs;
> status->vhost_dev->vq_index = hdev->vq_index;
> status->vhost_dev->features =
> - qmp_decode_features(vdev->device_id, hdev->features_ex);
> + qmp_decode_features(vdev->device_id, vhost_dev_features_ex(hdev));
> status->vhost_dev->acked_features =
> qmp_decode_features(vdev->device_id, hdev->acked_features_ex);
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index e308bc4556..1ba1af1d86 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -98,10 +98,11 @@ struct vhost_dev {
> * offered by a backend which may be a subset of the total
> * features eventually offered to the guest.
> *
> - * @features: available features provided by the backend
> + * @_features: available features provided by the backend, private,
> + * direct access only in vhost.h/vhost.c
> * @acked_features: final negotiated features with front-end driver
> */
> - VIRTIO_DECLARE_FEATURES(features);
> + VIRTIO_DECLARE_FEATURES(_features);
> VIRTIO_DECLARE_FEATURES(acked_features);
>
> uint64_t max_queues;
> @@ -403,6 +404,40 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> struct vhost_inflight *inflight);
> bool vhost_dev_has_iommu(struct vhost_dev *dev);
>
> +static inline bool vhost_dev_has_feature(struct vhost_dev *dev,
> + uint64_t feature)
> +{
> + return virtio_has_feature(dev->_features, feature);
> +}
> +
> +static inline bool vhost_dev_has_feature_ex(struct vhost_dev *dev,
> + uint64_t feature)
> +{
> + return virtio_has_feature_ex(dev->_features_ex, feature);
> +}
> +
> +static inline uint64_t vhost_dev_features(struct vhost_dev *dev)
> +{
> + return dev->_features;
> +}
> +
> +static inline const uint64_t *vhost_dev_features_ex(struct vhost_dev *dev)
> +{
> + return dev->_features_ex;
> +}
> +
> +static inline void vhost_dev_clear_feature(struct vhost_dev *dev,
> + uint64_t feature)
> +{
> + virtio_clear_feature(&dev->_features, feature);
> +}
> +
> +static inline void vhost_dev_clear_feature_ex(struct vhost_dev *dev,
> + uint64_t feature)
> +{
> + virtio_clear_feature_ex(dev->_features_ex, feature);
> +}
> +
> #ifdef CONFIG_VHOST
> int vhost_reset_device(struct vhost_dev *hdev);
> #else
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 74d26a9497..0af0d3bdd3 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -256,15 +256,14 @@ static bool vhost_vdpa_get_vnet_hash_supported_types(NetClientState *nc,
> {
> assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> - uint64_t features = s->vhost_vdpa.dev->features;
> int fd = s->vhost_vdpa.shared->device_fd;
> struct {
> struct vhost_vdpa_config hdr;
> uint32_t supported_hash_types;
> } config;
>
> - if (!virtio_has_feature(features, VIRTIO_NET_F_HASH_REPORT) &&
> - !virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
> + if (!vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_HASH_REPORT) &&
> + !vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_RSS)) {
> return false;
> }
>
> @@ -585,7 +584,7 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
> * If we early return in these cases SVQ will not be enabled. The migration
> * will be blocked as long as vhost-vdpa backends will not offer _F_LOG.
> */
> - if (!vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
> + if (!vhost_vdpa_net_valid_svq_features(vhost_dev_features(v->dev), NULL)) {
> return 0;
> }
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 06/23] vhost: make vhost_dev.features private
2025-10-17 10:32 ` Daniil Tatianin
@ 2025-10-17 17:02 ` Vladimir Sementsov-Ogievskiy
2025-10-31 17:31 ` Daniil Tatianin
0 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-17 17:02 UTC (permalink / raw)
To: Daniil Tatianin, mst
Cc: sgarzare, raphael, qemu-devel, yc-core, Marc-André Lureau,
Jason Wang, Alex Bennée
On 17.10.25 13:32, Daniil Tatianin wrote:
> On 10/15/25 5:57 PM, Vladimir Sementsov-Ogievskiy wrote:
>
>> It's hard to control where and how do we use this field. Let's
>> cover all usages by getters/setters, and keep direct access to the
>> field only in vhost.c. It will help to control migration of this
>> field in further commits.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> hw/display/vhost-user-gpu.c | 7 +++----
>> hw/net/vhost_net.c | 18 ++++++++---------
>> hw/virtio/vdpa-dev.c | 2 +-
>> hw/virtio/vhost-user-base.c | 8 ++++++--
>> hw/virtio/vhost-user.c | 4 ++--
>> hw/virtio/vhost.c | 6 +++---
>> hw/virtio/virtio-qmp.c | 2 +-
>> include/hw/virtio/vhost.h | 39 +++++++++++++++++++++++++++++++++++--
>> net/vhost-vdpa.c | 7 +++----
>> 9 files changed, 65 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
>> index 79ea64b12c..146620e0a3 100644
>> --- a/hw/display/vhost-user-gpu.c
>> +++ b/hw/display/vhost-user-gpu.c
>> @@ -631,17 +631,16 @@ vhost_user_gpu_device_realize(DeviceState *qdev, Error **errp)
>> /* existing backend may send DMABUF, so let's add that requirement */
>> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_DMABUF_ENABLED;
>> - if (virtio_has_feature(g->vhost->dev.features, VIRTIO_GPU_F_VIRGL)) {
>> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_VIRGL)) {
>> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED;
>> }
>> - if (virtio_has_feature(g->vhost->dev.features, VIRTIO_GPU_F_EDID)) {
>> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_EDID)) {
>> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_EDID_ENABLED;
>> } else {
>> error_report("EDID requested but the backend doesn't support it.");
>> g->parent_obj.conf.flags &= ~(1 << VIRTIO_GPU_FLAG_EDID_ENABLED);
>> }
>> - if (virtio_has_feature(g->vhost->dev.features,
>> - VIRTIO_GPU_F_RESOURCE_UUID)) {
>> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_RESOURCE_UUID)) {
>> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED;
>> }
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index ca19983126..323d117735 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -54,8 +54,8 @@ void vhost_net_ack_features_ex(struct vhost_net *net, const uint64_t *features)
>> {
>> virtio_features_clear(net->dev.acked_features_ex);
>> if (net->backend == -1) {
>> - net->dev.acked_features =
>> - net->dev.features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
>> + net->dev.acked_features = (vhost_dev_features(&net->dev) &
>> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
>> } else if (!qemu_has_vnet_hdr(net->nc)) {
>> net->dev.acked_features = 1ULL << VHOST_NET_F_VIRTIO_NET_HDR;
>> }
>> @@ -282,15 +282,15 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>> if (backend_kernel) {
>> if (!qemu_has_vnet_hdr_len(options->net_backend,
>> sizeof(struct virtio_net_hdr_mrg_rxbuf))) {
>> - net->dev.features &= ~(1ULL << VIRTIO_NET_F_MRG_RXBUF);
>> + vhost_dev_clear_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF);
>> }
>> if (!qemu_has_vnet_hdr(options->net_backend) &&
>> - (~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR))) {
>> - fprintf(stderr, "vhost lacks feature mask 0x%llx for backend\n",
>> - ~net->dev.features & (1ULL << VHOST_NET_F_VIRTIO_NET_HDR));
>> - goto fail;
>> - }
>> + !vhost_dev_has_feature(&net->dev, VHOST_NET_F_VIRTIO_NET_HDR)) {
>> + fprintf(stderr, "vhost lacks VHOST_NET_F_VIRTIO_NET_HDR "
>> + "feature for backend\n");
>> + goto fail;
>> + }
>> }
>> /* Set sane init value. Override when guest acks. */
>> @@ -298,7 +298,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>> virtio_features_from_u64(features,
>> options->get_acked_features(net->nc));
>> if (virtio_features_andnot(missing_features, features,
>> - net->dev.features_ex)) {
>> + vhost_dev_features_ex(&net->dev))) {
>> fprintf(stderr, "vhost lacks feature mask 0x" VIRTIO_FEATURES_FMT
>> " for backend\n", VIRTIO_FEATURES_PR(missing_features));
>> goto fail;
>> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>> index efd9f68420..e1a2ff433d 100644
>> --- a/hw/virtio/vdpa-dev.c
>> +++ b/hw/virtio/vdpa-dev.c
>> @@ -224,7 +224,7 @@ static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
>> Error **errp)
>> {
>> VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
>> - uint64_t backend_features = s->dev.features;
>> + uint64_t backend_features = vhost_dev_features(&s->dev);
>> if (!virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM)) {
>> virtio_clear_feature(&backend_features, VIRTIO_F_IOMMU_PLATFORM);
>> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
>> index ff67a020b4..cf311c3bfc 100644
>> --- a/hw/virtio/vhost-user-base.c
>> +++ b/hw/virtio/vhost-user-base.c
>> @@ -118,9 +118,13 @@ static uint64_t vub_get_features(VirtIODevice *vdev,
>> uint64_t requested_features, Error **errp)
>> {
>> VHostUserBase *vub = VHOST_USER_BASE(vdev);
>> + uint64_t backend_features = vhost_dev_features(&vub->vhost_dev);
>> +
>> /* This should be set when the vhost connection initialises */
>> - g_assert(vub->vhost_dev.features);
>> - return vub->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
>> + g_assert(backend_features);
>> + virtio_clear_feature(&backend_features, VHOST_USER_F_PROTOCOL_FEATURES);
>> +
>> + return backend_features;
>> }
>> /*
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 3fd11a3b57..9f26515fd4 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -1252,7 +1252,7 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
>> {
>> int i;
>> - if (!virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>> + if (!vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
>> /*
>> * For vhost-user devices, if VHOST_USER_F_PROTOCOL_FEATURES has not
>> * been negotiated, the rings start directly in the enabled state,
>> @@ -1469,7 +1469,7 @@ static int vhost_user_set_features(struct vhost_dev *dev,
>> * Don't lose VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user
>> * specific.
>> */
>> - if (virtio_has_feature(dev->features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>> + if (vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
>> features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>> }
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 414a48a218..94efa409aa 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1603,7 +1603,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> }
>> }
>> - virtio_features_copy(hdev->features_ex, features);
>> + virtio_features_copy(hdev->_features_ex, features);
>
>
> This should probably use the vhost_dev_features_ex getter
Hmm, here we write into hdev->_features_ex.
>
> Otherwise, LGTM:
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>
>> hdev->memory_listener = (MemoryListener) {
>> .name = "vhost",
>> @@ -1626,7 +1626,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> };
>> if (hdev->migration_blocker == NULL) {
>> - if (!virtio_has_feature_ex(hdev->features_ex, VHOST_F_LOG_ALL)) {
>> + if (!vhost_dev_has_feature_ex(hdev, VHOST_F_LOG_ALL)) {
>> error_setg(&hdev->migration_blocker,
>> "Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");
>> } else if (vhost_dev_log_is_shared(hdev) && !qemu_memfd_alloc_check()) {
>> @@ -1900,7 +1900,7 @@ void vhost_get_features_ex(struct vhost_dev *hdev,
>> const int *bit = feature_bits;
>> while (*bit != VHOST_INVALID_FEATURE_BIT) {
>> - if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
>> + if (!vhost_dev_has_feature_ex(hdev, *bit)) {
>> virtio_clear_feature_ex(features, *bit);
>> }
>> bit++;
>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>> index 82acb6d232..33d32720e1 100644
>> --- a/hw/virtio/virtio-qmp.c
>> +++ b/hw/virtio/virtio-qmp.c
>> @@ -817,7 +817,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
>> status->vhost_dev->nvqs = hdev->nvqs;
>> status->vhost_dev->vq_index = hdev->vq_index;
>> status->vhost_dev->features =
>> - qmp_decode_features(vdev->device_id, hdev->features_ex);
>> + qmp_decode_features(vdev->device_id, vhost_dev_features_ex(hdev));
>> status->vhost_dev->acked_features =
>> qmp_decode_features(vdev->device_id, hdev->acked_features_ex);
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index e308bc4556..1ba1af1d86 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -98,10 +98,11 @@ struct vhost_dev {
>> * offered by a backend which may be a subset of the total
>> * features eventually offered to the guest.
>> *
>> - * @features: available features provided by the backend
>> + * @_features: available features provided by the backend, private,
>> + * direct access only in vhost.h/vhost.c
>> * @acked_features: final negotiated features with front-end driver
>> */
>> - VIRTIO_DECLARE_FEATURES(features);
>> + VIRTIO_DECLARE_FEATURES(_features);
>> VIRTIO_DECLARE_FEATURES(acked_features);
>> uint64_t max_queues;
>> @@ -403,6 +404,40 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
>> struct vhost_inflight *inflight);
>> bool vhost_dev_has_iommu(struct vhost_dev *dev);
>> +static inline bool vhost_dev_has_feature(struct vhost_dev *dev,
>> + uint64_t feature)
>> +{
>> + return virtio_has_feature(dev->_features, feature);
>> +}
>> +
>> +static inline bool vhost_dev_has_feature_ex(struct vhost_dev *dev,
>> + uint64_t feature)
>> +{
>> + return virtio_has_feature_ex(dev->_features_ex, feature);
>> +}
>> +
>> +static inline uint64_t vhost_dev_features(struct vhost_dev *dev)
>> +{
>> + return dev->_features;
>> +}
>> +
>> +static inline const uint64_t *vhost_dev_features_ex(struct vhost_dev *dev)
>> +{
>> + return dev->_features_ex;
>> +}
>> +
>> +static inline void vhost_dev_clear_feature(struct vhost_dev *dev,
>> + uint64_t feature)
>> +{
>> + virtio_clear_feature(&dev->_features, feature);
>> +}
>> +
>> +static inline void vhost_dev_clear_feature_ex(struct vhost_dev *dev,
>> + uint64_t feature)
>> +{
>> + virtio_clear_feature_ex(dev->_features_ex, feature);
>> +}
>> +
>> #ifdef CONFIG_VHOST
>> int vhost_reset_device(struct vhost_dev *hdev);
>> #else
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 74d26a9497..0af0d3bdd3 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -256,15 +256,14 @@ static bool vhost_vdpa_get_vnet_hash_supported_types(NetClientState *nc,
>> {
>> assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>> - uint64_t features = s->vhost_vdpa.dev->features;
>> int fd = s->vhost_vdpa.shared->device_fd;
>> struct {
>> struct vhost_vdpa_config hdr;
>> uint32_t supported_hash_types;
>> } config;
>> - if (!virtio_has_feature(features, VIRTIO_NET_F_HASH_REPORT) &&
>> - !virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
>> + if (!vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_HASH_REPORT) &&
>> + !vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_RSS)) {
>> return false;
>> }
>> @@ -585,7 +584,7 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
>> * If we early return in these cases SVQ will not be enabled. The migration
>> * will be blocked as long as vhost-vdpa backends will not offer _F_LOG.
>> */
>> - if (!vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
>> + if (!vhost_vdpa_net_valid_svq_features(vhost_dev_features(v->dev), NULL)) {
>> return 0;
>> }
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 06/23] vhost: make vhost_dev.features private
2025-10-17 17:02 ` Vladimir Sementsov-Ogievskiy
@ 2025-10-31 17:31 ` Daniil Tatianin
0 siblings, 0 replies; 36+ messages in thread
From: Daniil Tatianin @ 2025-10-31 17:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, mst
Cc: sgarzare, raphael, qemu-devel, yc-core, Marc-André Lureau,
Jason Wang, Alex Bennée
On 10/17/25 8:02 PM, Vladimir Sementsov-Ogievskiy wrote:
> On 17.10.25 13:32, Daniil Tatianin wrote:
>> On 10/15/25 5:57 PM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> It's hard to control where and how do we use this field. Let's
>>> cover all usages by getters/setters, and keep direct access to the
>>> field only in vhost.c. It will help to control migration of this
>>> field in further commits.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>> hw/display/vhost-user-gpu.c | 7 +++----
>>> hw/net/vhost_net.c | 18 ++++++++---------
>>> hw/virtio/vdpa-dev.c | 2 +-
>>> hw/virtio/vhost-user-base.c | 8 ++++++--
>>> hw/virtio/vhost-user.c | 4 ++--
>>> hw/virtio/vhost.c | 6 +++---
>>> hw/virtio/virtio-qmp.c | 2 +-
>>> include/hw/virtio/vhost.h | 39
>>> +++++++++++++++++++++++++++++++++++--
>>> net/vhost-vdpa.c | 7 +++----
>>> 9 files changed, 65 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
>>> index 79ea64b12c..146620e0a3 100644
>>> --- a/hw/display/vhost-user-gpu.c
>>> +++ b/hw/display/vhost-user-gpu.c
>>> @@ -631,17 +631,16 @@ vhost_user_gpu_device_realize(DeviceState
>>> *qdev, Error **errp)
>>> /* existing backend may send DMABUF, so let's add that
>>> requirement */
>>> g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_DMABUF_ENABLED;
>>> - if (virtio_has_feature(g->vhost->dev.features,
>>> VIRTIO_GPU_F_VIRGL)) {
>>> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_VIRGL)) {
>>> g->parent_obj.conf.flags |= 1 <<
>>> VIRTIO_GPU_FLAG_VIRGL_ENABLED;
>>> }
>>> - if (virtio_has_feature(g->vhost->dev.features,
>>> VIRTIO_GPU_F_EDID)) {
>>> + if (vhost_dev_has_feature(&g->vhost->dev, VIRTIO_GPU_F_EDID)) {
>>> g->parent_obj.conf.flags |= 1 <<
>>> VIRTIO_GPU_FLAG_EDID_ENABLED;
>>> } else {
>>> error_report("EDID requested but the backend doesn't
>>> support it.");
>>> g->parent_obj.conf.flags &= ~(1 <<
>>> VIRTIO_GPU_FLAG_EDID_ENABLED);
>>> }
>>> - if (virtio_has_feature(g->vhost->dev.features,
>>> - VIRTIO_GPU_F_RESOURCE_UUID)) {
>>> + if (vhost_dev_has_feature(&g->vhost->dev,
>>> VIRTIO_GPU_F_RESOURCE_UUID)) {
>>> g->parent_obj.conf.flags |= 1 <<
>>> VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED;
>>> }
>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>> index ca19983126..323d117735 100644
>>> --- a/hw/net/vhost_net.c
>>> +++ b/hw/net/vhost_net.c
>>> @@ -54,8 +54,8 @@ void vhost_net_ack_features_ex(struct vhost_net
>>> *net, const uint64_t *features)
>>> {
>>> virtio_features_clear(net->dev.acked_features_ex);
>>> if (net->backend == -1) {
>>> - net->dev.acked_features =
>>> - net->dev.features & (1ULL <<
>>> VHOST_USER_F_PROTOCOL_FEATURES);
>>> + net->dev.acked_features = (vhost_dev_features(&net->dev) &
>>> + (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
>>> } else if (!qemu_has_vnet_hdr(net->nc)) {
>>> net->dev.acked_features = 1ULL << VHOST_NET_F_VIRTIO_NET_HDR;
>>> }
>>> @@ -282,15 +282,15 @@ struct vhost_net
>>> *vhost_net_init(VhostNetOptions *options)
>>> if (backend_kernel) {
>>> if (!qemu_has_vnet_hdr_len(options->net_backend,
>>> sizeof(struct
>>> virtio_net_hdr_mrg_rxbuf))) {
>>> - net->dev.features &= ~(1ULL << VIRTIO_NET_F_MRG_RXBUF);
>>> + vhost_dev_clear_feature(&net->dev,
>>> VIRTIO_NET_F_MRG_RXBUF);
>>> }
>>> if (!qemu_has_vnet_hdr(options->net_backend) &&
>>> - (~net->dev.features & (1ULL <<
>>> VHOST_NET_F_VIRTIO_NET_HDR))) {
>>> - fprintf(stderr, "vhost lacks feature mask 0x%llx for
>>> backend\n",
>>> - ~net->dev.features & (1ULL <<
>>> VHOST_NET_F_VIRTIO_NET_HDR));
>>> - goto fail;
>>> - }
>>> + !vhost_dev_has_feature(&net->dev,
>>> VHOST_NET_F_VIRTIO_NET_HDR)) {
>>> + fprintf(stderr, "vhost lacks VHOST_NET_F_VIRTIO_NET_HDR "
>>> + "feature for backend\n");
>>> + goto fail;
>>> + }
>>> }
>>> /* Set sane init value. Override when guest acks. */
>>> @@ -298,7 +298,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
>>> *options)
>>> virtio_features_from_u64(features,
>>> options->get_acked_features(net->nc));
>>> if (virtio_features_andnot(missing_features, features,
>>> - net->dev.features_ex)) {
>>> + vhost_dev_features_ex(&net->dev))) {
>>> fprintf(stderr, "vhost lacks feature mask 0x"
>>> VIRTIO_FEATURES_FMT
>>> " for backend\n",
>>> VIRTIO_FEATURES_PR(missing_features));
>>> goto fail;
>>> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>>> index efd9f68420..e1a2ff433d 100644
>>> --- a/hw/virtio/vdpa-dev.c
>>> +++ b/hw/virtio/vdpa-dev.c
>>> @@ -224,7 +224,7 @@ static uint64_t
>>> vhost_vdpa_device_get_features(VirtIODevice *vdev,
>>> Error **errp)
>>> {
>>> VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
>>> - uint64_t backend_features = s->dev.features;
>>> + uint64_t backend_features = vhost_dev_features(&s->dev);
>>> if (!virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM)) {
>>> virtio_clear_feature(&backend_features,
>>> VIRTIO_F_IOMMU_PLATFORM);
>>> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
>>> index ff67a020b4..cf311c3bfc 100644
>>> --- a/hw/virtio/vhost-user-base.c
>>> +++ b/hw/virtio/vhost-user-base.c
>>> @@ -118,9 +118,13 @@ static uint64_t vub_get_features(VirtIODevice
>>> *vdev,
>>> uint64_t requested_features,
>>> Error **errp)
>>> {
>>> VHostUserBase *vub = VHOST_USER_BASE(vdev);
>>> + uint64_t backend_features = vhost_dev_features(&vub->vhost_dev);
>>> +
>>> /* This should be set when the vhost connection initialises */
>>> - g_assert(vub->vhost_dev.features);
>>> - return vub->vhost_dev.features & ~(1ULL <<
>>> VHOST_USER_F_PROTOCOL_FEATURES);
>>> + g_assert(backend_features);
>>> + virtio_clear_feature(&backend_features,
>>> VHOST_USER_F_PROTOCOL_FEATURES);
>>> +
>>> + return backend_features;
>>> }
>>> /*
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index 3fd11a3b57..9f26515fd4 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -1252,7 +1252,7 @@ static int vhost_user_set_vring_enable(struct
>>> vhost_dev *dev, int enable)
>>> {
>>> int i;
>>> - if (!virtio_has_feature(dev->features,
>>> VHOST_USER_F_PROTOCOL_FEATURES)) {
>>> + if (!vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
>>> /*
>>> * For vhost-user devices, if
>>> VHOST_USER_F_PROTOCOL_FEATURES has not
>>> * been negotiated, the rings start directly in the
>>> enabled state,
>>> @@ -1469,7 +1469,7 @@ static int vhost_user_set_features(struct
>>> vhost_dev *dev,
>>> * Don't lose VHOST_USER_F_PROTOCOL_FEATURES, which is vhost-user
>>> * specific.
>>> */
>>> - if (virtio_has_feature(dev->features,
>>> VHOST_USER_F_PROTOCOL_FEATURES)) {
>>> + if (vhost_dev_has_feature(dev, VHOST_USER_F_PROTOCOL_FEATURES)) {
>>> features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>>> }
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 414a48a218..94efa409aa 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -1603,7 +1603,7 @@ int vhost_dev_init(struct vhost_dev *hdev,
>>> void *opaque,
>>> }
>>> }
>>> - virtio_features_copy(hdev->features_ex, features);
>>> + virtio_features_copy(hdev->_features_ex, features);
>>
>>
>> This should probably use the vhost_dev_features_ex getter
>
> Hmm, here we write into hdev->_features_ex.
Right, I missed that it returns a const pointer.
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>
>>
>> Otherwise, LGTM:
>> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>>
>>> hdev->memory_listener = (MemoryListener) {
>>> .name = "vhost",
>>> @@ -1626,7 +1626,7 @@ int vhost_dev_init(struct vhost_dev *hdev,
>>> void *opaque,
>>> };
>>> if (hdev->migration_blocker == NULL) {
>>> - if (!virtio_has_feature_ex(hdev->features_ex,
>>> VHOST_F_LOG_ALL)) {
>>> + if (!vhost_dev_has_feature_ex(hdev, VHOST_F_LOG_ALL)) {
>>> error_setg(&hdev->migration_blocker,
>>> "Migration disabled: vhost lacks
>>> VHOST_F_LOG_ALL feature.");
>>> } else if (vhost_dev_log_is_shared(hdev) &&
>>> !qemu_memfd_alloc_check()) {
>>> @@ -1900,7 +1900,7 @@ void vhost_get_features_ex(struct vhost_dev
>>> *hdev,
>>> const int *bit = feature_bits;
>>> while (*bit != VHOST_INVALID_FEATURE_BIT) {
>>> - if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
>>> + if (!vhost_dev_has_feature_ex(hdev, *bit)) {
>>> virtio_clear_feature_ex(features, *bit);
>>> }
>>> bit++;
>>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>>> index 82acb6d232..33d32720e1 100644
>>> --- a/hw/virtio/virtio-qmp.c
>>> +++ b/hw/virtio/virtio-qmp.c
>>> @@ -817,7 +817,7 @@ VirtioStatus *qmp_x_query_virtio_status(const
>>> char *path, Error **errp)
>>> status->vhost_dev->nvqs = hdev->nvqs;
>>> status->vhost_dev->vq_index = hdev->vq_index;
>>> status->vhost_dev->features =
>>> - qmp_decode_features(vdev->device_id, hdev->features_ex);
>>> + qmp_decode_features(vdev->device_id,
>>> vhost_dev_features_ex(hdev));
>>> status->vhost_dev->acked_features =
>>> qmp_decode_features(vdev->device_id,
>>> hdev->acked_features_ex);
>>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>>> index e308bc4556..1ba1af1d86 100644
>>> --- a/include/hw/virtio/vhost.h
>>> +++ b/include/hw/virtio/vhost.h
>>> @@ -98,10 +98,11 @@ struct vhost_dev {
>>> * offered by a backend which may be a subset of the total
>>> * features eventually offered to the guest.
>>> *
>>> - * @features: available features provided by the backend
>>> + * @_features: available features provided by the backend,
>>> private,
>>> + * direct access only in vhost.h/vhost.c
>>> * @acked_features: final negotiated features with front-end
>>> driver
>>> */
>>> - VIRTIO_DECLARE_FEATURES(features);
>>> + VIRTIO_DECLARE_FEATURES(_features);
>>> VIRTIO_DECLARE_FEATURES(acked_features);
>>> uint64_t max_queues;
>>> @@ -403,6 +404,40 @@ int vhost_dev_get_inflight(struct vhost_dev
>>> *dev, uint16_t queue_size,
>>> struct vhost_inflight *inflight);
>>> bool vhost_dev_has_iommu(struct vhost_dev *dev);
>>> +static inline bool vhost_dev_has_feature(struct vhost_dev *dev,
>>> + uint64_t feature)
>>> +{
>>> + return virtio_has_feature(dev->_features, feature);
>>> +}
>>> +
>>> +static inline bool vhost_dev_has_feature_ex(struct vhost_dev *dev,
>>> + uint64_t feature)
>>> +{
>>> + return virtio_has_feature_ex(dev->_features_ex, feature);
>>> +}
>>> +
>>> +static inline uint64_t vhost_dev_features(struct vhost_dev *dev)
>>> +{
>>> + return dev->_features;
>>> +}
>>> +
>>> +static inline const uint64_t *vhost_dev_features_ex(struct
>>> vhost_dev *dev)
>>> +{
>>> + return dev->_features_ex;
>>> +}
>>> +
>>> +static inline void vhost_dev_clear_feature(struct vhost_dev *dev,
>>> + uint64_t feature)
>>> +{
>>> + virtio_clear_feature(&dev->_features, feature);
>>> +}
>>> +
>>> +static inline void vhost_dev_clear_feature_ex(struct vhost_dev *dev,
>>> + uint64_t feature)
>>> +{
>>> + virtio_clear_feature_ex(dev->_features_ex, feature);
>>> +}
>>> +
>>> #ifdef CONFIG_VHOST
>>> int vhost_reset_device(struct vhost_dev *hdev);
>>> #else
>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>> index 74d26a9497..0af0d3bdd3 100644
>>> --- a/net/vhost-vdpa.c
>>> +++ b/net/vhost-vdpa.c
>>> @@ -256,15 +256,14 @@ static bool
>>> vhost_vdpa_get_vnet_hash_supported_types(NetClientState *nc,
>>> {
>>> assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>>> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>> - uint64_t features = s->vhost_vdpa.dev->features;
>>> int fd = s->vhost_vdpa.shared->device_fd;
>>> struct {
>>> struct vhost_vdpa_config hdr;
>>> uint32_t supported_hash_types;
>>> } config;
>>> - if (!virtio_has_feature(features, VIRTIO_NET_F_HASH_REPORT) &&
>>> - !virtio_has_feature(features, VIRTIO_NET_F_RSS)) {
>>> + if (!vhost_dev_has_feature(s->vhost_vdpa.dev,
>>> VIRTIO_NET_F_HASH_REPORT) &&
>>> + !vhost_dev_has_feature(s->vhost_vdpa.dev, VIRTIO_NET_F_RSS)) {
>>> return false;
>>> }
>>> @@ -585,7 +584,7 @@ static int
>>> vhost_vdpa_net_cvq_start(NetClientState *nc)
>>> * If we early return in these cases SVQ will not be enabled.
>>> The migration
>>> * will be blocked as long as vhost-vdpa backends will not
>>> offer _F_LOG.
>>> */
>>> - if (!vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
>>> + if
>>> (!vhost_vdpa_net_valid_svq_features(vhost_dev_features(v->dev),
>>> NULL)) {
>>> return 0;
>>> }
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 07/23] virtio: move common part of _set_guest_notifier to generic code
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 06/23] vhost: make vhost_dev.features private Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-11-07 13:01 ` Michael S. Tsirkin
2025-10-15 14:57 ` [PATCH v3 08/23] virtio: drop *_set_guest_notifier_fd_handler() helpers Vladimir Sementsov-Ogievskiy
` (16 subsequent siblings)
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst; +Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin
virtio-pci and virtio-mmio handle config notifier equally but
with different code (mmio adds a separate function, when pci
use common function). Let's chose the more compact way (pci)
and reuse it for mmio.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/virtio-mmio.c | 41 +++++------------------------
hw/virtio/virtio-pci.c | 34 +++---------------------
hw/virtio/virtio.c | 48 +++++++++++++++++++++++++++++++---
include/hw/virtio/virtio-pci.h | 3 ---
include/hw/virtio/virtio.h | 16 +++++++++---
5 files changed, 67 insertions(+), 75 deletions(-)
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index fb58c36452..1f1d96129b 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -659,18 +659,11 @@ static int virtio_mmio_set_guest_notifier(DeviceState *d, int n, bool assign,
VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
- VirtQueue *vq = virtio_get_queue(vdev, n);
- EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
+ int r;
- if (assign) {
- int r = event_notifier_init(notifier, 0);
- if (r < 0) {
- return r;
- }
- virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
- } else {
- virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
- event_notifier_cleanup(notifier);
+ r = virtio_queue_set_guest_notifier(vdev, n, assign, with_irqfd);
+ if (r < 0) {
+ return r;
}
if (vdc->guest_notifier_mask && vdev->use_guest_notifier_mask) {
@@ -679,30 +672,7 @@ static int virtio_mmio_set_guest_notifier(DeviceState *d, int n, bool assign,
return 0;
}
-static int virtio_mmio_set_config_guest_notifier(DeviceState *d, bool assign,
- bool with_irqfd)
-{
- VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
- VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
- VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
- EventNotifier *notifier = virtio_config_get_guest_notifier(vdev);
- int r = 0;
- if (assign) {
- r = event_notifier_init(notifier, 0);
- if (r < 0) {
- return r;
- }
- virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
- } else {
- virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
- event_notifier_cleanup(notifier);
- }
- if (vdc->guest_notifier_mask && vdev->use_guest_notifier_mask) {
- vdc->guest_notifier_mask(vdev, VIRTIO_CONFIG_IRQ_IDX, !assign);
- }
- return r;
-}
static int virtio_mmio_set_guest_notifiers(DeviceState *d, int nvqs,
bool assign)
{
@@ -724,7 +694,8 @@ static int virtio_mmio_set_guest_notifiers(DeviceState *d, int nvqs,
goto assign_error;
}
}
- r = virtio_mmio_set_config_guest_notifier(d, assign, with_irqfd);
+ r = virtio_mmio_set_guest_notifier(d, VIRTIO_CONFIG_IRQ_IDX, assign,
+ with_irqfd);
if (r < 0) {
goto assign_error;
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 937e22f08a..6c4576a17f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1197,43 +1197,17 @@ static void virtio_pci_vector_poll(PCIDevice *dev,
}
}
-void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev, VirtQueue *vq,
- int n, bool assign,
- bool with_irqfd)
-{
- if (n == VIRTIO_CONFIG_IRQ_IDX) {
- virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
- } else {
- virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
- }
-}
-
static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign,
bool with_irqfd)
{
VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
- VirtQueue *vq = NULL;
- EventNotifier *notifier = NULL;
+ int r;
- if (n == VIRTIO_CONFIG_IRQ_IDX) {
- notifier = virtio_config_get_guest_notifier(vdev);
- } else {
- vq = virtio_get_queue(vdev, n);
- notifier = virtio_queue_get_guest_notifier(vq);
- }
-
- if (assign) {
- int r = event_notifier_init(notifier, 0);
- if (r < 0) {
- return r;
- }
- virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, true, with_irqfd);
- } else {
- virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, false,
- with_irqfd);
- event_notifier_cleanup(notifier);
+ r = virtio_queue_set_guest_notifier(vdev, n, assign, with_irqfd);
+ if (r < 0) {
+ return r;
}
if (!msix_enabled(&proxy->pci_dev) &&
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 153ee0a0cf..704bc7943f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3803,8 +3803,10 @@ static void virtio_config_guest_notifier_read(EventNotifier *n)
virtio_notify_config(vdev);
}
}
-void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
- bool with_irqfd)
+
+static void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq,
+ bool assign,
+ bool with_irqfd)
{
if (assign && !with_irqfd) {
event_notifier_set_handler(&vq->guest_notifier,
@@ -3819,7 +3821,7 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
}
}
-void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
+static void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
bool assign, bool with_irqfd)
{
EventNotifier *n;
@@ -3836,6 +3838,46 @@ void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
}
}
+static void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev,
+ VirtQueue *vq,
+ int n, bool assign,
+ bool with_irqfd)
+{
+ if (n == VIRTIO_CONFIG_IRQ_IDX) {
+ virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
+ } else {
+ virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
+ }
+}
+
+int virtio_queue_set_guest_notifier(VirtIODevice *vdev, int n, bool assign,
+ bool with_irqfd)
+{
+ VirtQueue *vq = NULL;
+ EventNotifier *notifier = NULL;
+
+ if (n == VIRTIO_CONFIG_IRQ_IDX) {
+ notifier = virtio_config_get_guest_notifier(vdev);
+ } else {
+ vq = virtio_get_queue(vdev, n);
+ notifier = virtio_queue_get_guest_notifier(vq);
+ }
+
+ if (assign) {
+ int r = event_notifier_init(notifier, 0);
+ if (r < 0) {
+ return r;
+ }
+ virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, true, with_irqfd);
+ } else {
+ virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, false,
+ with_irqfd);
+ event_notifier_cleanup(notifier);
+ }
+
+ return 0;
+}
+
EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
{
return &vq->guest_notifier;
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index 639752977e..2a5b65f374 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -263,9 +263,6 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
* @fixed_queues.
*/
unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues);
-void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev, VirtQueue *vq,
- int n, bool assign,
- bool with_irqfd);
int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, uint8_t bar, uint64_t offset,
uint64_t length, uint8_t id);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d97529c3f1..7db8046766 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -420,8 +420,6 @@ void virtio_queue_update_used_idx(VirtIODevice *vdev, int n);
VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n);
uint16_t virtio_get_queue_index(VirtQueue *vq);
EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq);
-void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
- bool with_irqfd);
int virtio_device_start_ioeventfd(VirtIODevice *vdev);
int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
void virtio_device_release_ioeventfd(VirtIODevice *vdev);
@@ -435,8 +433,18 @@ void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx);
VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
EventNotifier *virtio_config_get_guest_notifier(VirtIODevice *vdev);
-void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
- bool assign, bool with_irqfd);
+
+/**
+ * virtio_queue_set_guest_notifier - set/unset queue or config guest
+ * notifier
+ *
+ * @vdev: the VirtIO device
+ * @n: queue number, or VIRTIO_CONFIG_IRQ_IDX to set config notifer
+ * @assign: true to set notifier, false to unset
+ * @with_irqfd: irqfd enabled
+ */
+int virtio_queue_set_guest_notifier(VirtIODevice *vdev, int n, bool assign,
+ bool with_irqfd);
static inline void virtio_add_feature(uint64_t *features, unsigned int fbit)
{
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 07/23] virtio: move common part of _set_guest_notifier to generic code
2025-10-15 14:57 ` [PATCH v3 07/23] virtio: move common part of _set_guest_notifier to generic code Vladimir Sementsov-Ogievskiy
@ 2025-11-07 13:01 ` Michael S. Tsirkin
0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2025-11-07 13:01 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin
On Wed, Oct 15, 2025 at 05:57:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> virtio-pci and virtio-mmio handle config notifier equally but
> with different code (mmio adds a separate function, when pci
> use common function). Let's chose the more compact way (pci)
> and reuse it for mmio.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Breaks virtio-ccw which also uses this.
> ---
> hw/virtio/virtio-mmio.c | 41 +++++------------------------
> hw/virtio/virtio-pci.c | 34 +++---------------------
> hw/virtio/virtio.c | 48 +++++++++++++++++++++++++++++++---
> include/hw/virtio/virtio-pci.h | 3 ---
> include/hw/virtio/virtio.h | 16 +++++++++---
> 5 files changed, 67 insertions(+), 75 deletions(-)
>
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index fb58c36452..1f1d96129b 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -659,18 +659,11 @@ static int virtio_mmio_set_guest_notifier(DeviceState *d, int n, bool assign,
> VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
> VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> - VirtQueue *vq = virtio_get_queue(vdev, n);
> - EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
> + int r;
>
> - if (assign) {
> - int r = event_notifier_init(notifier, 0);
> - if (r < 0) {
> - return r;
> - }
> - virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
> - } else {
> - virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
> - event_notifier_cleanup(notifier);
> + r = virtio_queue_set_guest_notifier(vdev, n, assign, with_irqfd);
> + if (r < 0) {
> + return r;
> }
>
> if (vdc->guest_notifier_mask && vdev->use_guest_notifier_mask) {
> @@ -679,30 +672,7 @@ static int virtio_mmio_set_guest_notifier(DeviceState *d, int n, bool assign,
>
> return 0;
> }
> -static int virtio_mmio_set_config_guest_notifier(DeviceState *d, bool assign,
> - bool with_irqfd)
> -{
> - VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
> - VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> - VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> - EventNotifier *notifier = virtio_config_get_guest_notifier(vdev);
> - int r = 0;
>
> - if (assign) {
> - r = event_notifier_init(notifier, 0);
> - if (r < 0) {
> - return r;
> - }
> - virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
> - } else {
> - virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
> - event_notifier_cleanup(notifier);
> - }
> - if (vdc->guest_notifier_mask && vdev->use_guest_notifier_mask) {
> - vdc->guest_notifier_mask(vdev, VIRTIO_CONFIG_IRQ_IDX, !assign);
> - }
> - return r;
> -}
> static int virtio_mmio_set_guest_notifiers(DeviceState *d, int nvqs,
> bool assign)
> {
> @@ -724,7 +694,8 @@ static int virtio_mmio_set_guest_notifiers(DeviceState *d, int nvqs,
> goto assign_error;
> }
> }
> - r = virtio_mmio_set_config_guest_notifier(d, assign, with_irqfd);
> + r = virtio_mmio_set_guest_notifier(d, VIRTIO_CONFIG_IRQ_IDX, assign,
> + with_irqfd);
> if (r < 0) {
> goto assign_error;
> }
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 937e22f08a..6c4576a17f 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1197,43 +1197,17 @@ static void virtio_pci_vector_poll(PCIDevice *dev,
> }
> }
>
> -void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev, VirtQueue *vq,
> - int n, bool assign,
> - bool with_irqfd)
> -{
> - if (n == VIRTIO_CONFIG_IRQ_IDX) {
> - virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
> - } else {
> - virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
> - }
> -}
> -
> static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign,
> bool with_irqfd)
> {
> VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> - VirtQueue *vq = NULL;
> - EventNotifier *notifier = NULL;
> + int r;
>
> - if (n == VIRTIO_CONFIG_IRQ_IDX) {
> - notifier = virtio_config_get_guest_notifier(vdev);
> - } else {
> - vq = virtio_get_queue(vdev, n);
> - notifier = virtio_queue_get_guest_notifier(vq);
> - }
> -
> - if (assign) {
> - int r = event_notifier_init(notifier, 0);
> - if (r < 0) {
> - return r;
> - }
> - virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, true, with_irqfd);
> - } else {
> - virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, false,
> - with_irqfd);
> - event_notifier_cleanup(notifier);
> + r = virtio_queue_set_guest_notifier(vdev, n, assign, with_irqfd);
> + if (r < 0) {
> + return r;
> }
>
> if (!msix_enabled(&proxy->pci_dev) &&
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 153ee0a0cf..704bc7943f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3803,8 +3803,10 @@ static void virtio_config_guest_notifier_read(EventNotifier *n)
> virtio_notify_config(vdev);
> }
> }
> -void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
> - bool with_irqfd)
> +
> +static void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq,
> + bool assign,
> + bool with_irqfd)
> {
> if (assign && !with_irqfd) {
> event_notifier_set_handler(&vq->guest_notifier,
> @@ -3819,7 +3821,7 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
> }
> }
>
> -void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
> +static void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
> bool assign, bool with_irqfd)
> {
> EventNotifier *n;
> @@ -3836,6 +3838,46 @@ void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
> }
> }
>
> +static void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev,
> + VirtQueue *vq,
> + int n, bool assign,
> + bool with_irqfd)
> +{
> + if (n == VIRTIO_CONFIG_IRQ_IDX) {
> + virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
> + } else {
> + virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
> + }
> +}
> +
> +int virtio_queue_set_guest_notifier(VirtIODevice *vdev, int n, bool assign,
> + bool with_irqfd)
> +{
> + VirtQueue *vq = NULL;
> + EventNotifier *notifier = NULL;
> +
> + if (n == VIRTIO_CONFIG_IRQ_IDX) {
> + notifier = virtio_config_get_guest_notifier(vdev);
> + } else {
> + vq = virtio_get_queue(vdev, n);
> + notifier = virtio_queue_get_guest_notifier(vq);
> + }
> +
> + if (assign) {
> + int r = event_notifier_init(notifier, 0);
> + if (r < 0) {
> + return r;
> + }
> + virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, true, with_irqfd);
> + } else {
> + virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, false,
> + with_irqfd);
> + event_notifier_cleanup(notifier);
> + }
> +
> + return 0;
> +}
> +
> EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
> {
> return &vq->guest_notifier;
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 639752977e..2a5b65f374 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -263,9 +263,6 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
> * @fixed_queues.
> */
> unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues);
> -void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev, VirtQueue *vq,
> - int n, bool assign,
> - bool with_irqfd);
>
> int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, uint8_t bar, uint64_t offset,
> uint64_t length, uint8_t id);
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index d97529c3f1..7db8046766 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -420,8 +420,6 @@ void virtio_queue_update_used_idx(VirtIODevice *vdev, int n);
> VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n);
> uint16_t virtio_get_queue_index(VirtQueue *vq);
> EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq);
> -void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
> - bool with_irqfd);
> int virtio_device_start_ioeventfd(VirtIODevice *vdev);
> int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
> void virtio_device_release_ioeventfd(VirtIODevice *vdev);
> @@ -435,8 +433,18 @@ void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx);
> VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
> VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
> EventNotifier *virtio_config_get_guest_notifier(VirtIODevice *vdev);
> -void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
> - bool assign, bool with_irqfd);
> +
> +/**
> + * virtio_queue_set_guest_notifier - set/unset queue or config guest
> + * notifier
> + *
> + * @vdev: the VirtIO device
> + * @n: queue number, or VIRTIO_CONFIG_IRQ_IDX to set config notifer
> + * @assign: true to set notifier, false to unset
> + * @with_irqfd: irqfd enabled
> + */
> +int virtio_queue_set_guest_notifier(VirtIODevice *vdev, int n, bool assign,
> + bool with_irqfd);
>
> static inline void virtio_add_feature(uint64_t *features, unsigned int fbit)
> {
> --
> 2.48.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 08/23] virtio: drop *_set_guest_notifier_fd_handler() helpers
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 07/23] virtio: move common part of _set_guest_notifier to generic code Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 09/23] vhost-user: keep QIOChannelSocket for backend channel Vladimir Sementsov-Ogievskiy
` (15 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst; +Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin
Now they don't make code more readable. Let's better put the whole
logic into virtio_queue_set_guest_notifier().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/virtio.c | 76 +++++++++++-----------------------------------
1 file changed, 17 insertions(+), 59 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 704bc7943f..4184aff75c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3804,74 +3804,32 @@ static void virtio_config_guest_notifier_read(EventNotifier *n)
}
}
-static void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq,
- bool assign,
- bool with_irqfd)
-{
- if (assign && !with_irqfd) {
- event_notifier_set_handler(&vq->guest_notifier,
- virtio_queue_guest_notifier_read);
- } else {
- event_notifier_set_handler(&vq->guest_notifier, NULL);
- }
- if (!assign) {
- /* Test and clear notifier before closing it,
- * in case poll callback didn't have time to run. */
- virtio_queue_guest_notifier_read(&vq->guest_notifier);
- }
-}
-
-static void virtio_config_set_guest_notifier_fd_handler(VirtIODevice *vdev,
- bool assign, bool with_irqfd)
-{
- EventNotifier *n;
- n = &vdev->config_notifier;
- if (assign && !with_irqfd) {
- event_notifier_set_handler(n, virtio_config_guest_notifier_read);
- } else {
- event_notifier_set_handler(n, NULL);
- }
- if (!assign) {
- /* Test and clear notifier before closing it,*/
- /* in case poll callback didn't have time to run. */
- virtio_config_guest_notifier_read(n);
- }
-}
-
-static void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev,
- VirtQueue *vq,
- int n, bool assign,
- bool with_irqfd)
-{
- if (n == VIRTIO_CONFIG_IRQ_IDX) {
- virtio_config_set_guest_notifier_fd_handler(vdev, assign, with_irqfd);
- } else {
- virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
- }
-}
-
int virtio_queue_set_guest_notifier(VirtIODevice *vdev, int n, bool assign,
bool with_irqfd)
{
- VirtQueue *vq = NULL;
- EventNotifier *notifier = NULL;
-
- if (n == VIRTIO_CONFIG_IRQ_IDX) {
- notifier = virtio_config_get_guest_notifier(vdev);
- } else {
- vq = virtio_get_queue(vdev, n);
- notifier = virtio_queue_get_guest_notifier(vq);
- }
+ bool is_config = n == VIRTIO_CONFIG_IRQ_IDX;
+ VirtQueue *vq = is_config ? NULL : virtio_get_queue(vdev, n);
+ EventNotifier *notifier = is_config ?
+ virtio_config_get_guest_notifier(vdev) :
+ virtio_queue_get_guest_notifier(vq);
+ EventNotifierHandler *read_fn = is_config ?
+ virtio_config_guest_notifier_read :
+ virtio_queue_guest_notifier_read;
if (assign) {
int r = event_notifier_init(notifier, 0);
if (r < 0) {
return r;
}
- virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, true, with_irqfd);
- } else {
- virtio_pci_set_guest_notifier_fd_handler(vdev, vq, n, false,
- with_irqfd);
+ }
+
+ event_notifier_set_handler(notifier,
+ (assign && !with_irqfd) ? read_fn : NULL);
+
+ if (!assign) {
+ /* Test and clear notifier before closing it,*/
+ /* in case poll callback didn't have time to run. */
+ read_fn(notifier);
event_notifier_cleanup(notifier);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 09/23] vhost-user: keep QIOChannelSocket for backend channel
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 08/23] virtio: drop *_set_guest_notifier_fd_handler() helpers Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 10/23] vhost: vhost_virtqueue_start(): fix failure path Vladimir Sementsov-Ogievskiy
` (14 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Keep QIOChannelSocket pointer instead of more generic
QIOChannel. No real difference for now, but it would
be simpler to migrate socket fd in further commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Acked-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost-user.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 9f26515fd4..23e7c12b16 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -244,7 +244,7 @@ struct vhost_user {
struct vhost_dev *dev;
/* Shared between vhost devs of the same virtio device */
VhostUserState *user;
- QIOChannel *backend_ioc;
+ QIOChannelSocket *backend_sioc;
GSource *backend_src;
NotifierWithReturn postcopy_notifier;
struct PostCopyFD postcopy_fd;
@@ -1796,8 +1796,8 @@ static void close_backend_channel(struct vhost_user *u)
g_source_destroy(u->backend_src);
g_source_unref(u->backend_src);
u->backend_src = NULL;
- object_unref(OBJECT(u->backend_ioc));
- u->backend_ioc = NULL;
+ object_unref(OBJECT(u->backend_sioc));
+ u->backend_sioc = NULL;
}
static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
@@ -1904,7 +1904,6 @@ static int vhost_setup_backend_channel(struct vhost_dev *dev)
bool reply_supported =
vhost_user_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK);
Error *local_err = NULL;
- QIOChannel *ioc;
if (!vhost_user_has_protocol_feature(
dev, VHOST_USER_PROTOCOL_F_BACKEND_REQ)) {
@@ -1917,15 +1916,15 @@ static int vhost_setup_backend_channel(struct vhost_dev *dev)
return -saved_errno;
}
- ioc = QIO_CHANNEL(qio_channel_socket_new_fd(sv[0], &local_err));
- if (!ioc) {
+ u->backend_sioc = qio_channel_socket_new_fd(sv[0], &local_err);
+ if (!u->backend_sioc) {
error_report_err(local_err);
return -ECONNREFUSED;
}
- u->backend_ioc = ioc;
- u->backend_src = qio_channel_add_watch_source(u->backend_ioc,
- G_IO_IN | G_IO_HUP,
- backend_read, dev, NULL, NULL);
+ u->backend_src = qio_channel_add_watch_source(QIO_CHANNEL(u->backend_sioc),
+ G_IO_IN | G_IO_HUP,
+ backend_read, dev,
+ NULL, NULL);
if (reply_supported) {
msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
@@ -2336,7 +2335,7 @@ static int vhost_user_backend_cleanup(struct vhost_dev *dev)
close(u->postcopy_fd.fd);
u->postcopy_fd.handler = NULL;
}
- if (u->backend_ioc) {
+ if (u->backend_sioc) {
close_backend_channel(u);
}
g_free(u->region_rb);
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 10/23] vhost: vhost_virtqueue_start(): fix failure path
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 09/23] vhost-user: keep QIOChannelSocket for backend channel Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-20 18:24 ` Raphael Norwitz
2025-10-15 14:57 ` [PATCH v3 11/23] vhost: make vhost_memory_unmap() null-safe Vladimir Sementsov-Ogievskiy
` (13 subsequent siblings)
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst; +Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin
We miss call to unmap in cases when vhost_memory_map() returns
lenght less than requested (still we consider such cases as an
error). Let's fix it in vhost_memory_map().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 94efa409aa..be2245cc7e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -453,11 +453,20 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
}
static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
- hwaddr *plen, bool is_write)
+ hwaddr len, bool is_write)
{
if (!vhost_dev_has_iommu(dev)) {
- return address_space_map(dev->vdev->dma_as, addr, plen, is_write,
- MEMTXATTRS_UNSPECIFIED);
+ hwaddr mapped_len = len;
+ void *res = address_space_map(dev->vdev->dma_as, addr, &mapped_len,
+ is_write, MEMTXATTRS_UNSPECIFIED);
+ if (!res) {
+ return NULL;
+ }
+ if (len != mapped_len) {
+ address_space_unmap(dev->vdev->dma_as, res, mapped_len, 0, 0);
+ return NULL;
+ }
+ return res;
} else {
return (void *)(uintptr_t)addr;
}
@@ -1261,7 +1270,7 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusState *vbus = VIRTIO_BUS(qbus);
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
- hwaddr s, l, a;
+ hwaddr l, a;
int r;
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
struct vhost_vring_file file = {
@@ -1301,24 +1310,24 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
}
}
- vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
+ vq->desc_size = l = virtio_queue_get_desc_size(vdev, idx);
vq->desc_phys = a;
- vq->desc = vhost_memory_map(dev, a, &l, false);
- if (!vq->desc || l != s) {
+ vq->desc = vhost_memory_map(dev, a, l, false);
+ if (!vq->desc) {
r = -ENOMEM;
goto fail_alloc_desc;
}
- vq->avail_size = s = l = virtio_queue_get_avail_size(vdev, idx);
+ vq->avail_size = l = virtio_queue_get_avail_size(vdev, idx);
vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
- vq->avail = vhost_memory_map(dev, a, &l, false);
- if (!vq->avail || l != s) {
+ vq->avail = vhost_memory_map(dev, a, l, false);
+ if (!vq->avail) {
r = -ENOMEM;
goto fail_alloc_avail;
}
- vq->used_size = s = l = virtio_queue_get_used_size(vdev, idx);
+ vq->used_size = l = virtio_queue_get_used_size(vdev, idx);
vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
- vq->used = vhost_memory_map(dev, a, &l, true);
- if (!vq->used || l != s) {
+ vq->used = vhost_memory_map(dev, a, l, true);
+ if (!vq->used) {
r = -ENOMEM;
goto fail_alloc_used;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 10/23] vhost: vhost_virtqueue_start(): fix failure path
2025-10-15 14:57 ` [PATCH v3 10/23] vhost: vhost_virtqueue_start(): fix failure path Vladimir Sementsov-Ogievskiy
@ 2025-10-20 18:24 ` Raphael Norwitz
0 siblings, 0 replies; 36+ messages in thread
From: Raphael Norwitz @ 2025-10-20 18:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: mst, sgarzare, raphael, qemu-devel, yc-core, d-tatianin
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
On Wed, Oct 15, 2025 at 11:07 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> We miss call to unmap in cases when vhost_memory_map() returns
> lenght less than requested (still we consider such cases as an
> error). Let's fix it in vhost_memory_map().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
> hw/virtio/vhost.c | 35 ++++++++++++++++++++++-------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 94efa409aa..be2245cc7e 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -453,11 +453,20 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> }
>
> static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
> - hwaddr *plen, bool is_write)
> + hwaddr len, bool is_write)
> {
> if (!vhost_dev_has_iommu(dev)) {
> - return address_space_map(dev->vdev->dma_as, addr, plen, is_write,
> - MEMTXATTRS_UNSPECIFIED);
> + hwaddr mapped_len = len;
> + void *res = address_space_map(dev->vdev->dma_as, addr, &mapped_len,
> + is_write, MEMTXATTRS_UNSPECIFIED);
> + if (!res) {
> + return NULL;
> + }
> + if (len != mapped_len) {
> + address_space_unmap(dev->vdev->dma_as, res, mapped_len, 0, 0);
> + return NULL;
> + }
> + return res;
> } else {
> return (void *)(uintptr_t)addr;
> }
> @@ -1261,7 +1270,7 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> VirtioBusState *vbus = VIRTIO_BUS(qbus);
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> - hwaddr s, l, a;
> + hwaddr l, a;
> int r;
> int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
> struct vhost_vring_file file = {
> @@ -1301,24 +1310,24 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
> }
> }
>
> - vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
> + vq->desc_size = l = virtio_queue_get_desc_size(vdev, idx);
> vq->desc_phys = a;
> - vq->desc = vhost_memory_map(dev, a, &l, false);
> - if (!vq->desc || l != s) {
> + vq->desc = vhost_memory_map(dev, a, l, false);
> + if (!vq->desc) {
> r = -ENOMEM;
> goto fail_alloc_desc;
> }
> - vq->avail_size = s = l = virtio_queue_get_avail_size(vdev, idx);
> + vq->avail_size = l = virtio_queue_get_avail_size(vdev, idx);
> vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
> - vq->avail = vhost_memory_map(dev, a, &l, false);
> - if (!vq->avail || l != s) {
> + vq->avail = vhost_memory_map(dev, a, l, false);
> + if (!vq->avail) {
> r = -ENOMEM;
> goto fail_alloc_avail;
> }
> - vq->used_size = s = l = virtio_queue_get_used_size(vdev, idx);
> + vq->used_size = l = virtio_queue_get_used_size(vdev, idx);
> vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
> - vq->used = vhost_memory_map(dev, a, &l, true);
> - if (!vq->used || l != s) {
> + vq->used = vhost_memory_map(dev, a, l, true);
> + if (!vq->used) {
> r = -ENOMEM;
> goto fail_alloc_used;
> }
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 11/23] vhost: make vhost_memory_unmap() null-safe
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 10/23] vhost: vhost_virtqueue_start(): fix failure path Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 12/23] vhost: simplify calls to vhost_memory_unmap() Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
This helps to simplify failure paths of vhost_virtqueue_start()
a lot. We also need to zero-out pointers on unmap, to not try
to unmap invalid pointers.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/virtio/vhost.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index be2245cc7e..db6ea42f9b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -472,14 +472,20 @@ static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
}
}
-static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
+static void vhost_memory_unmap(struct vhost_dev *dev, void **buffer,
hwaddr len, int is_write,
hwaddr access_len)
{
+ if (!*buffer) {
+ return;
+ }
+
if (!vhost_dev_has_iommu(dev)) {
- address_space_unmap(dev->vdev->dma_as, buffer, len, is_write,
+ address_space_unmap(dev->vdev->dma_as, *buffer, len, is_write,
access_len);
}
+
+ *buffer = NULL;
}
static int vhost_verify_ring_part_mapping(void *ring_hva,
@@ -1315,33 +1321,33 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
vq->desc = vhost_memory_map(dev, a, l, false);
if (!vq->desc) {
r = -ENOMEM;
- goto fail_alloc_desc;
+ goto fail;
}
vq->avail_size = l = virtio_queue_get_avail_size(vdev, idx);
vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
vq->avail = vhost_memory_map(dev, a, l, false);
if (!vq->avail) {
r = -ENOMEM;
- goto fail_alloc_avail;
+ goto fail;
}
vq->used_size = l = virtio_queue_get_used_size(vdev, idx);
vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
vq->used = vhost_memory_map(dev, a, l, true);
if (!vq->used) {
r = -ENOMEM;
- goto fail_alloc_used;
+ goto fail;
}
r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
if (r < 0) {
- goto fail_alloc;
+ goto fail;
}
file.fd = event_notifier_get_fd(virtio_queue_get_host_notifier(vvq));
r = dev->vhost_ops->vhost_set_vring_kick(dev, &file);
if (r) {
VHOST_OPS_DEBUG(r, "vhost_set_vring_kick failed");
- goto fail_kick;
+ goto fail;
}
/* Clear and discard previous events if any. */
@@ -1361,24 +1367,19 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
file.fd = -1;
r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
if (r) {
- goto fail_vector;
+ goto fail;
}
}
return 0;
-fail_vector:
-fail_kick:
-fail_alloc:
- vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
+fail:
+ vhost_memory_unmap(dev, &vq->used, virtio_queue_get_used_size(vdev, idx),
0, 0);
-fail_alloc_used:
- vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
+ vhost_memory_unmap(dev, &vq->avail, virtio_queue_get_avail_size(vdev, idx),
0, 0);
-fail_alloc_avail:
- vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
+ vhost_memory_unmap(dev, &vq->desc, virtio_queue_get_desc_size(vdev, idx),
0, 0);
-fail_alloc_desc:
return r;
}
@@ -1425,11 +1426,11 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
vhost_vq_index);
}
- vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
+ vhost_memory_unmap(dev, &vq->used, virtio_queue_get_used_size(vdev, idx),
1, virtio_queue_get_used_size(vdev, idx));
- vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
+ vhost_memory_unmap(dev, &vq->avail, virtio_queue_get_avail_size(vdev, idx),
0, virtio_queue_get_avail_size(vdev, idx));
- vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
+ vhost_memory_unmap(dev, &vq->desc, virtio_queue_get_desc_size(vdev, idx),
0, virtio_queue_get_desc_size(vdev, idx));
return r;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 12/23] vhost: simplify calls to vhost_memory_unmap()
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 11/23] vhost: make vhost_memory_unmap() null-safe Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 13/23] vhost: move vrings mapping to the top of vhost_virtqueue_start() Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
No reason to calculate memory size again, as we have corresponding
variable for each vring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/virtio/vhost.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index db6ea42f9b..c8ec6b4911 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1374,12 +1374,9 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
return 0;
fail:
- vhost_memory_unmap(dev, &vq->used, virtio_queue_get_used_size(vdev, idx),
- 0, 0);
- vhost_memory_unmap(dev, &vq->avail, virtio_queue_get_avail_size(vdev, idx),
- 0, 0);
- vhost_memory_unmap(dev, &vq->desc, virtio_queue_get_desc_size(vdev, idx),
- 0, 0);
+ vhost_memory_unmap(dev, &vq->used, vq->used_size, 0, 0);
+ vhost_memory_unmap(dev, &vq->avail, vq->avail_size, 0, 0);
+ vhost_memory_unmap(dev, &vq->desc, vq->desc_size, 0, 0);
return r;
}
@@ -1426,12 +1423,9 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
vhost_vq_index);
}
- vhost_memory_unmap(dev, &vq->used, virtio_queue_get_used_size(vdev, idx),
- 1, virtio_queue_get_used_size(vdev, idx));
- vhost_memory_unmap(dev, &vq->avail, virtio_queue_get_avail_size(vdev, idx),
- 0, virtio_queue_get_avail_size(vdev, idx));
- vhost_memory_unmap(dev, &vq->desc, virtio_queue_get_desc_size(vdev, idx),
- 0, virtio_queue_get_desc_size(vdev, idx));
+ vhost_memory_unmap(dev, &vq->used, vq->used_size, 1, vq->used_size);
+ vhost_memory_unmap(dev, &vq->avail, vq->avail_size, 0, vq->avail_size);
+ vhost_memory_unmap(dev, &vq->desc, vq->desc_size, 0, vq->desc_size);
return r;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 13/23] vhost: move vrings mapping to the top of vhost_virtqueue_start()
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 12/23] vhost: simplify calls to vhost_memory_unmap() Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 14/23] vhost: vhost_virtqueue_start(): drop extra local variables Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
This simplifies further refactoring and final introduction
of vhost backend live migration.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost.c | 47 +++++++++++++++++++++++------------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c8ec6b4911..db93b93fb1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1292,30 +1292,6 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
/* Queue might not be ready for start */
return 0;
}
-
- vq->num = state.num = virtio_queue_get_num(vdev, idx);
- r = dev->vhost_ops->vhost_set_vring_num(dev, &state);
- if (r) {
- VHOST_OPS_DEBUG(r, "vhost_set_vring_num failed");
- return r;
- }
-
- state.num = virtio_queue_get_last_avail_idx(vdev, idx);
- r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
- if (r) {
- VHOST_OPS_DEBUG(r, "vhost_set_vring_base failed");
- return r;
- }
-
- if (vhost_needs_vring_endian(vdev)) {
- r = vhost_virtqueue_set_vring_endian_legacy(dev,
- virtio_is_big_endian(vdev),
- vhost_vq_index);
- if (r) {
- return r;
- }
- }
-
vq->desc_size = l = virtio_queue_get_desc_size(vdev, idx);
vq->desc_phys = a;
vq->desc = vhost_memory_map(dev, a, l, false);
@@ -1338,6 +1314,29 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
goto fail;
}
+ vq->num = state.num = virtio_queue_get_num(vdev, idx);
+ r = dev->vhost_ops->vhost_set_vring_num(dev, &state);
+ if (r) {
+ VHOST_OPS_DEBUG(r, "vhost_set_vring_num failed");
+ goto fail;
+ }
+
+ state.num = virtio_queue_get_last_avail_idx(vdev, idx);
+ r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
+ if (r) {
+ VHOST_OPS_DEBUG(r, "vhost_set_vring_base failed");
+ goto fail;
+ }
+
+ if (vhost_needs_vring_endian(vdev)) {
+ r = vhost_virtqueue_set_vring_endian_legacy(dev,
+ virtio_is_big_endian(vdev),
+ vhost_vq_index);
+ if (r) {
+ goto fail;
+ }
+ }
+
r = vhost_virtqueue_set_addr(dev, vq, vhost_vq_index, dev->log_enabled);
if (r < 0) {
goto fail;
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 14/23] vhost: vhost_virtqueue_start(): drop extra local variables
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 13/23] vhost: move vrings mapping to the top of vhost_virtqueue_start() Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:57 ` [PATCH v3 15/23] vhost: final refactoring of vhost vrings map/unmap Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
One letter named variables doesn't really help to read the code,
and they simply duplicate structure fields.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index db93b93fb1..52a3236fff 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1276,7 +1276,6 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusState *vbus = VIRTIO_BUS(qbus);
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
- hwaddr l, a;
int r;
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
struct vhost_vring_file file = {
@@ -1287,28 +1286,27 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
};
struct VirtQueue *vvq = virtio_get_queue(vdev, idx);
- a = virtio_queue_get_desc_addr(vdev, idx);
- if (a == 0) {
+ vq->desc_phys = virtio_queue_get_desc_addr(vdev, idx);
+ if (vq->desc_phys == 0) {
/* Queue might not be ready for start */
return 0;
}
- vq->desc_size = l = virtio_queue_get_desc_size(vdev, idx);
- vq->desc_phys = a;
- vq->desc = vhost_memory_map(dev, a, l, false);
+ vq->desc_size = virtio_queue_get_desc_size(vdev, idx);
+ vq->desc = vhost_memory_map(dev, vq->desc_phys, vq->desc_size, false);
if (!vq->desc) {
r = -ENOMEM;
goto fail;
}
- vq->avail_size = l = virtio_queue_get_avail_size(vdev, idx);
- vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
- vq->avail = vhost_memory_map(dev, a, l, false);
+ vq->avail_size = virtio_queue_get_avail_size(vdev, idx);
+ vq->avail_phys = virtio_queue_get_avail_addr(vdev, idx);
+ vq->avail = vhost_memory_map(dev, vq->avail_phys, vq->avail_size, false);
if (!vq->avail) {
r = -ENOMEM;
goto fail;
}
- vq->used_size = l = virtio_queue_get_used_size(vdev, idx);
- vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
- vq->used = vhost_memory_map(dev, a, l, true);
+ vq->used_size = virtio_queue_get_used_size(vdev, idx);
+ vq->used_phys = virtio_queue_get_used_addr(vdev, idx);
+ vq->used = vhost_memory_map(dev, vq->used_phys, vq->used_size, true);
if (!vq->used) {
r = -ENOMEM;
goto fail;
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 15/23] vhost: final refactoring of vhost vrings map/unmap
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (13 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 14/23] vhost: vhost_virtqueue_start(): drop extra local variables Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:57 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:58 ` [PATCH v3 16/23] vhost: simplify vhost_dev_init() error-path Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:57 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Introduce helper functions vhost_vrings_map() and
vhost_vrings_unmap() and use them.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/virtio/vhost.c | 82 ++++++++++++++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 30 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 52a3236fff..8c5e434b89 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -488,6 +488,53 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void **buffer,
*buffer = NULL;
}
+static void vhost_vrings_unmap(struct vhost_dev *dev,
+ struct vhost_virtqueue *vq, bool touched)
+{
+ vhost_memory_unmap(dev, &vq->used, vq->used_size, touched,
+ touched ? vq->used_size : 0);
+ vhost_memory_unmap(dev, &vq->avail, vq->avail_size, 0,
+ touched ? vq->avail_size : 0);
+ vhost_memory_unmap(dev, &vq->desc, vq->desc_size, 0,
+ touched ? vq->desc_size : 0);
+}
+
+static int vhost_vrings_map(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
+{
+ vq->desc_phys = virtio_queue_get_desc_addr(vdev, idx);
+ if (vq->desc_phys == 0) {
+ /* Queue might not be ready for start */
+ return 0;
+ }
+ vq->desc_size = virtio_queue_get_desc_size(vdev, idx);
+ vq->desc = vhost_memory_map(dev, vq->desc_phys, vq->desc_size, false);
+ if (!vq->desc) {
+ return -ENOMEM;
+ }
+
+ vq->avail_size = virtio_queue_get_avail_size(vdev, idx);
+ vq->avail_phys = virtio_queue_get_avail_addr(vdev, idx);
+ vq->avail = vhost_memory_map(dev, vq->avail_phys, vq->avail_size, false);
+ if (!vq->avail) {
+ goto fail;
+ }
+
+ vq->used_size = virtio_queue_get_used_size(vdev, idx);
+ vq->used_phys = virtio_queue_get_used_addr(vdev, idx);
+ vq->used = vhost_memory_map(dev, vq->used_phys, vq->used_size, true);
+ if (!vq->used) {
+ goto fail;
+ }
+
+ return 1;
+fail:
+ vhost_vrings_unmap(dev, vq, false);
+ return -ENOMEM;
+}
+
static int vhost_verify_ring_part_mapping(void *ring_hva,
uint64_t ring_gpa,
uint64_t ring_size,
@@ -1286,30 +1333,9 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
};
struct VirtQueue *vvq = virtio_get_queue(vdev, idx);
- vq->desc_phys = virtio_queue_get_desc_addr(vdev, idx);
- if (vq->desc_phys == 0) {
- /* Queue might not be ready for start */
- return 0;
- }
- vq->desc_size = virtio_queue_get_desc_size(vdev, idx);
- vq->desc = vhost_memory_map(dev, vq->desc_phys, vq->desc_size, false);
- if (!vq->desc) {
- r = -ENOMEM;
- goto fail;
- }
- vq->avail_size = virtio_queue_get_avail_size(vdev, idx);
- vq->avail_phys = virtio_queue_get_avail_addr(vdev, idx);
- vq->avail = vhost_memory_map(dev, vq->avail_phys, vq->avail_size, false);
- if (!vq->avail) {
- r = -ENOMEM;
- goto fail;
- }
- vq->used_size = virtio_queue_get_used_size(vdev, idx);
- vq->used_phys = virtio_queue_get_used_addr(vdev, idx);
- vq->used = vhost_memory_map(dev, vq->used_phys, vq->used_size, true);
- if (!vq->used) {
- r = -ENOMEM;
- goto fail;
+ r = vhost_vrings_map(dev, vdev, vq, idx);
+ if (r <= 0) {
+ return r;
}
vq->num = state.num = virtio_queue_get_num(vdev, idx);
@@ -1371,9 +1397,7 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
return 0;
fail:
- vhost_memory_unmap(dev, &vq->used, vq->used_size, 0, 0);
- vhost_memory_unmap(dev, &vq->avail, vq->avail_size, 0, 0);
- vhost_memory_unmap(dev, &vq->desc, vq->desc_size, 0, 0);
+ vhost_vrings_unmap(dev, vq, false);
return r;
}
@@ -1420,9 +1444,7 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
vhost_vq_index);
}
- vhost_memory_unmap(dev, &vq->used, vq->used_size, 1, vq->used_size);
- vhost_memory_unmap(dev, &vq->avail, vq->avail_size, 0, vq->avail_size);
- vhost_memory_unmap(dev, &vq->desc, vq->desc_size, 0, vq->desc_size);
+ vhost_vrings_unmap(dev, vq, true);
return r;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 16/23] vhost: simplify vhost_dev_init() error-path
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (14 preceding siblings ...)
2025-10-15 14:57 ` [PATCH v3 15/23] vhost: final refactoring of vhost vrings map/unmap Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:58 ` [PATCH v3 17/23] vhost: move busyloop timeout initialization to vhost_virtqueue_init() Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
No reason to rollback setting up busyloop timeout on failure.
We don't do such rollback for other things we setup in backend.
Also, look at vhost_net_init() in hw/net/vhost_net.c: we may fail
after successfully called vhost_dev_init(), and in this case we'll
just call vhost_dev_cleanup(), which doesn't rollback busyloop
timeout.
So, let's keep it simple.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Acked-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 8c5e434b89..2dba7d53c9 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1621,7 +1621,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
busyloop_timeout);
if (r < 0) {
error_setg_errno(errp, -r, "Failed to set busyloop timeout");
- goto fail_busyloop;
+ goto fail;
}
}
}
@@ -1661,7 +1661,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
if (hdev->migration_blocker != NULL) {
r = migrate_add_blocker_normal(&hdev->migration_blocker, errp);
if (r < 0) {
- goto fail_busyloop;
+ goto fail;
}
}
@@ -1693,17 +1693,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
" than current number of used (%d) and reserved (%d)"
" memory slots for memory devices.", limit, used, reserved);
r = -EINVAL;
- goto fail_busyloop;
+ goto fail;
}
return 0;
-fail_busyloop:
- if (busyloop_timeout) {
- while (--i >= 0) {
- vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
- }
- }
fail:
hdev->nvqs = n_initialized_vqs;
vhost_dev_cleanup(hdev);
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 17/23] vhost: move busyloop timeout initialization to vhost_virtqueue_init()
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (15 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 16/23] vhost: simplify vhost_dev_init() error-path Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:58 ` [PATCH v3 18/23] vhost: introduce check_memslots() helper Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Let's all per-virtqueue initializations be in one place.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2dba7d53c9..34846edf36 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1493,7 +1493,8 @@ static void vhost_virtqueue_error_notifier(EventNotifier *n)
}
static int vhost_virtqueue_init(struct vhost_dev *dev,
- struct vhost_virtqueue *vq, int n)
+ struct vhost_virtqueue *vq, int n,
+ bool busyloop_timeout)
{
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, n);
struct vhost_vring_file file = {
@@ -1530,6 +1531,14 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
vhost_virtqueue_error_notifier);
}
+ if (busyloop_timeout) {
+ r = vhost_virtqueue_set_busyloop_timeout(dev, n, busyloop_timeout);
+ if (r < 0) {
+ VHOST_OPS_DEBUG(r, "Failed to set busyloop timeout");
+ goto fail_err;
+ }
+ }
+
return 0;
fail_err:
@@ -1608,24 +1617,14 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
- r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
+ r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i,
+ busyloop_timeout);
if (r < 0) {
error_setg_errno(errp, -r, "Failed to initialize virtqueue %d", i);
goto fail;
}
}
- if (busyloop_timeout) {
- for (i = 0; i < hdev->nvqs; ++i) {
- r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
- busyloop_timeout);
- if (r < 0) {
- error_setg_errno(errp, -r, "Failed to set busyloop timeout");
- goto fail;
- }
- }
- }
-
virtio_features_copy(hdev->_features_ex, features);
hdev->memory_listener = (MemoryListener) {
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 18/23] vhost: introduce check_memslots() helper
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (16 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 17/23] vhost: move busyloop timeout initialization to vhost_virtqueue_init() Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-11-03 22:19 ` Michael S. Tsirkin
2025-10-15 14:58 ` [PATCH v3 19/23] vhost: vhost_dev_init(): simplify features initialization Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/vhost.c | 71 ++++++++++++++++++++++++++---------------------
1 file changed, 40 insertions(+), 31 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 34846edf36..56a812b06b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1572,12 +1572,50 @@ static int vhost_dev_get_features(struct vhost_dev *hdev,
return r;
}
+static bool check_memslots(struct vhost_dev *hdev, Error **errp)
+{
+ unsigned int used, reserved, limit;
+
+ limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
+ if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
+ memory_devices_memslot_auto_decision_active()) {
+ error_setg(errp, "some memory device (like virtio-mem)"
+ " decided how many memory slots to use based on the overall"
+ " number of memory slots; this vhost backend would further"
+ " restricts the overall number of memory slots");
+ error_append_hint(errp, "Try plugging this vhost backend before"
+ " plugging such memory devices.\n");
+ return false;
+ }
+
+ /*
+ * The listener we registered properly setup the number of required
+ * memslots in vhost_commit().
+ */
+ used = hdev->mem->nregions;
+
+ /*
+ * We assume that all reserved memslots actually require a real memslot
+ * in our vhost backend. This might not be true, for example, if the
+ * memslot would be ROM. If ever relevant, we can optimize for that --
+ * but we'll need additional information about the reservations.
+ */
+ reserved = memory_devices_get_reserved_memslots();
+ if (used + reserved > limit) {
+ error_setg(errp, "vhost backend memory slots limit (%d) is less"
+ " than current number of used (%d) and reserved (%d)"
+ " memory slots for memory devices.", limit, used, reserved);
+ return false;
+ }
+
+ return true;
+}
+
int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
VhostBackendType backend_type, uint32_t busyloop_timeout,
Error **errp)
{
uint64_t features[VIRTIO_FEATURES_NU64S];
- unsigned int used, reserved, limit;
int i, r, n_initialized_vqs = 0;
hdev->vdev = NULL;
@@ -1603,19 +1641,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
goto fail;
}
- limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
- if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
- memory_devices_memslot_auto_decision_active()) {
- error_setg(errp, "some memory device (like virtio-mem)"
- " decided how many memory slots to use based on the overall"
- " number of memory slots; this vhost backend would further"
- " restricts the overall number of memory slots");
- error_append_hint(errp, "Try plugging this vhost backend before"
- " plugging such memory devices.\n");
- r = -EINVAL;
- goto fail;
- }
-
for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i,
busyloop_timeout);
@@ -1674,23 +1699,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
memory_listener_register(&hdev->memory_listener, &address_space_memory);
QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
- /*
- * The listener we registered properly setup the number of required
- * memslots in vhost_commit().
- */
- used = hdev->mem->nregions;
-
- /*
- * We assume that all reserved memslots actually require a real memslot
- * in our vhost backend. This might not be true, for example, if the
- * memslot would be ROM. If ever relevant, we can optimize for that --
- * but we'll need additional information about the reservations.
- */
- reserved = memory_devices_get_reserved_memslots();
- if (used + reserved > limit) {
- error_setg(errp, "vhost backend memory slots limit (%d) is less"
- " than current number of used (%d) and reserved (%d)"
- " memory slots for memory devices.", limit, used, reserved);
+ if (!check_memslots(hdev, errp)) {
r = -EINVAL;
goto fail;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 18/23] vhost: introduce check_memslots() helper
2025-10-15 14:58 ` [PATCH v3 18/23] vhost: introduce check_memslots() helper Vladimir Sementsov-Ogievskiy
@ 2025-11-03 22:19 ` Michael S. Tsirkin
2025-11-04 10:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 36+ messages in thread
From: Michael S. Tsirkin @ 2025-11-03 22:19 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin,
Raphael Norwitz
On Wed, Oct 15, 2025 at 05:58:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
This needs a better commit log.
the subject makes it look like it's merely adding a helper
but it is not the case: e.g. errors are now handled
somewhat differently.
Pls document the thinking process explaining why it does not
matter. CB.
> ---
> hw/virtio/vhost.c | 71 ++++++++++++++++++++++++++---------------------
> 1 file changed, 40 insertions(+), 31 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 34846edf36..56a812b06b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1572,12 +1572,50 @@ static int vhost_dev_get_features(struct vhost_dev *hdev,
> return r;
> }
>
> +static bool check_memslots(struct vhost_dev *hdev, Error **errp)
> +{
> + unsigned int used, reserved, limit;
> +
> + limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> + if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
> + memory_devices_memslot_auto_decision_active()) {
> + error_setg(errp, "some memory device (like virtio-mem)"
> + " decided how many memory slots to use based on the overall"
> + " number of memory slots; this vhost backend would further"
> + " restricts the overall number of memory slots");
> + error_append_hint(errp, "Try plugging this vhost backend before"
> + " plugging such memory devices.\n");
> + return false;
> + }
> +
> + /*
> + * The listener we registered properly setup the number of required
This comment worked in the original code but not now.
The listener here is memory_listener_register which yes happens to
be called before check_memslots but it is far from obvious.
> + * memslots in vhost_commit().
> + */
> + used = hdev->mem->nregions;
> +
> + /*
> + * We assume that all reserved memslots actually require a real memslot
> + * in our vhost backend. This might not be true, for example, if the
> + * memslot would be ROM. If ever relevant, we can optimize for that --
> + * but we'll need additional information about the reservations.
> + */
> + reserved = memory_devices_get_reserved_memslots();
> + if (used + reserved > limit) {
> + error_setg(errp, "vhost backend memory slots limit (%d) is less"
> + " than current number of used (%d) and reserved (%d)"
> + " memory slots for memory devices.", limit, used, reserved);
> + return false;
> + }
> +
> + return true;
> +}
> +
> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> VhostBackendType backend_type, uint32_t busyloop_timeout,
> Error **errp)
> {
> uint64_t features[VIRTIO_FEATURES_NU64S];
> - unsigned int used, reserved, limit;
> int i, r, n_initialized_vqs = 0;
>
> hdev->vdev = NULL;
> @@ -1603,19 +1641,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> goto fail;
> }
>
> - limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> - if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
> - memory_devices_memslot_auto_decision_active()) {
> - error_setg(errp, "some memory device (like virtio-mem)"
> - " decided how many memory slots to use based on the overall"
> - " number of memory slots; this vhost backend would further"
> - " restricts the overall number of memory slots");
> - error_append_hint(errp, "Try plugging this vhost backend before"
> - " plugging such memory devices.\n");
> - r = -EINVAL;
> - goto fail;
> - }
> -
> for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
> r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i,
> busyloop_timeout);
> @@ -1674,23 +1699,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> memory_listener_register(&hdev->memory_listener, &address_space_memory);
So it looks like with your change
this will temporarily register the listener
restricting the number of slots, then check and unregister.
Is this ever a problem?
commit log needs better documentation why.
> QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
>
> - /*
> - * The listener we registered properly setup the number of required
> - * memslots in vhost_commit().
> - */
> - used = hdev->mem->nregions;
> -
> - /*
> - * We assume that all reserved memslots actually require a real memslot
> - * in our vhost backend. This might not be true, for example, if the
> - * memslot would be ROM. If ever relevant, we can optimize for that --
> - * but we'll need additional information about the reservations.
> - */
> - reserved = memory_devices_get_reserved_memslots();
> - if (used + reserved > limit) {
> - error_setg(errp, "vhost backend memory slots limit (%d) is less"
> - " than current number of used (%d) and reserved (%d)"
> - " memory slots for memory devices.", limit, used, reserved);
> + if (!check_memslots(hdev, errp)) {
> r = -EINVAL;
> goto fail;
> }
> --
> 2.48.1
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 18/23] vhost: introduce check_memslots() helper
2025-11-03 22:19 ` Michael S. Tsirkin
@ 2025-11-04 10:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-11-04 10:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin,
Raphael Norwitz
On 04.11.25 01:19, Michael S. Tsirkin wrote:
> On Wed, Oct 15, 2025 at 05:58:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
>> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>
>
> This needs a better commit log.
> the subject makes it look like it's merely adding a helper
> but it is not the case: e.g. errors are now handled
> somewhat differently.
>
> Pls document the thinking process explaining why it does not
> matter. CB.
>
Will do. Or, split into separate commits for helper and for logic change.
>
>> ---
>> hw/virtio/vhost.c | 71 ++++++++++++++++++++++++++---------------------
>> 1 file changed, 40 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 34846edf36..56a812b06b 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1572,12 +1572,50 @@ static int vhost_dev_get_features(struct vhost_dev *hdev,
>> return r;
>> }
>>
>> +static bool check_memslots(struct vhost_dev *hdev, Error **errp)
>> +{
>> + unsigned int used, reserved, limit;
>> +
>> + limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
>> + if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
>> + memory_devices_memslot_auto_decision_active()) {
>> + error_setg(errp, "some memory device (like virtio-mem)"
>> + " decided how many memory slots to use based on the overall"
>> + " number of memory slots; this vhost backend would further"
>> + " restricts the overall number of memory slots");
>> + error_append_hint(errp, "Try plugging this vhost backend before"
>> + " plugging such memory devices.\n");
>> + return false;
>> + }
>> +
>> + /*
>> + * The listener we registered properly setup the number of required
>
> This comment worked in the original code but not now.
>
> The listener here is memory_listener_register which yes happens to
> be called before check_memslots but it is far from obvious.
>
>
>
>> + * memslots in vhost_commit().
>> + */
>> + used = hdev->mem->nregions;
>
>
>
>> +
>> + /*
>> + * We assume that all reserved memslots actually require a real memslot
>> + * in our vhost backend. This might not be true, for example, if the
>> + * memslot would be ROM. If ever relevant, we can optimize for that --
>> + * but we'll need additional information about the reservations.
>> + */
>> + reserved = memory_devices_get_reserved_memslots();
>> + if (used + reserved > limit) {
>> + error_setg(errp, "vhost backend memory slots limit (%d) is less"
>> + " than current number of used (%d) and reserved (%d)"
>> + " memory slots for memory devices.", limit, used, reserved);
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> VhostBackendType backend_type, uint32_t busyloop_timeout,
>> Error **errp)
>> {
>> uint64_t features[VIRTIO_FEATURES_NU64S];
>> - unsigned int used, reserved, limit;
>> int i, r, n_initialized_vqs = 0;
>>
>> hdev->vdev = NULL;
>> @@ -1603,19 +1641,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> goto fail;
>> }
>>
>> - limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
>> - if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
>> - memory_devices_memslot_auto_decision_active()) {
>> - error_setg(errp, "some memory device (like virtio-mem)"
>> - " decided how many memory slots to use based on the overall"
>> - " number of memory slots; this vhost backend would further"
>> - " restricts the overall number of memory slots");
>> - error_append_hint(errp, "Try plugging this vhost backend before"
>> - " plugging such memory devices.\n");
>> - r = -EINVAL;
>> - goto fail;
>> - }
>> -
>> for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>> r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i,
>> busyloop_timeout);
>> @@ -1674,23 +1699,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> memory_listener_register(&hdev->memory_listener, &address_space_memory);
>
>
> So it looks like with your change
> this will temporarily register the listener
> restricting the number of slots, then check and unregister.
>
> Is this ever a problem?
>
> commit log needs better documentation why.
>
>
>
>> QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
>>
>> - /*
>> - * The listener we registered properly setup the number of required
>> - * memslots in vhost_commit().
>> - */
>> - used = hdev->mem->nregions;
>> -
>> - /*
>> - * We assume that all reserved memslots actually require a real memslot
>> - * in our vhost backend. This might not be true, for example, if the
>> - * memslot would be ROM. If ever relevant, we can optimize for that --
>> - * but we'll need additional information about the reservations.
>> - */
>> - reserved = memory_devices_get_reserved_memslots();
>> - if (used + reserved > limit) {
>> - error_setg(errp, "vhost backend memory slots limit (%d) is less"
>> - " than current number of used (%d) and reserved (%d)"
>> - " memory slots for memory devices.", limit, used, reserved);
>> + if (!check_memslots(hdev, errp)) {
>> r = -EINVAL;
>> goto fail;
>> }
>> --
>> 2.48.1
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 19/23] vhost: vhost_dev_init(): simplify features initialization
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (17 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 18/23] vhost: introduce check_memslots() helper Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:58 ` [PATCH v3 20/23] hw/virtio/virtio-bus: refactor virtio_bus_set_host_notifier() Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Drop extra variable and extra function parameter passing, initialize
dev._features directly.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/virtio/vhost.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 56a812b06b..fb5c4ba1ca 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1557,18 +1557,17 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
}
}
-static int vhost_dev_get_features(struct vhost_dev *hdev,
- uint64_t *features)
+static int vhost_dev_init_features(struct vhost_dev *hdev)
{
uint64_t features64;
int r;
if (hdev->vhost_ops->vhost_get_features_ex) {
- return hdev->vhost_ops->vhost_get_features_ex(hdev, features);
+ return hdev->vhost_ops->vhost_get_features_ex(hdev, hdev->_features_ex);
}
r = hdev->vhost_ops->vhost_get_features(hdev, &features64);
- virtio_features_from_u64(features, features64);
+ virtio_features_from_u64(hdev->_features_ex, features64);
return r;
}
@@ -1615,7 +1614,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
VhostBackendType backend_type, uint32_t busyloop_timeout,
Error **errp)
{
- uint64_t features[VIRTIO_FEATURES_NU64S];
int i, r, n_initialized_vqs = 0;
hdev->vdev = NULL;
@@ -1635,9 +1633,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
goto fail;
}
- r = vhost_dev_get_features(hdev, features);
+ r = vhost_dev_init_features(hdev);
if (r < 0) {
- error_setg_errno(errp, -r, "vhost_get_features failed");
+ error_setg_errno(errp, -r, "vhost_init_features failed");
goto fail;
}
@@ -1650,8 +1648,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
}
- virtio_features_copy(hdev->_features_ex, features);
-
hdev->memory_listener = (MemoryListener) {
.name = "vhost",
.begin = vhost_begin,
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 20/23] hw/virtio/virtio-bus: refactor virtio_bus_set_host_notifier()
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (18 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 19/23] vhost: vhost_dev_init(): simplify features initialization Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-10-15 14:58 ` [PATCH v3 21/23] vhost-user: make trace events more readable Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
23 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Philippe Mathieu-Daudé, Raphael Norwitz
The logic kept as is. Refactor to simplify further changes.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/virtio-bus.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index cef944e015..9b545acda3 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -298,20 +298,18 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
__func__, strerror(-r), r);
return r;
}
- r = k->ioeventfd_assign(proxy, notifier, n, true);
- if (r < 0) {
- error_report("%s: unable to assign ioeventfd: %d", __func__, r);
- virtio_bus_cleanup_host_notifier(bus, n);
- }
- } else {
- k->ioeventfd_assign(proxy, notifier, n, false);
}
- if (r == 0) {
- virtio_queue_set_host_notifier_enabled(vq, assign);
+ r = k->ioeventfd_assign(proxy, notifier, n, assign);
+ if (r < 0 && assign) {
+ error_report("%s: unable to assign ioeventfd: %d", __func__, r);
+ virtio_bus_cleanup_host_notifier(bus, n);
+ return r;
}
- return r;
+ virtio_queue_set_host_notifier_enabled(vq, assign);
+
+ return 0;
}
void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n)
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 21/23] vhost-user: make trace events more readable
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (19 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 20/23] hw/virtio/virtio-bus: refactor virtio_bus_set_host_notifier() Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-11-03 22:22 ` Michael S. Tsirkin
2025-10-15 14:58 ` [PATCH v3 22/23] vhost-user-blk: add some useful trace-points Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Philippe Mathieu-Daudé, Raphael Norwitz
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/trace-events | 4 +-
hw/virtio/vhost-user.c | 94 +++++++++++++++++++++++++++++++++++++++++-
2 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 658cc365e7..aa1ffa5e94 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -25,8 +25,8 @@ vhost_user_set_mem_table_withfd(int index, const char *name, uint64_t memory_siz
vhost_user_postcopy_waker(const char *rb, uint64_t rb_offset) "%s + 0x%"PRIx64
vhost_user_postcopy_waker_found(uint64_t client_addr) "0x%"PRIx64
vhost_user_postcopy_waker_nomatch(const char *rb, uint64_t rb_offset) "%s + 0x%"PRIx64
-vhost_user_read(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32""
-vhost_user_write(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32""
+vhost_user_read(uint32_t req, const char *req_name, uint32_t flags) "req:%d (%s) flags:0x%"PRIx32""
+vhost_user_write(uint32_t req, const char *req_name, uint32_t flags) "req:%d (%s) flags:0x%"PRIx32""
vhost_user_create_notifier(int idx, void *n) "idx:%d n:%p"
# vhost-vdpa.c
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 23e7c12b16..e45b74eddd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -119,6 +119,94 @@ typedef enum VhostUserBackendRequest {
VHOST_USER_BACKEND_MAX
} VhostUserBackendRequest;
+static const char *vhost_req_name(VhostUserRequest req)
+{
+ switch (req) {
+ case VHOST_USER_NONE:
+ return "NONE";
+ case VHOST_USER_GET_FEATURES:
+ return "GET_FEATURES";
+ case VHOST_USER_SET_FEATURES:
+ return "SET_FEATURES";
+ case VHOST_USER_SET_OWNER:
+ return "SET_OWNER";
+ case VHOST_USER_RESET_OWNER:
+ return "RESET_OWNER";
+ case VHOST_USER_SET_MEM_TABLE:
+ return "SET_MEM_TABLE";
+ case VHOST_USER_SET_LOG_BASE:
+ return "SET_LOG_BASE";
+ case VHOST_USER_SET_LOG_FD:
+ return "SET_LOG_FD";
+ case VHOST_USER_SET_VRING_NUM:
+ return "SET_VRING_NUM";
+ case VHOST_USER_SET_VRING_ADDR:
+ return "SET_VRING_ADDR";
+ case VHOST_USER_SET_VRING_BASE:
+ return "SET_VRING_BASE";
+ case VHOST_USER_GET_VRING_BASE:
+ return "GET_VRING_BASE";
+ case VHOST_USER_SET_VRING_KICK:
+ return "SET_VRING_KICK";
+ case VHOST_USER_SET_VRING_CALL:
+ return "SET_VRING_CALL";
+ case VHOST_USER_SET_VRING_ERR:
+ return "SET_VRING_ERR";
+ case VHOST_USER_GET_PROTOCOL_FEATURES:
+ return "GET_PROTOCOL_FEATURES";
+ case VHOST_USER_SET_PROTOCOL_FEATURES:
+ return "SET_PROTOCOL_FEATURES";
+ case VHOST_USER_GET_QUEUE_NUM:
+ return "GET_QUEUE_NUM";
+ case VHOST_USER_SET_VRING_ENABLE:
+ return "SET_VRING_ENABLE";
+ case VHOST_USER_SEND_RARP:
+ return "SEND_RARP";
+ case VHOST_USER_NET_SET_MTU:
+ return "NET_SET_MTU";
+ case VHOST_USER_SET_BACKEND_REQ_FD:
+ return "SET_BACKEND_REQ_FD";
+ case VHOST_USER_IOTLB_MSG:
+ return "IOTLB_MSG";
+ case VHOST_USER_SET_VRING_ENDIAN:
+ return "SET_VRING_ENDIAN";
+ case VHOST_USER_GET_CONFIG:
+ return "GET_CONFIG";
+ case VHOST_USER_SET_CONFIG:
+ return "SET_CONFIG";
+ case VHOST_USER_CREATE_CRYPTO_SESSION:
+ return "CREATE_CRYPTO_SESSION";
+ case VHOST_USER_CLOSE_CRYPTO_SESSION:
+ return "CLOSE_CRYPTO_SESSION";
+ case VHOST_USER_POSTCOPY_ADVISE:
+ return "POSTCOPY_ADVISE";
+ case VHOST_USER_POSTCOPY_LISTEN:
+ return "POSTCOPY_LISTEN";
+ case VHOST_USER_POSTCOPY_END:
+ return "POSTCOPY_END";
+ case VHOST_USER_GET_INFLIGHT_FD:
+ return "GET_INFLIGHT_FD";
+ case VHOST_USER_SET_INFLIGHT_FD:
+ return "SET_INFLIGHT_FD";
+ case VHOST_USER_GPU_SET_SOCKET:
+ return "GPU_SET_SOCKET";
+ case VHOST_USER_RESET_DEVICE:
+ return "RESET_DEVICE";
+ case VHOST_USER_GET_MAX_MEM_SLOTS:
+ return "GET_MAX_MEM_SLOTS";
+ case VHOST_USER_ADD_MEM_REG:
+ return "ADD_MEM_REG";
+ case VHOST_USER_REM_MEM_REG:
+ return "REM_MEM_REG";
+ case VHOST_USER_SET_STATUS:
+ return "SET_STATUS";
+ case VHOST_USER_GET_STATUS:
+ return "GET_STATUS";
+ default:
+ return "<unknown>";
+ }
+}
+
typedef struct VhostUserMemoryRegion {
uint64_t guest_phys_addr;
uint64_t memory_size;
@@ -311,7 +399,8 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
return -EPROTO;
}
- trace_vhost_user_read(msg->hdr.request, msg->hdr.flags);
+ trace_vhost_user_read(msg->hdr.request,
+ vhost_req_name(msg->hdr.request), msg->hdr.flags);
return 0;
}
@@ -431,7 +520,8 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
return ret < 0 ? -saved_errno : -EIO;
}
- trace_vhost_user_write(msg->hdr.request, msg->hdr.flags);
+ trace_vhost_user_write(msg->hdr.request, vhost_req_name(msg->hdr.request),
+ msg->hdr.flags);
return 0;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 21/23] vhost-user: make trace events more readable
2025-10-15 14:58 ` [PATCH v3 21/23] vhost-user: make trace events more readable Vladimir Sementsov-Ogievskiy
@ 2025-11-03 22:22 ` Michael S. Tsirkin
0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2025-11-03 22:22 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin,
Philippe Mathieu-Daudé, Raphael Norwitz
On Wed, Oct 15, 2025 at 05:58:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
> hw/virtio/trace-events | 4 +-
> hw/virtio/vhost-user.c | 94 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 658cc365e7..aa1ffa5e94 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -25,8 +25,8 @@ vhost_user_set_mem_table_withfd(int index, const char *name, uint64_t memory_siz
> vhost_user_postcopy_waker(const char *rb, uint64_t rb_offset) "%s + 0x%"PRIx64
> vhost_user_postcopy_waker_found(uint64_t client_addr) "0x%"PRIx64
> vhost_user_postcopy_waker_nomatch(const char *rb, uint64_t rb_offset) "%s + 0x%"PRIx64
> -vhost_user_read(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32""
> -vhost_user_write(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32""
> +vhost_user_read(uint32_t req, const char *req_name, uint32_t flags) "req:%d (%s) flags:0x%"PRIx32""
> +vhost_user_write(uint32_t req, const char *req_name, uint32_t flags) "req:%d (%s) flags:0x%"PRIx32""
> vhost_user_create_notifier(int idx, void *n) "idx:%d n:%p"
>
> # vhost-vdpa.c
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 23e7c12b16..e45b74eddd 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -119,6 +119,94 @@ typedef enum VhostUserBackendRequest {
> VHOST_USER_BACKEND_MAX
> } VhostUserBackendRequest;
>
> +static const char *vhost_req_name(VhostUserRequest req)
> +{
> + switch (req) {
> + case VHOST_USER_NONE:
> + return "NONE";
> + case VHOST_USER_GET_FEATURES:
> + return "GET_FEATURES";
> + case VHOST_USER_SET_FEATURES:
> + return "SET_FEATURES";
> + case VHOST_USER_SET_OWNER:
> + return "SET_OWNER";
> + case VHOST_USER_RESET_OWNER:
> + return "RESET_OWNER";
> + case VHOST_USER_SET_MEM_TABLE:
> + return "SET_MEM_TABLE";
> + case VHOST_USER_SET_LOG_BASE:
> + return "SET_LOG_BASE";
> + case VHOST_USER_SET_LOG_FD:
> + return "SET_LOG_FD";
> + case VHOST_USER_SET_VRING_NUM:
> + return "SET_VRING_NUM";
> + case VHOST_USER_SET_VRING_ADDR:
> + return "SET_VRING_ADDR";
> + case VHOST_USER_SET_VRING_BASE:
> + return "SET_VRING_BASE";
> + case VHOST_USER_GET_VRING_BASE:
> + return "GET_VRING_BASE";
> + case VHOST_USER_SET_VRING_KICK:
> + return "SET_VRING_KICK";
> + case VHOST_USER_SET_VRING_CALL:
> + return "SET_VRING_CALL";
> + case VHOST_USER_SET_VRING_ERR:
> + return "SET_VRING_ERR";
> + case VHOST_USER_GET_PROTOCOL_FEATURES:
> + return "GET_PROTOCOL_FEATURES";
> + case VHOST_USER_SET_PROTOCOL_FEATURES:
> + return "SET_PROTOCOL_FEATURES";
> + case VHOST_USER_GET_QUEUE_NUM:
> + return "GET_QUEUE_NUM";
> + case VHOST_USER_SET_VRING_ENABLE:
> + return "SET_VRING_ENABLE";
> + case VHOST_USER_SEND_RARP:
> + return "SEND_RARP";
> + case VHOST_USER_NET_SET_MTU:
> + return "NET_SET_MTU";
> + case VHOST_USER_SET_BACKEND_REQ_FD:
> + return "SET_BACKEND_REQ_FD";
> + case VHOST_USER_IOTLB_MSG:
> + return "IOTLB_MSG";
> + case VHOST_USER_SET_VRING_ENDIAN:
> + return "SET_VRING_ENDIAN";
> + case VHOST_USER_GET_CONFIG:
> + return "GET_CONFIG";
> + case VHOST_USER_SET_CONFIG:
> + return "SET_CONFIG";
> + case VHOST_USER_CREATE_CRYPTO_SESSION:
> + return "CREATE_CRYPTO_SESSION";
> + case VHOST_USER_CLOSE_CRYPTO_SESSION:
> + return "CLOSE_CRYPTO_SESSION";
> + case VHOST_USER_POSTCOPY_ADVISE:
> + return "POSTCOPY_ADVISE";
> + case VHOST_USER_POSTCOPY_LISTEN:
> + return "POSTCOPY_LISTEN";
> + case VHOST_USER_POSTCOPY_END:
> + return "POSTCOPY_END";
> + case VHOST_USER_GET_INFLIGHT_FD:
> + return "GET_INFLIGHT_FD";
> + case VHOST_USER_SET_INFLIGHT_FD:
> + return "SET_INFLIGHT_FD";
> + case VHOST_USER_GPU_SET_SOCKET:
> + return "GPU_SET_SOCKET";
> + case VHOST_USER_RESET_DEVICE:
> + return "RESET_DEVICE";
> + case VHOST_USER_GET_MAX_MEM_SLOTS:
> + return "GET_MAX_MEM_SLOTS";
> + case VHOST_USER_ADD_MEM_REG:
> + return "ADD_MEM_REG";
> + case VHOST_USER_REM_MEM_REG:
> + return "REM_MEM_REG";
> + case VHOST_USER_SET_STATUS:
> + return "SET_STATUS";
> + case VHOST_USER_GET_STATUS:
> + return "GET_STATUS";
> + default:
> + return "<unknown>";
> + }
> +}
> +
Please use a macro so we don't have this duplication.
E.g.
#define VHOST_USER_CASE(name) \
case VHOST_USER_##name: \
return #name;
> typedef struct VhostUserMemoryRegion {
> uint64_t guest_phys_addr;
> uint64_t memory_size;
> @@ -311,7 +399,8 @@ static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
> return -EPROTO;
> }
>
> - trace_vhost_user_read(msg->hdr.request, msg->hdr.flags);
> + trace_vhost_user_read(msg->hdr.request,
> + vhost_req_name(msg->hdr.request), msg->hdr.flags);
>
> return 0;
> }
> @@ -431,7 +520,8 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
> return ret < 0 ? -saved_errno : -EIO;
> }
>
> - trace_vhost_user_write(msg->hdr.request, msg->hdr.flags);
> + trace_vhost_user_write(msg->hdr.request, vhost_req_name(msg->hdr.request),
> + msg->hdr.flags);
>
> return 0;
> }
> --
> 2.48.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 22/23] vhost-user-blk: add some useful trace-points
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (20 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 21/23] vhost-user: make trace events more readable Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-11-03 21:28 ` Michael S. Tsirkin
2025-10-15 14:58 ` [PATCH v3 23/23] vhost: " Vladimir Sementsov-Ogievskiy
2025-10-18 15:33 ` [PATCH v3 00/23] vhost refactoring and fixes Lei Yang
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz, Kevin Wolf, Hanna Reitz,
open list:Block layer core
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/block/trace-events | 10 ++++++++++
hw/block/vhost-user-blk.c | 19 +++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/hw/block/trace-events b/hw/block/trace-events
index cc9a9f2460..dbaa5ca6cb 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -58,6 +58,16 @@ virtio_blk_handle_zone_mgmt(void *vdev, void *req, uint8_t op, int64_t sector, i
virtio_blk_handle_zone_reset_all(void *vdev, void *req, int64_t sector, int64_t len) "vdev %p req %p sector 0x%" PRIx64 " cap 0x%" PRIx64 ""
virtio_blk_handle_zone_append(void *vdev, void *req, int64_t sector) "vdev %p req %p, append sector 0x%" PRIx64 ""
+# vhost-user-blk.c
+vhost_user_blk_start_in(void *vdev) "vdev %p"
+vhost_user_blk_start_out(void *vdev) "vdev %p"
+vhost_user_blk_stop_in(void *vdev) "vdev %p"
+vhost_user_blk_stop_out(void *vdev) "vdev %p"
+vhost_user_blk_connect_in(void *vdev) "vdev %p"
+vhost_user_blk_connect_out(void *vdev) "vdev %p"
+vhost_user_blk_device_realize_in(void *vdev) "vdev %p"
+vhost_user_blk_device_realize_out(void *vdev) "vdev %p"
+
# hd-geometry.c
hd_geometry_lchs_guess(void *blk, int cyls, int heads, int secs) "blk %p LCHS %d %d %d"
hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int trans) "blk %p CHS %u %u %u trans %d"
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index de7a810c93..a5daed4346 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -31,6 +31,7 @@
#include "hw/virtio/virtio-access.h"
#include "system/system.h"
#include "system/runstate.h"
+#include "trace.h"
static const int user_feature_bits[] = {
VIRTIO_BLK_F_SIZE_MAX,
@@ -137,6 +138,8 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int i, ret;
+ trace_vhost_user_blk_start_in(vdev);
+
if (!k->set_guest_notifiers) {
error_setg(errp, "binding does not support guest notifiers");
return -ENOSYS;
@@ -192,6 +195,8 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
}
s->started_vu = true;
+ trace_vhost_user_blk_start_out(vdev);
+
return ret;
err_guest_notifiers:
@@ -212,6 +217,8 @@ static int vhost_user_blk_stop(VirtIODevice *vdev)
int ret;
bool force_stop = false;
+ trace_vhost_user_blk_stop_in(vdev);
+
if (!s->started_vu) {
return 0;
}
@@ -233,6 +240,9 @@ static int vhost_user_blk_stop(VirtIODevice *vdev)
}
vhost_dev_disable_notifiers(&s->dev, vdev);
+
+ trace_vhost_user_blk_stop_out(vdev);
+
return ret;
}
@@ -340,6 +350,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
VHostUserBlk *s = VHOST_USER_BLK(vdev);
int ret = 0;
+ trace_vhost_user_blk_connect_in(vdev);
+
if (s->connected) {
return 0;
}
@@ -365,6 +377,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
ret = vhost_user_blk_start(vdev, errp);
}
+ trace_vhost_user_blk_connect_out(vdev);
+
return ret;
}
@@ -455,6 +469,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
int retries;
int i, ret;
+ trace_vhost_user_blk_device_realize_in(vdev);
+
if (!s->chardev.chr) {
error_setg(errp, "chardev is mandatory");
return;
@@ -514,6 +530,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
vhost_user_blk_event, NULL, (void *)dev,
NULL, true);
+
+ trace_vhost_user_blk_device_realize_out(vdev);
+
return;
virtio_err:
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 22/23] vhost-user-blk: add some useful trace-points
2025-10-15 14:58 ` [PATCH v3 22/23] vhost-user-blk: add some useful trace-points Vladimir Sementsov-Ogievskiy
@ 2025-11-03 21:28 ` Michael S. Tsirkin
2025-11-04 11:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 36+ messages in thread
From: Michael S. Tsirkin @ 2025-11-03 21:28 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin,
Raphael Norwitz, Kevin Wolf, Hanna Reitz,
open list:Block layer core
On Wed, Oct 15, 2025 at 05:58:06PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> ---
> hw/block/trace-events | 10 ++++++++++
> hw/block/vhost-user-blk.c | 19 +++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index cc9a9f2460..dbaa5ca6cb 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -58,6 +58,16 @@ virtio_blk_handle_zone_mgmt(void *vdev, void *req, uint8_t op, int64_t sector, i
> virtio_blk_handle_zone_reset_all(void *vdev, void *req, int64_t sector, int64_t len) "vdev %p req %p sector 0x%" PRIx64 " cap 0x%" PRIx64 ""
> virtio_blk_handle_zone_append(void *vdev, void *req, int64_t sector) "vdev %p req %p, append sector 0x%" PRIx64 ""
>
> +# vhost-user-blk.c
> +vhost_user_blk_start_in(void *vdev) "vdev %p"
> +vhost_user_blk_start_out(void *vdev) "vdev %p"
> +vhost_user_blk_stop_in(void *vdev) "vdev %p"
> +vhost_user_blk_stop_out(void *vdev) "vdev %p"
> +vhost_user_blk_connect_in(void *vdev) "vdev %p"
> +vhost_user_blk_connect_out(void *vdev) "vdev %p"
> +vhost_user_blk_device_realize_in(void *vdev) "vdev %p"
> +vhost_user_blk_device_realize_out(void *vdev) "vdev %p"
> +
> # hd-geometry.c
> hd_geometry_lchs_guess(void *blk, int cyls, int heads, int secs) "blk %p LCHS %d %d %d"
> hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int trans) "blk %p CHS %u %u %u trans %d"
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index de7a810c93..a5daed4346 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -31,6 +31,7 @@
> #include "hw/virtio/virtio-access.h"
> #include "system/system.h"
> #include "system/runstate.h"
> +#include "trace.h"
>
> static const int user_feature_bits[] = {
> VIRTIO_BLK_F_SIZE_MAX,
> @@ -137,6 +138,8 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> int i, ret;
>
> + trace_vhost_user_blk_start_in(vdev);
> +
> if (!k->set_guest_notifiers) {
> error_setg(errp, "binding does not support guest notifiers");
> return -ENOSYS;
> @@ -192,6 +195,8 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
> }
> s->started_vu = true;
>
> + trace_vhost_user_blk_start_out(vdev);
> +
> return ret;
>
> err_guest_notifiers:
> @@ -212,6 +217,8 @@ static int vhost_user_blk_stop(VirtIODevice *vdev)
> int ret;
> bool force_stop = false;
>
> + trace_vhost_user_blk_stop_in(vdev);
> +
> if (!s->started_vu) {
> return 0;
> }
> @@ -233,6 +240,9 @@ static int vhost_user_blk_stop(VirtIODevice *vdev)
> }
>
> vhost_dev_disable_notifiers(&s->dev, vdev);
> +
> + trace_vhost_user_blk_stop_out(vdev);
> +
> return ret;
> }
>
> @@ -340,6 +350,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> int ret = 0;
>
> + trace_vhost_user_blk_connect_in(vdev);
> +
> if (s->connected) {
> return 0;
> }
> @@ -365,6 +377,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
> ret = vhost_user_blk_start(vdev, errp);
> }
>
> + trace_vhost_user_blk_connect_out(vdev);
> +
> return ret;
> }
>
> @@ -455,6 +469,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> int retries;
> int i, ret;
>
> + trace_vhost_user_blk_device_realize_in(vdev);
> +
> if (!s->chardev.chr) {
> error_setg(errp, "chardev is mandatory");
> return;
> @@ -514,6 +530,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> vhost_user_blk_event, NULL, (void *)dev,
> NULL, true);
> +
> + trace_vhost_user_blk_device_realize_out(vdev);
> +
> return;
>
> virtio_err:
Is it ok that on error there's an in trace but not an out trace?
> --
> 2.48.1
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 22/23] vhost-user-blk: add some useful trace-points
2025-11-03 21:28 ` Michael S. Tsirkin
@ 2025-11-04 11:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-11-04 11:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin,
Raphael Norwitz, Kevin Wolf, Hanna Reitz,
open list:Block layer core
On 04.11.25 00:28, Michael S. Tsirkin wrote:
> On Wed, Oct 15, 2025 at 05:58:06PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
>> ---
>> hw/block/trace-events | 10 ++++++++++
>> hw/block/vhost-user-blk.c | 19 +++++++++++++++++++
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/hw/block/trace-events b/hw/block/trace-events
>> index cc9a9f2460..dbaa5ca6cb 100644
>> --- a/hw/block/trace-events
>> +++ b/hw/block/trace-events
>> @@ -58,6 +58,16 @@ virtio_blk_handle_zone_mgmt(void *vdev, void *req, uint8_t op, int64_t sector, i
>> virtio_blk_handle_zone_reset_all(void *vdev, void *req, int64_t sector, int64_t len) "vdev %p req %p sector 0x%" PRIx64 " cap 0x%" PRIx64 ""
>> virtio_blk_handle_zone_append(void *vdev, void *req, int64_t sector) "vdev %p req %p, append sector 0x%" PRIx64 ""
>>
>> +# vhost-user-blk.c
>> +vhost_user_blk_start_in(void *vdev) "vdev %p"
>> +vhost_user_blk_start_out(void *vdev) "vdev %p"
>> +vhost_user_blk_stop_in(void *vdev) "vdev %p"
>> +vhost_user_blk_stop_out(void *vdev) "vdev %p"
>> +vhost_user_blk_connect_in(void *vdev) "vdev %p"
>> +vhost_user_blk_connect_out(void *vdev) "vdev %p"
>> +vhost_user_blk_device_realize_in(void *vdev) "vdev %p"
>> +vhost_user_blk_device_realize_out(void *vdev) "vdev %p"
>> +
>> # hd-geometry.c
>> hd_geometry_lchs_guess(void *blk, int cyls, int heads, int secs) "blk %p LCHS %d %d %d"
>> hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int trans) "blk %p CHS %u %u %u trans %d"
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index de7a810c93..a5daed4346 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -31,6 +31,7 @@
>> #include "hw/virtio/virtio-access.h"
>> #include "system/system.h"
>> #include "system/runstate.h"
>> +#include "trace.h"
>>
>> static const int user_feature_bits[] = {
>> VIRTIO_BLK_F_SIZE_MAX,
>> @@ -137,6 +138,8 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> int i, ret;
>>
>> + trace_vhost_user_blk_start_in(vdev);
>> +
>> if (!k->set_guest_notifiers) {
>> error_setg(errp, "binding does not support guest notifiers");
>> return -ENOSYS;
>> @@ -192,6 +195,8 @@ static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
>> }
>> s->started_vu = true;
>>
>> + trace_vhost_user_blk_start_out(vdev);
>> +
>> return ret;
>>
>> err_guest_notifiers:
>> @@ -212,6 +217,8 @@ static int vhost_user_blk_stop(VirtIODevice *vdev)
>> int ret;
>> bool force_stop = false;
>>
>> + trace_vhost_user_blk_stop_in(vdev);
>> +
>> if (!s->started_vu) {
>> return 0;
>> }
>> @@ -233,6 +240,9 @@ static int vhost_user_blk_stop(VirtIODevice *vdev)
>> }
>>
>> vhost_dev_disable_notifiers(&s->dev, vdev);
>> +
>> + trace_vhost_user_blk_stop_out(vdev);
>> +
>> return ret;
>> }
>>
>> @@ -340,6 +350,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
>> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>> int ret = 0;
>>
>> + trace_vhost_user_blk_connect_in(vdev);
>> +
>> if (s->connected) {
>> return 0;
>> }
>> @@ -365,6 +377,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
>> ret = vhost_user_blk_start(vdev, errp);
>> }
>>
>> + trace_vhost_user_blk_connect_out(vdev);
>> +
>> return ret;
>> }
>>
>> @@ -455,6 +469,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>> int retries;
>> int i, ret;
>>
>> + trace_vhost_user_blk_device_realize_in(vdev);
>> +
>> if (!s->chardev.chr) {
>> error_setg(errp, "chardev is mandatory");
>> return;
>> @@ -514,6 +530,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>> qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
>> vhost_user_blk_event, NULL, (void *)dev,
>> NULL, true);
>> +
>> + trace_vhost_user_blk_device_realize_out(vdev);
>> +
>> return;
>>
>> virtio_err:
>
>
> Is it ok that on error there's an in trace but not an out trace?
>
I thought about it.
In my opinion, switching to use common "out: " and gotos instead of simple returns,
is too much pay for proper out trace-points.
So, it's a compromise: we have at least trace-point on success-out, and don't make
the code more complicated just for good trace points.
Probably, we can invent some smart macro, which will inject out tracepoint using
g_auto...
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 23/23] vhost: add some useful trace-points
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (21 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 22/23] vhost-user-blk: add some useful trace-points Vladimir Sementsov-Ogievskiy
@ 2025-10-15 14:58 ` Vladimir Sementsov-Ogievskiy
2025-11-03 21:26 ` Michael S. Tsirkin
2025-10-18 15:33 ` [PATCH v3 00/23] vhost refactoring and fixes Lei Yang
23 siblings, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-15 14:58 UTC (permalink / raw)
To: mst
Cc: sgarzare, raphael, qemu-devel, vsementsov, yc-core, d-tatianin,
Raphael Norwitz
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
---
hw/virtio/trace-events | 12 ++++++++++--
hw/virtio/vhost.c | 20 ++++++++++++++++++--
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index aa1ffa5e94..c2529814f9 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -9,8 +9,16 @@ vhost_section(const char *name) "%s"
vhost_reject_section(const char *name, int d) "%s:%d"
vhost_iotlb_miss(void *dev, int step) "%p step %d"
vhost_dev_cleanup(void *dev) "%p"
-vhost_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
-vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
+vhost_dev_start_in(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
+vhost_dev_start_out(void *dev, const char *name) "%p:%s"
+vhost_dev_stop_in(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
+vhost_dev_stop_out(void *dev, const char *name) "%p:%s"
+vhost_virtque_start_in(void *dev, const char *name, int idx) "%p:%s %d"
+vhost_virtque_start_out(void *dev, const char *name, int idx) "%p:%s %d"
+vhost_virtque_stop_in(void *dev, const char *name, int idx) "%p:%s %d"
+vhost_virtque_stop_out(void *dev, const char *name, int idx) "%p:%s %d"
+vhost_dev_init_in(void *dev) "%p"
+vhost_dev_init_out(void *dev) "%p"
# vhost-user.c
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index fb5c4ba1ca..7ba90c24db 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1333,6 +1333,8 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
};
struct VirtQueue *vvq = virtio_get_queue(vdev, idx);
+ trace_vhost_virtque_start_in(dev, vdev->name, idx);
+
r = vhost_vrings_map(dev, vdev, vq, idx);
if (r <= 0) {
return r;
@@ -1394,6 +1396,8 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
}
}
+ trace_vhost_virtque_start_out(dev, vdev->name, idx);
+
return 0;
fail:
@@ -1412,6 +1416,8 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
};
int r = 0;
+ trace_vhost_virtque_stop_in(dev, vdev->name, idx);
+
if (virtio_queue_get_desc_addr(vdev, idx) == 0) {
/* Don't stop the virtqueue which might have not been started */
return 0;
@@ -1445,6 +1451,8 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
}
vhost_vrings_unmap(dev, vq, true);
+
+ trace_vhost_virtque_stop_out(dev, vdev->name, idx);
return r;
}
@@ -1616,6 +1624,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
{
int i, r, n_initialized_vqs = 0;
+ trace_vhost_dev_init_in(hdev);
+
hdev->vdev = NULL;
hdev->migration_blocker = NULL;
@@ -1700,6 +1710,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
goto fail;
}
+ trace_vhost_dev_init_out(hdev);
+
return 0;
fail:
@@ -2048,7 +2060,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
/* should only be called after backend is connected */
assert(hdev->vhost_ops);
- trace_vhost_dev_start(hdev, vdev->name, vrings);
+ trace_vhost_dev_start_in(hdev, vdev->name, vrings);
vdev->vhost_started = true;
hdev->started = true;
@@ -2133,6 +2145,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
}
}
vhost_start_config_intr(hdev);
+
+ trace_vhost_dev_start_out(hdev, vdev->name);
return 0;
fail_iotlb:
if (vhost_dev_has_iommu(hdev) &&
@@ -2182,7 +2196,7 @@ static int do_vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev,
event_notifier_cleanup(
&hdev->vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier);
- trace_vhost_dev_stop(hdev, vdev->name, vrings);
+ trace_vhost_dev_stop_in(hdev, vdev->name, vrings);
if (hdev->vhost_ops->vhost_dev_start) {
hdev->vhost_ops->vhost_dev_start(hdev, false);
@@ -2212,6 +2226,8 @@ static int do_vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev,
hdev->started = false;
vdev->vhost_started = false;
hdev->vdev = NULL;
+
+ trace_vhost_dev_stop_out(hdev, vdev->name);
return rc;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 23/23] vhost: add some useful trace-points
2025-10-15 14:58 ` [PATCH v3 23/23] vhost: " Vladimir Sementsov-Ogievskiy
@ 2025-11-03 21:26 ` Michael S. Tsirkin
0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2025-11-03 21:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: sgarzare, raphael, qemu-devel, yc-core, d-tatianin,
Raphael Norwitz
On Wed, Oct 15, 2025 at 05:58:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
> ---
> hw/virtio/trace-events | 12 ++++++++++--
> hw/virtio/vhost.c | 20 ++++++++++++++++++--
> 2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index aa1ffa5e94..c2529814f9 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -9,8 +9,16 @@ vhost_section(const char *name) "%s"
> vhost_reject_section(const char *name, int d) "%s:%d"
> vhost_iotlb_miss(void *dev, int step) "%p step %d"
> vhost_dev_cleanup(void *dev) "%p"
> -vhost_dev_start(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> -vhost_dev_stop(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> +vhost_dev_start_in(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> +vhost_dev_start_out(void *dev, const char *name) "%p:%s"
> +vhost_dev_stop_in(void *dev, const char *name, bool vrings) "%p:%s vrings:%d"
> +vhost_dev_stop_out(void *dev, const char *name) "%p:%s"
> +vhost_virtque_start_in(void *dev, const char *name, int idx) "%p:%s %d"
> +vhost_virtque_start_out(void *dev, const char *name, int idx) "%p:%s %d"
> +vhost_virtque_stop_in(void *dev, const char *name, int idx) "%p:%s %d"
> +vhost_virtque_stop_out(void *dev, const char *name, int idx) "%p:%s %d"
virtqueue_ not virtque_
> +vhost_dev_init_in(void *dev) "%p"
> +vhost_dev_init_out(void *dev) "%p"
>
>
> # vhost-user.c
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index fb5c4ba1ca..7ba90c24db 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1333,6 +1333,8 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
> };
> struct VirtQueue *vvq = virtio_get_queue(vdev, idx);
>
> + trace_vhost_virtque_start_in(dev, vdev->name, idx);
> +
should be "virtqueue"
> r = vhost_vrings_map(dev, vdev, vq, idx);
> if (r <= 0) {
> return r;
> @@ -1394,6 +1396,8 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
> }
> }
>
> + trace_vhost_virtque_start_out(dev, vdev->name, idx);
> +
same
> return 0;
>
> fail:
> @@ -1412,6 +1416,8 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
> };
> int r = 0;
>
> + trace_vhost_virtque_stop_in(dev, vdev->name, idx);
> +
same
> if (virtio_queue_get_desc_addr(vdev, idx) == 0) {
> /* Don't stop the virtqueue which might have not been started */
> return 0;
> @@ -1445,6 +1451,8 @@ static int do_vhost_virtqueue_stop(struct vhost_dev *dev,
> }
>
> vhost_vrings_unmap(dev, vq, true);
> +
> + trace_vhost_virtque_stop_out(dev, vdev->name, idx);
same
> return r;
> }
>
> @@ -1616,6 +1624,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> {
> int i, r, n_initialized_vqs = 0;
>
> + trace_vhost_dev_init_in(hdev);
> +
> hdev->vdev = NULL;
> hdev->migration_blocker = NULL;
>
> @@ -1700,6 +1710,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> goto fail;
> }
>
> + trace_vhost_dev_init_out(hdev);
> +
> return 0;
>
> fail:
> @@ -2048,7 +2060,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
> /* should only be called after backend is connected */
> assert(hdev->vhost_ops);
>
> - trace_vhost_dev_start(hdev, vdev->name, vrings);
> + trace_vhost_dev_start_in(hdev, vdev->name, vrings);
>
> vdev->vhost_started = true;
> hdev->started = true;
> @@ -2133,6 +2145,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
> }
> }
> vhost_start_config_intr(hdev);
> +
> + trace_vhost_dev_start_out(hdev, vdev->name);
> return 0;
> fail_iotlb:
> if (vhost_dev_has_iommu(hdev) &&
> @@ -2182,7 +2196,7 @@ static int do_vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev,
> event_notifier_cleanup(
> &hdev->vqs[VHOST_QUEUE_NUM_CONFIG_INR].masked_config_notifier);
>
> - trace_vhost_dev_stop(hdev, vdev->name, vrings);
> + trace_vhost_dev_stop_in(hdev, vdev->name, vrings);
>
> if (hdev->vhost_ops->vhost_dev_start) {
> hdev->vhost_ops->vhost_dev_start(hdev, false);
> @@ -2212,6 +2226,8 @@ static int do_vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev,
> hdev->started = false;
> vdev->vhost_started = false;
> hdev->vdev = NULL;
> +
> + trace_vhost_dev_stop_out(hdev, vdev->name);
> return rc;
> }
>
> --
> 2.48.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 00/23] vhost refactoring and fixes
2025-10-15 14:57 [PATCH v3 00/23] vhost refactoring and fixes Vladimir Sementsov-Ogievskiy
` (22 preceding siblings ...)
2025-10-15 14:58 ` [PATCH v3 23/23] vhost: " Vladimir Sementsov-Ogievskiy
@ 2025-10-18 15:33 ` Lei Yang
23 siblings, 0 replies; 36+ messages in thread
From: Lei Yang @ 2025-10-18 15:33 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: mst, sgarzare, raphael, qemu-devel, yc-core, d-tatianin
Tested this series of patches with virtio-net regression tests,
everything works fine.
Tested-by: Lei Yang <leiyang@redhat.com>
On Wed, Oct 15, 2025 at 11:03 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Hi all. That's rebased and updated first part of
> [PATCH 00/33] vhost-user-blk: live-backend local migration
>
> v3:
> 01-05,07-23: add r-b by Daniil
> 01,03,04,11-12,15,19,22,23: add r-b by Raphael
> 02: - add a-b by Markus and Raphael
> - touch-up commit message
> 06: carefully follow existing pattern
> of split into features / features_ex
>
> Vladimir Sementsov-Ogievskiy (23):
> vhost-user: rework enabling vrings
> vhost: drop backend_features field
> vhost-user: introduce vhost_user_has_protocol_feature() helper
> vhost: move protocol_features to vhost_user
> vhost-user-gpu: drop code duplication
> vhost: make vhost_dev.features private
> virtio: move common part of _set_guest_notifier to generic code
> virtio: drop *_set_guest_notifier_fd_handler() helpers
> vhost-user: keep QIOChannelSocket for backend channel
> vhost: vhost_virtqueue_start(): fix failure path
> vhost: make vhost_memory_unmap() null-safe
> vhost: simplify calls to vhost_memory_unmap()
> vhost: move vrings mapping to the top of vhost_virtqueue_start()
> vhost: vhost_virtqueue_start(): drop extra local variables
> vhost: final refactoring of vhost vrings map/unmap
> vhost: simplify vhost_dev_init() error-path
> vhost: move busyloop timeout initialization to vhost_virtqueue_init()
> vhost: introduce check_memslots() helper
> vhost: vhost_dev_init(): simplify features initialization
> hw/virtio/virtio-bus: refactor virtio_bus_set_host_notifier()
> vhost-user: make trace events more readable
> vhost-user-blk: add some useful trace-points
> vhost: add some useful trace-points
>
> backends/cryptodev-vhost.c | 9 +-
> hw/block/trace-events | 10 ++
> hw/block/vhost-user-blk.c | 20 ++-
> hw/display/vhost-user-gpu.c | 11 +-
> hw/net/vhost_net.c | 35 ++--
> hw/scsi/vhost-scsi.c | 1 -
> hw/scsi/vhost-user-scsi.c | 1 -
> hw/virtio/trace-events | 16 +-
> hw/virtio/vdpa-dev.c | 3 +-
> hw/virtio/vhost-user-base.c | 8 +-
> hw/virtio/vhost-user.c | 285 ++++++++++++++++++++++---------
> hw/virtio/vhost.c | 300 +++++++++++++++++----------------
> hw/virtio/virtio-bus.c | 18 +-
> hw/virtio/virtio-hmp-cmds.c | 2 -
> hw/virtio/virtio-mmio.c | 41 +----
> hw/virtio/virtio-pci.c | 34 +---
> hw/virtio/virtio-qmp.c | 12 +-
> hw/virtio/virtio.c | 48 +++---
> include/hw/virtio/vhost-user.h | 3 +
> include/hw/virtio/vhost.h | 63 +++++--
> include/hw/virtio/virtio-pci.h | 3 -
> include/hw/virtio/virtio.h | 16 +-
> net/vhost-vdpa.c | 7 +-
> qapi/virtio.json | 3 -
> 24 files changed, 537 insertions(+), 412 deletions(-)
>
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread