Skip to content

Add support for virtio device reset#5891

Open
ilstam wants to merge 28 commits into
firecracker-microvm:mainfrom
ilstam:device-reset
Open

Add support for virtio device reset#5891
ilstam wants to merge 28 commits into
firecracker-microvm:mainfrom
ilstam:device-reset

Conversation

@ilstam

@ilstam ilstam commented May 15, 2026

Copy link
Copy Markdown
Contributor

Add support for virtio device reset

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

ilstam added a commit to ilstam/firecracker that referenced this pull request May 15, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
@ilstam ilstam requested review from Manciukic and micz010 as code owners May 15, 2026 16:40
@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 46.19048% with 113 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (fac4efd) to head (857bf16).

Files with missing lines Patch % Lines
src/vmm/src/devices/virtio/vsock/unix/muxer.rs 0.00% 19 Missing ⚠️
src/vmm/src/devices/virtio/block/virtio/device.rs 0.00% 11 Missing ⚠️
src/vmm/src/devices/virtio/balloon/device.rs 0.00% 10 Missing ⚠️
src/vmm/src/devices/virtio/block/device.rs 0.00% 10 Missing ⚠️
src/vmm/src/devices/virtio/vsock/device.rs 0.00% 9 Missing ⚠️
src/vmm/src/devices/virtio/vsock/packet.rs 0.00% 8 Missing ⚠️
.../vmm/src/devices/virtio/block/vhost_user/device.rs 0.00% 6 Missing ⚠️
...vmm/src/devices/virtio/block/virtio/io/async_io.rs 0.00% 6 Missing ⚠️
src/vmm/src/devices/virtio/mem/device.rs 0.00% 6 Missing ⚠️
src/vmm/src/devices/virtio/pmem/device.rs 0.00% 6 Missing ⚠️
... and 10 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5891      +/-   ##
==========================================
- Coverage   83.03%   82.82%   -0.21%     
==========================================
  Files         277      277              
  Lines       30212    30369     +157     
==========================================
+ Hits        25087    25154      +67     
- Misses       5125     5215      +90     
Flag Coverage Δ
5.10-m5n.metal 83.11% <46.19%> (-0.24%) ⬇️
5.10-m6a.metal 82.45% <46.19%> (-0.24%) ⬇️
5.10-m6g.metal 79.83% <46.19%> (-0.18%) ⬇️
5.10-m6i.metal 83.11% <46.19%> (-0.24%) ⬇️
5.10-m7a.metal-48xl 82.44% <46.19%> (-0.24%) ⬇️
5.10-m7g.metal 79.83% <46.19%> (-0.18%) ⬇️
5.10-m7i.metal-24xl 83.07% <46.19%> (-0.24%) ⬇️
5.10-m7i.metal-48xl 83.08% <46.19%> (-0.24%) ⬇️
5.10-m8g.metal-24xl 79.83% <46.19%> (-0.17%) ⬇️
5.10-m8g.metal-48xl 79.83% <46.19%> (-0.18%) ⬇️
5.10-m8i.metal-48xl 83.08% <46.19%> (-0.24%) ⬇️
5.10-m8i.metal-96xl 83.08% <46.19%> (-0.24%) ⬇️
6.1-m5n.metal 83.13% <46.19%> (-0.23%) ⬇️
6.1-m6a.metal 82.47% <46.19%> (-0.25%) ⬇️
6.1-m6g.metal 79.83% <46.19%> (-0.18%) ⬇️
6.1-m6i.metal 83.13% <46.19%> (-0.23%) ⬇️
6.1-m7a.metal-48xl 82.47% <46.19%> (-0.24%) ⬇️
6.1-m7g.metal 79.83% <46.19%> (-0.18%) ⬇️
6.1-m7i.metal-24xl 83.15% <46.19%> (-0.24%) ⬇️
6.1-m7i.metal-48xl 83.14% <46.19%> (-0.25%) ⬇️
6.1-m8g.metal-24xl 79.83% <46.19%> (-0.18%) ⬇️
6.1-m8g.metal-48xl 79.83% <46.19%> (-0.18%) ⬇️
6.1-m8i.metal-48xl 83.15% <46.19%> (-0.24%) ⬇️
6.1-m8i.metal-96xl 83.15% <46.19%> (-0.24%) ⬇️
6.18-m5n.metal 83.13% <46.19%> (-0.23%) ⬇️
6.18-m6a.metal 82.47% <46.19%> (-0.25%) ⬇️
6.18-m6g.metal 79.83% <46.19%> (-0.18%) ⬇️
6.18-m6i.metal 83.12% <46.19%> (-0.25%) ⬇️
6.18-m7a.metal-48xl 82.46% <46.19%> (-0.24%) ⬇️
6.18-m7g.metal 79.83% <46.19%> (-0.18%) ⬇️
6.18-m7i.metal-24xl 83.14% <46.19%> (-0.25%) ⬇️
6.18-m7i.metal-48xl 83.15% <46.19%> (-0.24%) ⬇️
6.18-m8g.metal-24xl 79.83% <46.19%> (-0.18%) ⬇️
6.18-m8g.metal-48xl 79.83% <46.19%> (-0.18%) ⬇️
6.18-m8i.metal-48xl 83.15% <46.19%> (-0.24%) ⬇️
6.18-m8i.metal-96xl 83.15% <46.19%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/vmm/src/devices/virtio/transport/pci/device.rs Outdated
Comment thread src/vmm/src/devices/virtio/block/device.rs Outdated
Comment thread src/vmm/src/devices/virtio/vsock/device.rs
Comment thread tests/integration_tests/functional/test_vsock.py Outdated
Comment thread src/vmm/src/devices/virtio/mem/device.rs
Comment thread src/vmm/src/devices/virtio/device.rs Outdated
for queue in self.queues_mut() {
*queue = Queue::new(queue.max_size);
}
self._reset()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be called before the rest? This is a semantic change from the old code where a failed reset left the device activated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It left it activated in Firecracker but set the state as FAILED so in theory the guest shouldn't continue using it. But does it matter much since we'll support reset for all devices except for vhost-user-block (for now)?

