From 5ae329305a047a68dce6f58afb83633ecff53e6a Mon Sep 17 00:00:00 2001 From: Anatol Belski Date: Wed, 1 Apr 2026 21:23:37 +0200 Subject: [PATCH] virtio-devices: block: Fix writeback mode update flow Virtio v1.2 says that if CONFIG_WCE is negotiated but FLUSH is not, the device must initialize writeback to 0. It also says that if CONFIG_WCE was not negotiated but FLUSH was, the driver should assume presence of a writeback cache. Introduce a pure is_writeback_enabled helper and a set_writeback_mode helper. This makes the two call flows explicit: * write_config resolves the guest requested mode against the negotiated features before storing it back * activate starts from the default writeback preference and then resolves it against the negotiated features * reset restores the initial writeback state This keeps the config space value and the runtime writeback flag in sync and makes the spec driven fallback easier to follow. Signed-off-by: Anatol Belski --- virtio-devices/src/block.rs | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index 1080b71bc..bd740fc4a 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -974,24 +974,24 @@ impl Block { } } - fn update_writeback(&mut self) { - // Use writeback from config if VIRTIO_BLK_F_CONFIG_WCE - let writeback = if self.common.feature_acked(VIRTIO_BLK_F_CONFIG_WCE.into()) { - self.config.writeback == 1 - } else { - // Else check if VIRTIO_BLK_F_FLUSH negotiated - self.common.feature_acked(VIRTIO_BLK_F_FLUSH.into()) - }; + /// The virtio v1.2 spec says "If VIRTIO_BLK_F_CONFIG_WCE was not + /// negotiated but VIRTIO_BLK_F_FLUSH was, the driver SHOULD assume + /// presence of a writeback cache." It also says "If + /// VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH is not, + /// the device MUST initialize writeback to 0." + fn is_writeback_enabled(&self, desired: bool) -> bool { + let flush = self.common.feature_acked(VIRTIO_BLK_F_FLUSH.into()); + let wce = self.common.feature_acked(VIRTIO_BLK_F_CONFIG_WCE.into()); + if wce { flush && desired } else { flush } + } + fn set_writeback_mode(&mut self, enabled: bool) { + self.config.writeback = enabled as u8; + self.writeback.store(enabled, Ordering::Release); info!( "Changing cache mode to {}", - if writeback { - "writeback" - } else { - "writethrough" - } + if enabled { "writeback" } else { "writethrough" } ); - self.writeback.store(writeback, Ordering::Release); } pub fn resize(&mut self, new_size: u64) -> Result<()> { @@ -1073,8 +1073,8 @@ impl VirtioDevice for Block { return; } - self.config.writeback = data[0]; - self.update_writeback(); + let writeback = self.is_writeback_enabled(data[0] == 1); + self.set_writeback_mode(writeback); } fn activate(&mut self, context: crate::device::ActivationContext) -> ActivateResult { @@ -1097,7 +1097,8 @@ impl VirtioDevice for Block { // Recompute the barrier size from the queues that are actually activated. self.common.paused_sync = Some(Arc::new(Barrier::new(queues.len() + 1))); - self.update_writeback(); + let writeback = self.is_writeback_enabled(self.config.writeback == 1); + self.set_writeback_mode(writeback); let mut epoll_threads = Vec::new(); let event_idx = self.common.feature_acked(VIRTIO_RING_F_EVENT_IDX.into()); @@ -1167,6 +1168,7 @@ impl VirtioDevice for Block { fn reset(&mut self) -> Option> { let result = self.common.reset(); + self.set_writeback_mode(true); event!("virtio-device", "reset", "id", &self.id); result }