From: Prasad Pandit <p...@fedoraproject.org>
QEMU threads use vhost_user_write/read calls to send
and receive request/reply messages from a vhost-user
device. When multiple threads communicate with the
same vhost-user device, they can receive each other's
messages, resulting in an erroneous state.
When fault_thread exits upon completion of Postcopy
migration, it sends a 'postcopy_end' message to the
vhost-user device. But sometimes 'postcopy_end' message
is sent while vhost device is being setup via
vhost_dev_start().
Thread-1 Thread-2
vhost_dev_start postcopy_ram_incoming_cleanup
vhost_device_iotlb_miss postcopy_notify
vhost_backend_update_device_iotlb vhost_user_postcopy_notifier
vhost_user_send_device_iotlb_msg vhost_user_postcopy_end
process_message_reply process_message_reply
vhost_user_read vhost_user_read
vhost_user_read_header vhost_user_read_header
"Fail to update device iotlb" "Failed to receive reply to postcopy_end"
This creates confusion when vhost-user device receives
'postcopy_end' message while it is trying to update IOTLB entries.
vhost_user_read_header:
700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5.
vhost_device_iotlb_miss:
700871,700871: Fail to update device iotlb
vhost_user_postcopy_end:
700871,700900: Failed to receive reply to postcopy_end
vhost_user_read_header:
700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5.
Here fault thread seems to end the postcopy migration
while another thread is starting the vhost-user device.
Add a mutex lock to hold for one request-reply cycle
and avoid such race condition.
Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
Suggested-by: Peter Xu <pet...@redhat.com>
Signed-off-by: Prasad Pandit <p...@fedoraproject.org>
---
hw/virtio/vhost-user.c | 74 ++++++++++++++++++++++++++++++++++
include/hw/virtio/vhost-user.h | 3 ++
2 files changed, 77 insertions(+)
v2:
- Place QEMU_LOCK_GUARD near the vhost_user_write() calls, holding
the lock for longer fails some tests during rpmbuild(8).
- rpmbuild(8) fails for some SRPMs, not all. RHEL-9 SRPM builds with
this patch, whereas Fedora SRPM does not build.
- The host OS also seems to affect rpmbuild(8). Some SRPMs build well
on RHEL-9, but not on Fedora-40 machine.
- koji builds successful with this patch
https://koji.fedoraproject.org/koji/taskinfo?taskID=122254011
https://koji.fedoraproject.org/koji/taskinfo?taskID=122252369
v1: Use QEMU_LOCK_GUARD(), rename lock variable
-
https://lore.kernel.org/qemu-devel/20240808095147.291626-3-ppan...@redhat.com/
v0:
- https://lore.kernel.org/all/Zo_9OlX0pV0paFj7@x1n/
- https://lore.kernel.org/all/20240720153808-mutt-send-email-...@kernel.org/
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 00561daa06..7b030ae2cd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -24,6 +24,7 @@
#include "qemu/main-loop.h"
#include "qemu/uuid.h"
#include "qemu/sockets.h"
+#include "qemu/lockable.h"
#include "sysemu/runstate.h"
#include "sysemu/cryptodev.h"
#include "migration/postcopy-ram.h"
@@ -446,6 +447,10 @@ static int vhost_user_set_log_base(struct vhost_dev *dev,
uint64_t base,
.hdr.size = sizeof(msg.payload.log),
};
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
/* Send only once with first queue pair */
if (dev->vq_index != 0) {
return 0;
@@ -664,6 +669,7 @@ static int send_remove_regions(struct vhost_dev *dev,
bool reply_supported)
{
struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
struct vhost_memory_region *shadow_reg;
int i, fd, shadow_reg_idx, ret;
ram_addr_t offset;
@@ -685,6 +691,8 @@ static int send_remove_regions(struct vhost_dev *dev,
vhost_user_fill_msg_region(®ion_buffer, shadow_reg, 0);
msg->payload.mem_reg.region = region_buffer;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -718,6 +726,7 @@ static int send_add_regions(struct vhost_dev *dev,
bool reply_supported, bool track_ramblocks)
{
struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
int i, fd, ret, reg_idx, reg_fd_idx;
struct vhost_memory_region *reg;
MemoryRegion *mr;
@@ -746,6 +755,8 @@ static int send_add_regions(struct vhost_dev *dev,
vhost_user_fill_msg_region(®ion_buffer, reg, offset);
msg->payload.mem_reg.region = region_buffer;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, msg, &fd, 1);
if (ret < 0) {
return ret;
@@ -893,6 +904,7 @@ static int vhost_user_set_mem_table_postcopy(struct
vhost_dev *dev,
bool config_mem_slots)
{
struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
size_t fd_num = 0;
VhostUserMsg msg_reply;
@@ -926,6 +938,8 @@ static int vhost_user_set_mem_table_postcopy(struct
vhost_dev *dev,
return ret;
}
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, fds, fd_num);
if (ret < 0) {
return ret;
@@ -1005,6 +1019,7 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
struct vhost_memory *mem)
{
struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
size_t fd_num = 0;
bool do_postcopy = u->postcopy_listen && u->postcopy_fd.handler;
@@ -1044,6 +1059,8 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
return ret;
}
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, fds, fd_num);
if (ret < 0) {
return ret;
@@ -1089,6 +1106,10 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int
request, uint64_t *u64)
return 0;
}
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -1138,6 +1159,10 @@ static int vhost_user_write_sync(struct vhost_dev *dev,
VhostUserMsg *msg,
}
}
+/* struct vhost_user *u = dev->opaque;
+ * struct VhostUserState *us = u->user;
+ * QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+ */
ret = vhost_user_write(dev, msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -1277,6 +1302,8 @@ static int vhost_user_get_vring_base(struct vhost_dev
*dev,
.hdr.size = sizeof(msg.payload.state),
};
struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
VhostUserHostNotifier *n = fetch_notifier(u->user, ring->index);
if (n) {
@@ -1669,6 +1696,9 @@ int vhost_user_get_shared_object(struct vhost_dev *dev,
unsigned char *uuid,
};
memcpy(msg.payload.object.uuid, uuid, sizeof(msg.payload.object.uuid));
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -1889,6 +1919,9 @@ static int vhost_setup_backend_channel(struct vhost_dev
*dev)
msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
}
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, &sv[1], 1);
if (ret) {
goto out;
@@ -1993,6 +2026,9 @@ static int vhost_user_postcopy_advise(struct vhost_dev
*dev, Error **errp)
.hdr.flags = VHOST_USER_VERSION,
};
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
error_setg(errp, "Failed to send postcopy_advise to vhost");
@@ -2051,6 +2087,9 @@ static int vhost_user_postcopy_listen(struct vhost_dev
*dev, Error **errp)
trace_vhost_user_postcopy_listen();
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
error_setg(errp, "Failed to send postcopy_listen to vhost");
@@ -2080,6 +2119,9 @@ static int vhost_user_postcopy_end(struct vhost_dev *dev,
Error **errp)
trace_vhost_user_postcopy_end_entry();
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
error_setg(errp, "Failed to send postcopy_end to vhost");
@@ -2372,6 +2414,10 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev,
uint16_t mtu)
msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
}
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -2396,6 +2442,10 @@ static int vhost_user_send_device_iotlb_msg(struct
vhost_dev *dev,
.payload.iotlb = *imsg,
};
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -2428,6 +2478,10 @@ static int vhost_user_get_config(struct vhost_dev *dev,
uint8_t *config,
assert(config_len <= VHOST_USER_MAX_CONFIG_SIZE);
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
msg.payload.config.offset = 0;
msg.payload.config.size = config_len;
ret = vhost_user_write(dev, &msg, NULL, 0);
@@ -2492,6 +2546,10 @@ static int vhost_user_set_config(struct vhost_dev *dev,
const uint8_t *data,
p = msg.payload.config.region;
memcpy(p, data, size);
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -2570,6 +2628,10 @@ static int vhost_user_crypto_create_session(struct
vhost_dev *dev,
}
}
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
msg.payload.session.op_code = backend_info->op_code;
msg.payload.session.session_id = backend_info->session_id;
ret = vhost_user_write(dev, &msg, NULL, 0);
@@ -2662,6 +2724,9 @@ static int vhost_user_get_inflight_fd(struct vhost_dev
*dev,
return 0;
}
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
return ret;
@@ -2757,6 +2822,7 @@ bool vhost_user_init(VhostUserState *user, CharBackend
*chr, Error **errp)
user->memory_slots = 0;
user->notifiers = g_ptr_array_new_full(VIRTIO_QUEUE_MAX / 4,
&vhost_user_state_destroy);
+ qemu_mutex_init(&user->vhost_user_request_reply_lock);
return true;
}
@@ -2769,6 +2835,7 @@ void vhost_user_cleanup(VhostUserState *user)
user->notifiers = (GPtrArray *) g_ptr_array_free(user->notifiers, true);
memory_region_transaction_commit();
user->chr = NULL;
+ qemu_mutex_destroy(&user->vhost_user_request_reply_lock);
}
@@ -2902,6 +2969,9 @@ static int vhost_user_set_device_state_fd(struct
vhost_dev *dev,
return -ENOTSUP;
}
+ struct VhostUserState *us = vu->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, &fd, 1);
close(fd);
if (ret < 0) {
@@ -2965,6 +3035,10 @@ static int vhost_user_check_device_state(struct
vhost_dev *dev, Error **errp)
return -ENOTSUP;
}
+ struct vhost_user *u = dev->opaque;
+ struct VhostUserState *us = u->user;
+ QEMU_LOCK_GUARD(&us->vhost_user_request_reply_lock);
+
ret = vhost_user_write(dev, &msg, NULL, 0);
if (ret < 0) {
error_setg_errno(errp, -ret,
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 324cd8663a..e96f12d449 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -67,6 +67,9 @@ typedef struct VhostUserState {
GPtrArray *notifiers;
int memory_slots;
bool supports_config;
+
+ /* Hold lock for a request-reply cycle */
+ QemuMutex vhost_user_request_reply_lock;
} VhostUserState;
/**
--
2.46.0