@bk0v bk0v May 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deactivate(), set_acked_features(0), and the queue wipe all run before _reset() is consulted. For a backend that returns false (vhost-user-block), the VMM-side state is already mutated while the external backend was never told to stop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I re-ordered the links and made reset() call _reset() first thing. However, obviously the device will end up in a FAILED state regardless.

Comment thread src/vmm/src/devices/virtio/balloon/device.rs
Comment thread src/vmm/src/devices/virtio/block/virtio/device.rs
Comment thread tests/integration_tests/functional/test_balloon.py
Comment thread tests/integration_tests/functional/test_pmem.py Outdated
Comment thread src/vmm/src/devices/virtio/device.rs Outdated
Comment thread src/vmm/src/devices/virtio/block/device.rs Outdated
ilstam added a commit to ilstam/firecracker that referenced this pull request May 21, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request May 21, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request May 21, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request May 21, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request May 21, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request May 21, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request May 26, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
@ilstam ilstam added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label May 26, 2026
Comment thread src/vmm/src/devices/virtio/block/virtio/device.rs
ilstam added a commit to ilstam/firecracker that referenced this pull request Jun 8, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Jun 8, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added a commit to ilstam/firecracker that referenced this pull request Jun 9, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Comment thread src/vmm/src/devices/virtio/device.rs Outdated
for queue in self.queues_mut() {
*queue = Queue::new(queue.max_size);
}
self._reset()

@bk0v bk0v May 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deactivate(), set_acked_features(0), and the queue wipe all run before _reset() is consulted. For a backend that returns false (vhost-user-block), the VMM-side state is already mutated while the external backend was never told to stop

}

