-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add support for virtio device reset #5891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dc09afb
03603b0
140072a
05ea8e3
ff1e508
fb227c3
ad333db
43500bc
17501db
05d33f0
511ed8c
d03aae7
ebd0dbf
d0bdb3b
2e6d6f9
dfacf7e
93f3bba
cd9e5df
29a6f2e
f27986e
7528064
208119d
a041c80
93dbf36
6f0ab8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -379,6 +379,14 @@ where | |
| fn is_activated(&self) -> bool { | ||
| self.device_state.is_activated() | ||
| } | ||
|
|
||
| fn deactivate(&mut self) { | ||
| self.device_state = DeviceState::Inactive; | ||
| } | ||
|
|
||
| fn _reset(&mut self) -> bool { | ||
| false | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning false here combined with the new MMIO path means any status=0 write after ACKNOWLEDGE permanently sets FAILED for vhost-user-block.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted in the other comment, even before this PR we'd set the device to FAILED state if we got a reset after DRIVER_OK. Does it matter now that this can happen after ACKNOWLEDGE? |
||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -674,6 +674,19 @@ impl VirtioDevice for VirtioBlock { | |
| fn is_activated(&self) -> bool { | ||
| self.device_state.is_activated() | ||
| } | ||
|
|
||
| fn deactivate(&mut self) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about the rate limiters? are we assuming we want to preserve the state across reset?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we must since otherwise guest can bypass rate limiter by resetting |
||
| self.device_state = DeviceState::Inactive; | ||
| } | ||
|
|
||
| fn _reset(&mut self) -> bool { | ||
| if let Err(err) = self.disk.file_engine.reset() { | ||
| error!("Failed to reset block IO engine: {:?}", err); | ||
| return false; | ||
| } | ||
| self.is_io_engine_throttled = false; | ||
|
ilstam marked this conversation as resolved.
|
||
| true | ||
| } | ||
| } | ||
|
|
||
| impl Drop for VirtioBlock { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,12 +178,27 @@ pub trait VirtioDevice: AsAny + MutEventSubscriber + Send { | |
| /// Checks if the resources of this device are activated. | ||
| fn is_activated(&self) -> bool; | ||
|
|
||
| /// Optionally deactivates this device and returns ownership of the guest memory map, interrupt | ||
| /// event, and queue events. | ||
| fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> { | ||
| None | ||
| /// Set the device state to Inactive | ||
| fn deactivate(&mut self); | ||
|
|
||
| /// Reset the device. Returns true on success, false otherwise. | ||
| /// It must not be overridden. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should "must not be overridden" be enforced somehow?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I felt the same, but unfortunately Rust doesn't seem to have a |
||
| fn reset(&mut self) -> bool { | ||
| if !self._reset() { | ||
| return false; | ||
| } | ||
| self.deactivate(); | ||
| self.set_acked_features(0); | ||
| for queue in self.queues_mut() { | ||
| *queue = Queue::new(queue.max_size); | ||
| } | ||
| true | ||
| } | ||
|
|
||
| /// Backend-specific reset logic. Returns true on success, false if the | ||
| /// backend does not support reset. | ||
| fn _reset(&mut self) -> bool; | ||
|
|
||
| /// Mark pages used by queues as dirty. | ||
| fn mark_queue_memory_dirty(&mut self, mem: &GuestMemoryMmap) -> Result<(), QueueError> { | ||
| for queue in self.queues_mut() { | ||
|
|
@@ -310,6 +325,14 @@ pub(crate) mod tests { | |
| fn is_activated(&self) -> bool { | ||
| todo!() | ||
| } | ||
|
|
||
| fn deactivate(&mut self) { | ||
| todo!() | ||
| } | ||
|
|
||
| fn _reset(&mut self) -> bool { | ||
| todo!() | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we check if we need to upated other documentation files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other documentation files did you have in mind?