Maintenance5#5937
Open
ShadowCurse wants to merge 11 commits into
Open
Conversation
ShadowCurse
force-pushed
the
maintenance5
branch
2 times, most recently
from
02e7fd5 to
05d557b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5937 +/- ##
==========================================
- Coverage 83.03% 83.00% -0.04%
==========================================
Files 277 277
Lines 30212 30164 -48
==========================================
- Hits 25087 25037 -50
- Misses 5125 5127 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ShadowCurse
force-pushed
the
maintenance5
branch
6 times, most recently
from
6512ffb to
b8a7ce2
Compare
ShadowCurse
force-pushed
the
maintenance5
branch
from
b8a7ce2 to
db3250f
Compare
ShadowCurse
force-pushed
the
maintenance5
branch
from
db3250f to
addf162
Compare
Remove the local `type Result<T> = result::Result<T, BusError>` alias from vstate/bus.rs. Replace all usages with explicit `Result<T, BusError>` for clarity and consistency with the rest of the codebase. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Replace the runtime assert! in Endpoint::new with a compile-time const assertion at the point where RCV_BUF_MAX_SIZE is defined. This enforces the invariant (RCV_BUF_MAX_SIZE <= MAX_WINDOW_SIZE) at compile time rather than at runtime. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
When a network device configuration is submitted with an existing device ID but different parameters, the previous device is silently destroyed and replaced. The ordering is justified by the fact that the new configuration can use same tap device, so we need to destroy old net device before creating a new one. The issue here is that if the new device fails to create, there is no old device to plug back. Add a warning log so the caller has visibility into this destructive operation. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Replace the Peekable iterator pattern in descriptor chain construction with windows(2) for add_scatter_gather and enumerate with get(i+1) for add_desc_chain. This makes the intent clearer: each iteration processes a descriptor and links it to the next one in the chain. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Bus::insert is only called during VMM setup with known addresses. A failure (zero-sized range, overflow, or overlap) indicates a programming error in the address layout, not a recoverable runtime condition. Replace the Result return type with asserts that panic on invariant violations. Bus::remove is only called with information we previously obtained when inserting the device, meaning the information must be valid. Remove the now-unused BusError::Overlap variant and the Bus(BusError) variants from MmioError, LegacyDeviceError, PciManagerError, AttachDeviceError, DevicePersistError, and DeviceManagerPersistError. PciSegment::new and PciSegment::build also no longer return Result since their only fallible operation was Bus::insert. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Instead of having both HashMap and Vec, use single flat array of optionals. This simplifies the implementation and removes need for any memory allocations. Storage is efficient as well since `Option<Arc<...>>` is a simple pointer and there is a limit of 32 device in general. The only notable difference this brings is that the logic of obtaining the device id and adding the device now is more coupled togather since now there is no option to "reserve" a device_id. This is not an issue since all code always does follow the "obtain id -> add device" scheme. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
RateLimiter `new` was returning Result even though it cannot fail. Remove that and update all relevant code paths. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Instead of manually specifying each byte for BUST_TYPE_ISA use fancy *b"..." syntax. This supposed to be easier to read. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
These debug logs only clutter the log output stream. This is especially bad in our CI since it makes useful logs to be buried under the flood of vsock logs. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Make `warn!` logs used when guest incorrectly reads/writes the config space of the device more coherent across devices. This also makes them provide more useful information (like what device emitted the log). Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Do a single allocation when setting irq routing instead of pushing to KvmIrqRouting multiple times. Saves us from dynamic re-alloations. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse
force-pushed
the
maintenance5
branch
from
addf162 to
6c12c47
Compare
| .devices | ||
| .write() | ||
| .unwrap() | ||
| .insert(new_range, Arc::downgrade(&device)) |
Contributor
There was a problem hiding this comment.
Was there a bug here before? If another piece of code obtained the lock between the check above and here, and inserted a device that overlaps, this insert call would have replaced the device despite this function returning an error.
Am I understanding correctly?
Contributor
Author
There was a problem hiding this comment.
yes and no. If this code could execute in parallel then this could happen. But all the bus manipulation happens on the VMM thread, so race could happen.
marco-marangoni
requested changes
Jun 25, 2026
| ); | ||
|
|
||
| Ok(()) | ||
| devices.insert(new_range, Arc::downgrade(&device)); |
Contributor
There was a problem hiding this comment.
If we decide to keep assert for this function, we should assert the returned value is None here for extra peace of mind.
| /// | ||
| /// Panics if `len` is zero, if the range overflows, or if the new range overlaps with an | ||
| /// existing device. These conditions indicate a bug in the VMM address layout. | ||
| pub fn insert(&self, device: Arc<dyn BusDeviceSync>, base: u64, len: u64) { |
Contributor
There was a problem hiding this comment.
I don't agree with using asserts instead of Result here. It's not obvious for the caller to know whether the given range overlaps or not, and it's also impossible to check at runtime, since devices are under a lock that might change between when the caller checks and the actual insertion.
I agree that current calls are static, that might change in the future.
Contributor
Author
There was a problem hiding this comment.
inputs into this (and remove) functions are obtained from ResourceAllocator and so must always be valid (ResourceAllocator must never return overlapping regions). If we would need to check the range at runtime (like if we implement BAR relocation) we would need to ask ResourceManager anyway to reserve that range and so the check if the range is used or not will be handled there. But if we would really need is_overlapping function in the Bus, we can add it.
| } | ||
|
|
||
| /// Removes the device at the given address space range. | ||
| pub fn remove(&self, base: u64, len: u64) -> Result<(), BusError> { |
Contributor
There was a problem hiding this comment.
Similarly to insert, a Result here is better because preconditions aren't obvious or possible to validate at runtime
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
See commits
Reason
Maintenance
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
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.