From 7f326dd314fdea0f98a19fa039c630b50deaec0b Mon Sep 17 00:00:00 2001 From: German Maglione Date: Fri, 17 Nov 2023 13:04:54 +0100 Subject: [PATCH] vhost-user-backend: SET_FEATURES must not disable any ring The vhost-user spec has been updated to clarify enabling and disabling vrings. The current spec states that upon receiving a VHOST_USER_SET_FEATURES message from the front-end without VHOST_USER_F_PROTOCOL_FEATURES set, the back-end must enable all rings immediately. Also, while processing the rings (whether they are enabled or not), the back-end must support changing some configuration aspects on the fly. However, the vhost-user-backend implementation will disable all vring whenever this feature is negotiated, which means that every VHOST_USER_SET_FEATURES message could unintentionally halt the device. This is an incorrect interpretation of the spec, and is problematic because the VHOST_F_LOG_ALL feature is also set or cleared by VHOST_USER_SET_FEATURES, which happens during migration. And doing so should not disable any vring nor halt the device. Signed-off-by: German Maglione --- crates/vhost-user-backend/src/handler.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/vhost-user-backend/src/handler.rs b/crates/vhost-user-backend/src/handler.rs index 8be4768..9087940 100644 --- a/crates/vhost-user-backend/src/handler.rs +++ b/crates/vhost-user-backend/src/handler.rs @@ -200,6 +200,7 @@ where } } + self.vrings[index as usize].set_enabled(false); self.vrings[index as usize].set_queue_ready(true); Ok(()) @@ -247,17 +248,18 @@ where self.acked_features = features; self.features_acked = true; - // If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, - // the ring is initialized in an enabled state. - // If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, - // the ring is initialized in a disabled state. Client must not - // pass data to/from the backend until ring is enabled by - // VHOST_USER_SET_VRING_ENABLE with parameter 1, or after it has - // been disabled by VHOST_USER_SET_VRING_ENABLE with parameter 0. - let vring_enabled = - self.acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits() == 0; - for vring in self.vrings.iter_mut() { - vring.set_enabled(vring_enabled); + // Upon receiving a `VHOST_USER_SET_FEATURES` message from the front-end without + // `VHOST_USER_F_PROTOCOL_FEATURES` set, the back-end must enable all rings immediately. + // While processing the rings (whether they are enabled or not), the back-end must support + // changing some configuration aspects on the fly. + // (see https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#ring-states) + // + // Note: If `VHOST_USER_F_PROTOCOL_FEATURES` has not been negotiated we must leave + // the vrings in their current state. + if self.acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits() == 0 { + for vring in self.vrings.iter_mut() { + vring.set_enabled(true); + } } self.backend.acked_features(self.acked_features);