From 3593055e776a7b570826d43c52cba9ca1f66e261 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Mon, 21 Feb 2022 09:52:25 +0100 Subject: [PATCH] virtio-devices: Consider vhost-user protocol feature as acked For vhost-user devices, we don't want to loose the vhost-user protocol feature through the negotiation between guest and device. Since we know VIRTIO has no knowledge of the vhost-user protocol feature, there is no way it would ever be acknowledged by the guest. For that reason, we create each vhost-user device with the set of acked features containing the vhost-user protocol feature is this one was part of the available list. Having the set of acked features containing this bit allows for solving a bug that was happening through the migration process since the vhost-user protocol feature wasn't explicitely enabled. Signed-off-by: Sebastien Boeuf --- virtio-devices/src/vhost_user/blk.rs | 14 ++++++-------- virtio-devices/src/vhost_user/fs.rs | 14 ++++++-------- virtio-devices/src/vhost_user/net.rs | 21 +++++++++++++-------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/virtio-devices/src/vhost_user/blk.rs b/virtio-devices/src/vhost_user/blk.rs index d7ed7a534..b08798df5 100644 --- a/virtio-devices/src/vhost_user/blk.rs +++ b/virtio-devices/src/vhost_user/blk.rs @@ -167,7 +167,11 @@ impl Blk { device_type: VirtioDeviceType::Block as u32, queue_sizes: vec![vu_cfg.queue_size; num_queues], avail_features: acked_features, - acked_features: 0, + // If part of the available features that have been acked, the + // PROTOCOL_FEATURES bit must be already set through the VIRTIO + // acked features as we know the guest would never ack it, thus + // the feature would be lost. + acked_features: acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits(), paused_sync: Some(Arc::new(Barrier::new(2))), min_queues: DEFAULT_QUEUE_NUMBER as u16, ..Default::default() @@ -288,12 +292,6 @@ impl VirtioDevice for Blk { let slave_req_handler: Option> = None; - // The backend acknowledged features must contain the protocol feature - // bit in case it was initially set but lost through the features - // negotiation with the guest. - let backend_acked_features = self.common.acked_features - | (self.common.avail_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits()); - // Run a dedicated thread for handling potential reconnections with // the backend. let (kill_evt, pause_evt) = self.common.dup_eventfds(); @@ -303,7 +301,7 @@ impl VirtioDevice for Blk { queues, queue_evts, interrupt_cb, - backend_acked_features, + self.common.acked_features, slave_req_handler, kill_evt, pause_evt, diff --git a/virtio-devices/src/vhost_user/fs.rs b/virtio-devices/src/vhost_user/fs.rs index 7fc208677..1cb9517c5 100644 --- a/virtio-devices/src/vhost_user/fs.rs +++ b/virtio-devices/src/vhost_user/fs.rs @@ -413,7 +413,11 @@ impl Fs { common: VirtioCommon { device_type: VirtioDeviceType::Fs as u32, avail_features: acked_features, - acked_features: 0, + // If part of the available features that have been acked, the + // PROTOCOL_FEATURES bit must be already set through the VIRTIO + // acked features as we know the guest would never ack it, thus + // the feature would be lost. + acked_features: acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits(), queue_sizes: vec![queue_size; num_queues], paused_sync: Some(Arc::new(Barrier::new(2))), min_queues: DEFAULT_QUEUE_NUMBER as u16, @@ -538,12 +542,6 @@ impl VirtioDevice for Fs { None }; - // The backend acknowledged features must contain the protocol feature - // bit in case it was initially set but lost through the features - // negotiation with the guest. - let backend_acked_features = self.common.acked_features - | (self.common.avail_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits()); - // Run a dedicated thread for handling potential reconnections with // the backend. let (kill_evt, pause_evt) = self.common.dup_eventfds(); @@ -553,7 +551,7 @@ impl VirtioDevice for Fs { queues, queue_evts, interrupt_cb, - backend_acked_features, + self.common.acked_features, slave_req_handler, kill_evt, pause_evt, diff --git a/virtio-devices/src/vhost_user/net.rs b/virtio-devices/src/vhost_user/net.rs index 40b77b934..68d344c52 100644 --- a/virtio-devices/src/vhost_user/net.rs +++ b/virtio-devices/src/vhost_user/net.rs @@ -222,7 +222,11 @@ impl Net { device_type: VirtioDeviceType::Net as u32, queue_sizes: vec![vu_cfg.queue_size; num_queues], avail_features: acked_features, - acked_features: 0, + // If part of the available features that have been acked, the + // PROTOCOL_FEATURES bit must be already set through the VIRTIO + // acked features as we know the guest would never ack it, thus + // the feature would be lost. + acked_features: acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits(), paused_sync: Some(Arc::new(Barrier::new(2))), min_queues: DEFAULT_QUEUE_NUMBER as u16, ..Default::default() @@ -261,9 +265,13 @@ impl Net { self.vu_common.acked_protocol_features = state.acked_protocol_features; self.vu_common.vu_num_queues = state.vu_num_queues; + // The backend acknowledged features must not contain VIRTIO_NET_F_MAC + // since we don't expect the backend to handle it. + let backend_acked_features = self.common.acked_features & !(1 << VIRTIO_NET_F_MAC); + if let Err(e) = self .vu_common - .restore_backend_connection(self.common.acked_features) + .restore_backend_connection(backend_acked_features) { error!( "Failed restoring connection with vhost-user backend: {:?}", @@ -355,12 +363,9 @@ impl VirtioDevice for Net { let slave_req_handler: Option> = None; - // The backend acknowledged features must contain the protocol feature - // bit in case it was initially set but lost through the features - // negotiation with the guest. Additionally, it must not contain - // VIRTIO_NET_F_MAC since we don't expect the backend to handle it. - let backend_acked_features = self.common.acked_features & !(1 << VIRTIO_NET_F_MAC) - | (self.common.avail_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits()); + // The backend acknowledged features must not contain VIRTIO_NET_F_MAC + // since we don't expect the backend to handle it. + let backend_acked_features = self.common.acked_features & !(1 << VIRTIO_NET_F_MAC); // Run a dedicated thread for handling potential reconnections with // the backend.