fn _reset(&mut self) -> bool {
false

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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?

} else if status == INIT {
{
let mut locked_device = self.device.lock().expect("Poisoned lock");
if locked_device.is_activated() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was dropping the is_activated() guard intentional? Previously the device-level reset only ran when is_activated(). Now it runs whenever device_status != INIT. Combined with _reset() -> false backends this set FAILED for resets issued during early init

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was intentional yes, as it was explicitly called out in the commit message. As far as I can tell the "activated" state is a Firecracker term rather than a virtio spec term. I don't think the spec says we must ignore resets before reaching a certain state such as DRIVER_OK. It's true that for vhost-user-block an early reset will lead to a FAILED state, but is this a problem? Already before this PR a reset after DRIVER_OK would lead to a FAILED state, does it matter that it now leads to a FAILED state even before DRIVER_OK?

fn deactivate(&mut self);

/// Reset the device. Returns true on success, false otherwise.
/// It must not be overridden.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should "must not be overridden" be enforced somehow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 final keyword like C++. I'm open to suggestions though if you can think of a different way to do it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can be enforces by moving this reset into a free standing reset(device: &mut impl VirtioDevice) function, and then _reset can become reset in the trait.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we could do that, but then what stops callers from calling VirtioDevice::reset() directly instead of going through the free-standing reset()? I think we would end up with a similar comment // Do not invoke this directly, it must only be invoked by the free-standing reset().

ilstam added a commit to ilstam/firecracker that referenced this pull request Jun 10, 2026
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam added 27 commits June 25, 2026 14:58
Simplify the MMIO transport reset logic by always performing the device
reset regardless of activation state.

This requires updating the affected unit tests and implementing reset()
for the DummyDevice. The unit test assumed that reset() is never
supported (i.e. always returns false), but that is going to change soon
so we want to make sure that resetting moves the device to the
deactivated state.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The reset success path locked self.device, then tried to lock it again
via self.virtio_device().lock() to reset queues. This was a deadlock
that was never triggered because no device previously implemented
reset() (all returned false). Use the already-held guard instead.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Reset device_feature_select and driver_feature_select to 0 on device
reset, matching what the MMIO transport does with features_select and
acked_features_select.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a reset_msix() method that clears the MSI-X configuration,
unregisters irqfds and removes the GSI routes from the routing table.

Add the eventfd2 system call to the vCPU seccomp filter to allow
KvmVm::create_msix_group() create new eventfds for interrupts from
within a vCPU thread (where the reset happens).

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add test_bus_device_reset_failure to verify that when device.reset()
returns false, the FAILED bit is set in device_status.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a deactivate() method that sets the device state to Inactive.
This will be used by the generic reset implementation in a subsequent
patch.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
A guest may add buffers to a queue and ring the notification doorbell as
soon as it sets DRIVER_OK, before Firecracker has finished activating
the device. If the notification eventfd is signalled while the device is
still in the inactive state, the device's event handler discards it as
spurious. At the moment the handler doesn't clear the eventfd, so the
handler will re-run again and again until the device is finally
activated. A subsequent commit will drain the eventfds when a spurious
notification arrives. That means that the notification will be lost: the
buffers the guest made available sit in the queue and are never
processed.

Notify the queue eventfds once activation completes so the device
processes any buffers that were made available during the activation
window.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The queue notification eventfds stay registered with the event manager
and with KVM across a device reset. As a result a notification that
arrives while the device is deactivated - for example a guest ringing a
doorbell during the reset/re-init window - would be reported by the
event manager over and over, spinning the event loop until the device is
activated again.

Drain the queue eventfds in the not-activated branch of each device's
event handler so the pending state is cleared and the spurious
notification is discarded.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Move the queue reset logic from the MMIO and PCI transport code into
the default reset() implementation in the VirtioDevice trait. This is
generic virtio state that should be reset for all devices, regardless
of transport.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Complete the reset() implementation by deactivating the device and
setting acked_features to 0. This must happen for all virtio backends.

Introduce a _reset() method that is called at the beginning of reset()
and can be overridden by virtio backends with backend specific reset
code. Make all backends return false for now since none of them supports
reset yet.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a clear() method to RxBuffers that resets all fields to their
initial state. This will be used by the virtio-net reset implementation.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Implement the _reset() method for the virtio-net device, resetting the
net specific state in-place.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-net device reset works end-to-end by
unbinding and rebinding the guest driver and checking the device
remains functional.

Suggested-by: Adam Jensen <adam@acj.sh>
Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Resetting an async block device recreates its io_uring instance. This
runs on the vCPU thread.

Allow the syscalls that IoUring::new() needs on the vCPU thread:
io_uring_setup, io_uring_register, the MAP_SHARED|MAP_POPULATE mmap used
to map the rings (mirroring the existing vmm rule), and getrandom.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Implement the _reset() method for the virtio-block device, resetting the
device-specific state in-place. For the async io_uring engine, replace
the ring with a fresh one so that any in-flight operations are cancelled
by the kernel when the old ring fd is closed.

This only adds support for the Virtio backend for now and not VhostUser.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-block device reset works end-to-end by
unbinding and rebinding the guest driver for a scratch block device
and checking the device remains functional.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Pmem does not have any backend specific state that needs resetting so
implement the _reset() method by simply returning true. The rest of the
state is handled by the generic reset().

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-pmem device reset works end-to-end by
unbinding and rebinding the guest driver and checking the device
remains functional.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Implement the _reset() method for the virtio-balloon device, resetting
the balloon specific state in-place. Do not deflate the balloon on
reset.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-balloon device reset works end-to-end
by unbinding and rebinding the guest driver and checking the device
remains functional.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The entropy device does not have any backend specific state that needs
resetting so implement the _reset() method by simply returning true.

The test_failed_reset_blocks_reinitialization() test used an entropy
device assuming it doesn't support reset. Since it now does, adjust the
test to check that the device is first deactivated after a reset and
then the driver can perform the initialization sequence again.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-rng device reset works end-to-end by
unbinding and rebinding the guest driver and checking the device
remains functional.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Implement the _reset() method for the virtio-vsock device, resetting the
vsock specific state in-place.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-vsock device reset works end-to-end by
unbinding and rebinding the guest driver and checking the device
remains functional.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
The virtio-mem device does not have any backend specific state that
needs resetting so implement the _reset() method by simply returning
true. Plugged memory regions remain intact.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a test that verifies virtio-mem device reset works end-to-end by
unbinding and rebinding the guest driver and checking the device remains
functional.

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
firecracker-microvm#5891

Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
@ilstam ilstam added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants