virtio: restore the PCI config space before reinstalling the doorbells - #4146
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
This PR fixes a virtio PCI restore-order bug where queue doorbells weren’t reinstalled after restoring from saved state because BAR0 wasn’t active yet when restore_common() attempted to compute the notify address. Restoring the PCI config space earlier ensures BARs are available before doorbell reinstallation, avoiding persistent MMIO exits after resume.
Changes:
- Restore PCI config space before
restore_common()sodoorbell_region()can see an active BAR0 and reinstall queue doorbells during restore. - Strengthen
pci_restore_reinstalls_doorbellsto restore into a “fresh” device (unprogrammed BARs) and assert the exact installed doorbell set (address + datamatch + length). - Enhance the test memory backend to track currently-live doorbell registrations (not just a cumulative registration count).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
vm/devices/virtio/virtio/src/transport/pci.rs |
Reorders restore steps so PCI config space (incl. BARs) is restored before doorbell installation occurs in restore_common(). |
vm/devices/virtio/virtio/src/tests.rs |
Improves PCI restore test to reflect real restore conditions and adds live doorbell tracking to validate reinstalls precisely. |
`VirtioPciDevice::restore` called `restore_common` first, and that is where `install_doorbells` runs. At that point the config space had not been restored, so `doorbell_region` read `bar_address(0)` with no active BAR, answered `None`, and no queue doorbell was installed. The only other install site is the guest writing DRIVER_OK, which a guest resumed mid-flight never does again, so the omission lasted for the life of the VM. Correctness was unaffected, since the notify write falls through to `mmio_write` and `notify_queue`. That is why it took counting exits to notice: on a restored virtio-net NIC every one of the guest's kicks at BAR0's notify register left the kernel as an MMIO exit, measured across three boots, where the same VM before its snapshot took none. With the config space restored first, both sides take none. The existing test could not see this. It programmed the target device's BARs by hand before restoring, which is not what a restore does. It now restores into a device with unprogrammed BARs, the way production does, and asserts the installed doorbell set (address plus per-queue datamatch) rather than a registration count, which cannot tell a reinstall from an install that happened once before the save.
912d27e to
dc6abbc
Compare
|
Thanks for the fix! |
|
Uh, You are motivated, working on sundays ... |
d31d664
into
microsoft:main
VirtioPciDevice::restorerestored the PCI config space last, afterrestore_common.restore_commonis whereinstall_doorbellsruns, and itasks
doorbell_regionfor the notify address, which readsbar_address(0).With the config space not yet restored there is no active BAR, so it answers
Noneand no queue doorbell is installed.Nothing recovers from that later. The only other install site is the guest
writing DRIVER_OK, and a guest resumed mid-flight never writes it again, so a
restored device runs without doorbells for the life of the VM.
The effect is performance, not correctness: the notify write still reaches
notify_queuethroughmmio_write. On a restored virtio-net NIC every one ofthe guest's kicks at BAR0's notify register left the kernel as an MMIO exit,
across three boots, where the same VM before its snapshot took none. With the
config space restored first, both sides take none.
The fix moves the existing
config_space.restorecall ahead ofrestore_common. Nothing inrestore_commonreads the interrupt state that isrestored after it, so the order of the rest is unchanged.
On the test
pci_restore_reinstalls_doorbellspassed throughout, because it programmed thetarget device's BARs by hand before restoring. A real restore does not do that,
which is exactly the condition that hides the bug.
It now restores into a device with unprogrammed BARs, and asserts the set of
installed doorbells rather than a registration count. The count could not tell a
reinstall from an install that happened once before the save, and the address
alone is not enough either: every queue of one device registers at the same
notify address and is distinguished by its datamatch value.
Against the unfixed code the test now fails with no doorbells installed where
two are expected.